Fix prefix parameter with link#6307
Draft
thomas-topway-it wants to merge 34 commits into
Draft
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenames the parameter in ValueTextsBuilder::getValueText to $dataValue and imports SMWWikiPageValue. Internal logic now computes $useLongText and $dataValueMethod, obtains a local $linker via getLinkerForColumn, and for SMWWikiPageValue sets its OPTION form (PREFIXED_FORM or SHORT_FORM) on the data value before generating text; rendering uses $dataValue->$dataValueMethod with the local linker and result is passed through sanitizeValueText. TableResultPrinter introduces an $isHtmlOutput/$parseAsWikitext path: for files/images/blobs/wiki-page values it pre-renders the raw value with the wiki output and linker and runs Message::get on that raw output; otherwise it caches a local $linker and pre-sets each data value’s rendering option before rendering. Existing separator/formatting and fallback flows are preserved. A reference to the prefix/link issue is included. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Query/ResultPrinters/ListResultPrinter/ValueTextsBuilder.php (1)
65-70:⚠️ Potential issue | 🟡 MinorFix PHPDoc parameter name mismatch.
Line 65 still documents
@param SMWDataValue $value, but the signature at Line 70 is$dataValue. Please align the docblock to avoid confusion in static analysis and maintenance.📝 Suggested doc fix
- * `@param` SMWDataValue $value + * `@param` SMWDataValue $dataValue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Query/ResultPrinters/ListResultPrinter/ValueTextsBuilder.php` around lines 65 - 70, Update the PHPDoc for getValueText to match the actual parameter names and types: change the `@param` line that currently says "@param SMWDataValue $value" to "@param SMWDataValue $dataValue" (and keep the existing "@param int $column" and "@return string") so the docblock matches the method signature of getValueText(SMWDataValue $dataValue, $column = 0).
🧹 Nitpick comments (1)
src/Query/ResultPrinters/TableResultPrinter.php (1)
330-331: Move linker resolution outside the value loop.Line 330 resolves the same linker on every iteration. Resolve it once before the loop to avoid redundant calls and keep per-cell rendering state stable.
♻️ Suggested refactor
$values = []; + $linker = $this->getLinker( $isSubject ); foreach ( $dataValues as $dv ) { - $linker = $this->getLinker( $isSubject ); $dataItem = $dv->getDataItem();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Query/ResultPrinters/TableResultPrinter.php` around lines 330 - 331, The code calls $this->getLinker($isSubject) inside the per-value loop (see TableResultPrinter::$linker and $dv->getDataItem usage), causing repeated resolution and unstable per-cell state; move the $linker = $this->getLinker($isSubject) call to just before the loop that iterates over values (so it runs once), then use that resolved $linker variable inside the loop when rendering each cell/value (keep $dv->getDataItem() where it belongs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Query/ResultPrinters/TableResultPrinter.php`:
- Around line 342-360: Add PHPUnit regression tests covering the changed
rendering paths in TableResultPrinter: exercise prefix+link combinations and the
parse branch by creating tests that assert output for prefix=none & link=none
(both subject and non-subject columns), prefix=subject & link=none, and HTML
output paths for DIWikiPage(NS_FILE) and DIBlob; specifically invoke the
rendering flow that exercises TableResultPrinter and the
ListResultPrinter\ValueTextsBuilder->getValueText behavior, include cases that
trigger $parseAsWikitext (Message::get PARSE) and the $dataValueMethod branches
(getLongText vs short form), and assert the produced HTML/text to prevent
regressions.
---
Outside diff comments:
In `@src/Query/ResultPrinters/ListResultPrinter/ValueTextsBuilder.php`:
- Around line 65-70: Update the PHPDoc for getValueText to match the actual
parameter names and types: change the `@param` line that currently says "@param
SMWDataValue $value" to "@param SMWDataValue $dataValue" (and keep the existing
"@param int $column" and "@return string") so the docblock matches the method
signature of getValueText(SMWDataValue $dataValue, $column = 0).
---
Nitpick comments:
In `@src/Query/ResultPrinters/TableResultPrinter.php`:
- Around line 330-331: The code calls $this->getLinker($isSubject) inside the
per-value loop (see TableResultPrinter::$linker and $dv->getDataItem usage),
causing repeated resolution and unstable per-cell state; move the $linker =
$this->getLinker($isSubject) call to just before the loop that iterates over
values (so it runs once), then use that resolved $linker variable inside the
loop when rendering each cell/value (keep $dv->getDataItem() where it belongs).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Query/ResultPrinters/ListResultPrinter/ValueTextsBuilder.phpsrc/Query/ResultPrinters/TableResultPrinter.php
Comment on lines
+342
to
+360
| // @see ListResultPrinter\ValueTextsBuilder -> getValueText | ||
| $dv->setOption( | ||
| $dataValueMethod === 'getLongText' | ||
| ? $dv::PREFIXED_FORM | ||
| : $dv::SHORT_FORM, | ||
| true | ||
| ); | ||
|
|
||
| if ( $parseAsWikitext ) { | ||
| $raw = $dv->$dataValueMethod( SMW_OUTPUT_WIKI, $linker ); | ||
|
|
||
| // Too lazy to handle the Parser object and besides the Message | ||
| // parse does the job and ensures no other hook is executed | ||
| $value = Message::get( | ||
| [ 'smw-parse', $dv->$dataValueMethod( SMW_OUTPUT_WIKI, $this->getLinker( $isSubject ) ) ], | ||
| [ 'smw-parse', $raw ], | ||
| Message::PARSE | ||
| ); | ||
| } else { | ||
| $value = $dv->$dataValueMethod( $outputMode, $this->getLinker( $isSubject ) ); | ||
| $value = $dv->$dataValueMethod( $outputMode, $linker ); |
Contributor
There was a problem hiding this comment.
Add regression tests for the prefix + link interaction and parse branch.
Lines 342-360 change the core rendering path used to fix issue #6305. Please add PHPUnit coverage for at least:
prefix=none | link=none(subject + non-subject columns),prefix=subject | link=none,- HTML output paths hitting
DIWikiPage(NS_FILE)andDIBlob.
Without this, this fix can regress silently.
I can draft a focused test matrix and skeleton tests if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Query/ResultPrinters/TableResultPrinter.php` around lines 342 - 360, Add
PHPUnit regression tests covering the changed rendering paths in
TableResultPrinter: exercise prefix+link combinations and the parse branch by
creating tests that assert output for prefix=none & link=none (both subject and
non-subject columns), prefix=subject & link=none, and HTML output paths for
DIWikiPage(NS_FILE) and DIBlob; specifically invoke the rendering flow that
exercises TableResultPrinter and the
ListResultPrinter\ValueTextsBuilder->getValueText behavior, include cases that
trigger $parseAsWikitext (Message::get PARSE) and the $dataValueMethod branches
(getLongText vs short form), and assert the produced HTML/text to prevent
regressions.
Contributor
|
@thomas-topway-it can you rebase please? |
Contributor
Author
|
done |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Query/ResultPrinters/TableResultPrinter.php (1)
333-335: Move invariant linker lookup outside the loop (optional cleanup).
$this->getLinker( $isSubject )is loop-invariant here, so retrieving it once improves readability and avoids repeated calls.♻️ Suggested diff
protected function getCellContent( array $dataValues, $outputMode, $isSubject ) { $dataValueMethod = $this->prefixParameterProcessor->useLongText( $isSubject ) ? 'getLongText' : 'getShortText'; $isHtmlOutput = $outputMode === SMW_OUTPUT_HTML; + $linker = $this->getLinker( $isSubject ); $values = []; foreach ( $dataValues as $dv ) { - $linker = $this->getLinker( $isSubject ); $dataItem = $dv->getDataItem();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Query/ResultPrinters/TableResultPrinter.php` around lines 333 - 335, The call to $this->getLinker($isSubject) inside the foreach over $dataValues is loop-invariant; move the call outside the loop to a local variable (e.g., $linker = $this->getLinker($isSubject)) before iterating so you reuse the same linker for each $dv->getDataItem() iteration and avoid repeated method calls in the loop body (locate in TableResultPrinter:: where $dataValues is iterated).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Query/ResultPrinters/TableResultPrinter.php`:
- Around line 333-335: The call to $this->getLinker($isSubject) inside the
foreach over $dataValues is loop-invariant; move the call outside the loop to a
local variable (e.g., $linker = $this->getLinker($isSubject)) before iterating
so you reuse the same linker for each $dv->getDataItem() iteration and avoid
repeated method calls in the loop body (locate in TableResultPrinter:: where
$dataValues is iterated).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d7fe494-ccef-4160-bdda-d8417690e80d
📒 Files selected for processing (2)
src/Query/ResultPrinters/ListResultPrinter/ValueTextsBuilder.phpsrc/Query/ResultPrinters/TableResultPrinter.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Query/ResultPrinters/ListResultPrinter/ValueTextsBuilder.php
paladox
reviewed
Mar 16, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Query/ResultPrinters/TableResultPrinter.php (1)
334-336: Consider hoisting$linkeroutside the loop.
$isSubjectis constant throughout the loop, so$this->getLinker($isSubject)returns the same value for every iteration. Moving it before theforeachavoids redundant calls.♻️ Suggested refactor
+ $linker = $this->getLinker( $isSubject ); + $values = []; foreach ( $dataValues as $dv ) { - $linker = $this->getLinker( $isSubject ); $dataItem = $dv->getDataItem();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Query/ResultPrinters/TableResultPrinter.php` around lines 334 - 336, Hoist the repeated call to $this->getLinker($isSubject) out of the foreach over $dataValues by assigning it once to $linker before the loop; update the loop to use that single $linker variable (leave $isSubject unchanged) so getLinker is not invoked on every iteration when its input is constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Query/ResultPrinters/TableResultPrinter.php`:
- Around line 334-336: Hoist the repeated call to $this->getLinker($isSubject)
out of the foreach over $dataValues by assigning it once to $linker before the
loop; update the loop to use that single $linker variable (leave $isSubject
unchanged) so getLinker is not invoked on every iteration when its input is
constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a5b4ddb-b6ff-4f35-827f-7d0f359957bd
📒 Files selected for processing (1)
src/Query/ResultPrinters/TableResultPrinter.php
Contributor
Author
|
done, there are test errors (not sure if they belong to this PR) but not syntax errors |
Contributor
|
Think the errors belong to this change (as it's not happening to others). It appears for one test I looked at, it should be Category:Project group C (expected) but instead it's Project group C (actual). |
Contributor
Author
|
ok, so we should decide whether to update the test or the current source of truth which is the following: not sure about this, I have asked @krabina |
Contributor
|
since the default result of a query like is to don't show the prefix (in this case the Glossary:-namespace, I think the test has to be updated to reflect this. |
Member
|
I am not sure if I understand the issue... the namespace should not be removed when @kghbln any thoughts? |
paladox
reviewed
Apr 10, 2026
paladox
reviewed
Apr 10, 2026
Contributor
Author
@jaideraf sure, our doubts are only about how to reconcile tests with the current logic, since there is a mismatch (not related to the link option, but to whether we should use a prefix or not) |
Contributor
Author
|
or alternatively this in this case prefix=subject is interpreted as: use prefix only for subjects but no prefix for other columns (in the other solution the prefix for other columns is auto) |
Member
@thomas-topway-it, but if I remember well, the test that was failing at the time was about the link option; it wasn't about the prefix option. So I disagree with the affirmation "the test has to be updated to reflect this." I don't think the test (about the link option) should be updated to accommodate a change that was not used in the specific test at all. I should have talked more clearly, but I was not sure if I had understood the point. Now, when the majority of the tests pass, we can analyze again what specific tests are falling. If they are related to the link option or the prefix option. |
Member
My point is: the default result of a query like this (look at the absence of the prefix option) is TO SHOW the namespace prefixes because it could return pages with the same title in different namespaces. So, if it omits the namespaces by default, it could lead the reader to think they are talking about the same entity (same page). |
Contributor
Author
yes, the proposed implementation takes into account your point |
Contributor
Author
|
@jaideraf and @krabina, I'm about to finalize this. So I plan to use regarding the question about the semantic of otherwise the meaning of |
Contributor
Author
|
@paladox @alistair3149 So currently there are a few tests that are failing most of them consistent to the behavior of |
Contributor
|
Seems like the errors are related to this pull as they are only caused by this pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SMW\Query\ResultPrinters\TableResultPrinter -> getCellContent