Open
Conversation
… with mediawiki version>1.35
…manticProperties, props are saved as specialProps
…ditional datatypes (edtf, localMedia)
removed debug msg to avoid nullpointerexception when running mediawiki maintenance/importDump script
handle null values
📝 WalkthroughWalkthroughThis PR updates package metadata (composer.json, extension.json), migrates property identifiers from PropertyId to NumericPropertyId throughout the codebase, introduces new hook handlers for parser initialization and content save operations, adds subobject support per property, and extends translation capabilities for EDTF and local media value types. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composer.json (1)
2-15:⚠️ Potential issue | 🟠 MajorPackage identity is inconsistent across distribution metadata.
The composer.json declares
baillyk/semantic-wikibaseon line 2, but the README.md installation instructions (lines 29-30) direct users to installprofessional-wiki/semantic-wikibase. Additionally, the source and issues URLs (lines 32-33) referencehttps://github.com/ProfessionalWiki/SemanticWikibase, while the homepage URL (line 15) points tohttps://github.com/baillyk/SemanticWikibase. This inconsistency will break downstream installations expecting the documented package name and creates confusion about the package's true identity.Either restore
professional-wiki/semantic-wikibaseas the package name and update the homepage URL, or update the README.md installation instructions to referencebaillyk/semantic-wikibaseand verify all repository URLs are consistently aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composer.json` around lines 2 - 15, The package identity is inconsistent between composer.json and README/metadata; update them to a single canonical name and matching repository URLs: either change the "name" field in composer.json from "baillyk/semantic-wikibase" to "professional-wiki/semantic-wikibase" and update "homepage", "source" and "support.issues" URLs to use https://github.com/ProfessionalWiki/SemanticWikibase, or update README.md installation instructions and any repository/source/issue URLs to reference "baillyk/semantic-wikibase" and https://github.com/baillyk/SemanticWikibase so the "name" field, homepage, source and issues URLs all match the chosen canonical package identity.
🧹 Nitpick comments (13)
src/Translation/UserDefinedProperties.php (1)
64-65: Use a lower-camel variable name for the helper parameter.Line 64 uses
$NumericPropertyId, which reads like a class/type name. A local variable name like$numericPropertyIdwould be clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/UserDefinedProperties.php` around lines 64 - 65, Rename the helper parameter in idFromWikibaseString from $NumericPropertyId to a lower-camel variable like $numericPropertyId and update any references inside the function body (the concatenation returning '___SWB_' . $NumericPropertyId) to use $numericPropertyId so the local variable follows standard lower-camel naming and avoids confusion with class/type names.src/Translation/StatementListTranslator.php (5)
12-16: Remove unused imports.
MWException,PropertyRegistry,StoreFactory, andStoreare imported but not used in this file.🧹 Remove unused imports
use Wikibase\DataModel\Statement\StatementList; -use MWException; -use SMW\PropertyRegistry; use SMW\Subobject; -use SMW\StoreFactory; -use SMW\Store;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` around lines 12 - 16, The file imports unused classes MWException, SMW\PropertyRegistry, SMW\StoreFactory, and SMW\Store; remove those unused use statements from StatementListTranslator.php and keep only the necessary imports (e.g., SMW\Subobject if it’s used). Edit the top-of-file use block to delete the lines referencing MWException, PropertyRegistry, StoreFactory, and Store so only actually referenced symbols remain.
40-40: Add type hint for$statementNrparameter.The parameter lacks a type hint. Given it's used as a statement counter, it should be typed as
int.✏️ Add type hint
- private function addStatement( SemanticEntity $semanticEntity, Statement $statement, $statementNr ): void { + private function addStatement( SemanticEntity $semanticEntity, Statement $statement, int $statementNr ): void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` at line 40, Add an explicit int type hint to the $statementNr parameter on the addStatement method declaration (method: addStatement(SemanticEntity $semanticEntity, Statement $statement, $statementNr)): change $statementNr to int $statementNr; also scan/adjust callers of addStatement to ensure they pass an int (or cast to int) so the signature remains consistent and no type errors occur at runtime.
69-83: Remove or track commented-out qualifier code.Large blocks of commented-out code reduce readability. If this is intended for future implementation, consider removing it and tracking in an issue. If it's work-in-progress, add a clear TODO with context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` around lines 69 - 83, Remove the large commented-out qualifier handling block or replace it with a clear tracked TODO and reference to an issue; specifically, either delete the commented block that iterates $qualifiers and uses $this->statementTranslator->snakToDataItem / $statement->getPropertyId() / $semanticEntity->addPropertyValue, or replace it with a single-line TODO comment that explains why qualifiers aren’t handled yet and includes an issue ID or ticket link so future work is tracked (e.g., "TODO: implement qualifier handling — tracked in ISSUE-123"). Ensure no dead commented code remains in StatementListTranslator.php.
31-34: Fix inconsistent indentation.Lines 31 and 34 use spaces while the rest of the file uses tabs for indentation.
✏️ Fix indentation
- $i = 1; + $i = 1; foreach ( $statements->getBestStatements()->getByRank( [ Statement::RANK_PREFERRED, Statement::RANK_NORMAL ] ) as $statement ) { $this->addStatement( $semanticEntity, $statement, $i ); - $i++; + $i++; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` around lines 31 - 34, The file uses tabs for indentation except for the two lines initializing and incrementing the counter in StatementListTranslator.php; change the leading spaces on the "$i = 1;" line and the "$i++;" line to tabs so they match the file's tab indentation style, preserving the current logic (the foreach over $statements->getBestStatements()->getByRank(...) that calls $this->addStatement($semanticEntity, $statement, $i)).
87-89: Method name violates PHP naming conventions.
NumericPropertyIdForStatementuses PascalCase. PHP methods should use camelCase (numericPropertyIdForStatement).✏️ Fix method naming
- private function NumericPropertyIdForStatement( Statement $statement ): string { + private function numericPropertyIdForStatement( Statement $statement ): string { return UserDefinedProperties::idFromWikibaseProperty( $statement->getPropertyId() ); }Also update the call sites at lines 50 and 56:
- $this->NumericPropertyIdForStatement( $statement ), + $this->numericPropertyIdForStatement( $statement ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementListTranslator.php` around lines 87 - 89, Rename the method NumericPropertyIdForStatement to camelCase numericPropertyIdForStatement in the StatementListTranslator class, keeping its signature (private function numericPropertyIdForStatement(Statement $statement): string) and behavior unchanged; then update every call site that invokes NumericPropertyIdForStatement (including the two internal calls in the same class) to use numericPropertyIdForStatement to comply with PHP method naming conventions.tests/Unit/Translation/ItemTranslatorTest.php (1)
138-147: Parameter naming violates PHP conventions.The parameter
$NumericPropertyIduses PascalCase, but PHP convention dictates camelCase for variables and parameters. This appears to be a copy-paste artifact from class/type naming.✏️ Suggested fix
- private function assertHasSubobjectWithPropertyValue( SemanticEntity $semanticEntity, string $NumericPropertyId, \SMWDataItem $expectedDataItem ) { + private function assertHasSubobjectWithPropertyValue( SemanticEntity $semanticEntity, string $propertyId, \SMWDataItem $expectedDataItem ) { /** * `@var` SMWDIContainer $container */ $container = $semanticEntity->getDataItemsForProperty( DIProperty::TYPE_SUBOBJECT )[0]; $this->assertEquals( [ $expectedDataItem ], - $container->getSemanticData()->getPropertyValues( new DIProperty( $NumericPropertyId ) ) + $container->getSemanticData()->getPropertyValues( new DIProperty( $propertyId ) ) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Translation/ItemTranslatorTest.php` around lines 138 - 147, Rename the parameter $NumericPropertyId to follow PHP camelCase (e.g. $numericPropertyId) in the method assertHasSubobjectWithPropertyValue and update all uses inside the method (the DIProperty construction and any other references) accordingly; also check and update any local call sites within this test class that pass the old-named argument so the symbol rename remains consistent.src/EntryPoints/HookHandlers.php (1)
55-77: Inconsistent indentation style.The new hook handlers use mixed indentation with tabs and spaces, which differs from the existing code style in the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EntryPoints/HookHandlers.php` around lines 55 - 77, The new hook handlers onPageSaveComplete and onMultiContentSave use mixed tabs and spaces; normalize their indentation to match the file's existing style (replace tabs with the project's preferred spaces and align all statements and braces consistently), ensuring lines inside the methods (wfDebug calls, variable assignments like $semanticData, $revision, $titleFactory, and calls to DIWikiPage::newFromTitle, StoreFactory::getStore, and SemanticWikibase::getGlobalInstance()->getSemanticDataUpdate()->run) follow the same indentation level as other methods in this class so the file has a consistent whitespace style.src/Translation/StatementTranslator.php (2)
89-93: Unused variables - dead code.Lines 89-93 create
$prop,$propertyDV, and$propertyDIthat are never used. Line 93 has a commented-out usage, suggesting this is leftover debug code.♻️ Proposed fix - remove dead code
$statementObj->addDataValue( DataValueFactory::getInstance()->newDataValueByText($statementProperty, $statementValue, false, $subject) ); - $prop = new DIProperty( 'semanticProperty:value'); - wfDebug('swb: prop: '.$prop); - $propertyDV = DataValueFactory::getInstance()->newPropertyValueByLabel("value"); - $propertyDI = $propertyDV->getDataItem(); - #$statementObj->addDataValue( DataValueFactory::getInstance()->newDataValueByItem($mainSnakDI, $propertyDI)); $result[] = $statementObj;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementTranslator.php` around lines 89 - 93, Remove the dead/debug code that creates unused variables $prop (DIProperty), $propertyDV (DataValueFactory::getInstance()->newPropertyValueByLabel("value")) and $propertyDI ($propertyDV->getDataItem()) and the wfDebug call in StatementTranslator::translate (the block shown around the commented $statementObj usage); either delete those three variable declarations and the wfDebug line and the commented-out statement, or if the commented line is intended to be active, replace it with the correct, intentional usage instead of leaving the unused variables — ensure no references to $prop, $propertyDV, or $propertyDI remain.
67-125: Missing type hints and magic strings.
- Line 67:
$statementNrparameter lacks a type hint.- The method uses many hardcoded strings like
'subObjectType','statement','qualifier','property_name', etc. Consider extracting these as class constants for maintainability.♻️ Suggested improvement
- public function statementToSubobjects( Statement $statement, DIWikiPage $subject , $statementNr): array { + public function statementToSubobjects( Statement $statement, DIWikiPage $subject, int $statementNr ): array {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/StatementTranslator.php` around lines 67 - 125, The method statementToSubobjects in class StatementTranslator is missing a type hint for the $statementNr parameter and uses many hardcoded string literals; update the signature to type-hint $statementNr as int and refactor all repeated magic strings (e.g. 'subObjectType', 'statement', 'qualifier', 'property_name', 'property_wb', 'property_smw', 'value', etc.) into descriptive class constants (e.g. const SUBOBJECT_TYPE, CONST_SUBOBJ_STATEMENT, CONST_SUBOBJ_QUALIFIER, CONST_PROP_NAME, CONST_PROP_WB_PREFIX, CONST_PROP_SMW_PREFIX, CONST_VALUE) and replace all inline literals in statementToSubobjects (and any related methods like snakToDataItem/valueDIToString usages) with those constants so callers like DataValueFactory::getInstance()->newDataValueByText(...) use the constants instead of strings.src/Translation/DataValueTranslator.php (3)
119-128: Remove unused variables and debug code.
- Line 122:
$tvArris assigned but never used.- Lines 123-129: The parser and humanizer are created only for debug logging. This adds overhead and should be cleaned up before merging.
♻️ Proposed cleanup
private function translateEDTF( String $value ): ?\SMWDITime { wfDebug( 'swb: translate edtf' ); - $tvb = new TimeValueBuilder( EdtfFactory::newParser() ); - $tvArr = $tvb->edtfToTimeValues( $value ); $parser = \EDTF\EdtfFactory::newParser(); $parsingResult = $parser->parse($value); - wfDebug($parsingResult->isValid()); // true + if ( !$parsingResult->isValid() ) { + wfDebug( 'swb: invalid EDTF value: ' . $value ); + return null; + } $edtfValue = $parsingResult->getEdtfValue(); // \EDTF\EdtfValue - $humanizer = \EDTF\EdtfFactory::newHumanizerForLanguage( 'en' ); - wfDebug($humanizer->humanize($edtfValue)); // string - wfDebug(get_class($edtfValue)); // string $result = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/DataValueTranslator.php` around lines 119 - 128, In translateEDTF remove the unused $tvArr assignment and all debugging-only parser/humanizer plumbing and wfDebug calls: delete the $tvArr assignment from TimeValueBuilder::edtfToTimeValues and remove creation/usage of $parser, $parsingResult, $edtfValue, $humanizer and their wfDebug humanize/isValid calls; retain the essential TimeValueBuilder usage that produces the actual return \SMWDITime value and any real logic needed to build/return the result in translateEDTF.
38-40: Use strict comparison for null check.Line 38 uses loose comparison
$value != null. In PHP, this can produce unexpected results with falsy values. Use strict comparison!== nullinstead.♻️ Proposed fix
- if( $value != null) { + if( $value !== null) { wfDebug( 'swb: translate: '.get_class( $value ).' ptype: '.$propertyType ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/DataValueTranslator.php` around lines 38 - 40, Replace the loose null check in the conditional that logs debug info—change the comparison of $value in the DataValueTranslator context from "$value != null" to a strict "$value !== null" so falsy but non-null values (e.g., 0, '', false) won’t be treated as null; update the conditional containing wfDebug('swb: translate: '.get_class( $value ).' ptype: '.$propertyType ) accordingly (preserve the wfDebug call and message).
22-29: Unused imports.
LoggerFactory(line 25) andTimeValueBuilder(line 23) appear to be imported but not effectively used (TimeValueBuilder result is unused, LoggerFactory is never called).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Translation/DataValueTranslator.php` around lines 22 - 29, Remove the unused imports to clean up the file: delete the use statements for MediaWiki\Logger\LoggerFactory and Wikibase\EDTF\Services\TimeValueBuilder (they are not referenced; TimeValueBuilder result is unused and LoggerFactory is never called). If you intended to use them, instead add the appropriate calls where needed (e.g., call LoggerFactory to obtain a logger or use TimeValueBuilder to build a time value) and reference those symbols in the methods that need logging or EDTF construction; otherwise remove the two use lines entirely.
🤖 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/EntryPoints/HookHandlers.php`:
- Around line 31-35: The comment in the onParserFirstCallInit method contains a
typo: change "aktivates" to "activates" in the inline comment that references
getInstance() calling initProperties() which activates the existing hook
onSmwInitProperties(); update the comment text near
PropertyRegistry::getInstance() inside the onParserFirstCallInit function
accordingly.
- Around line 63-77: The onMultiContentSave handler creates a redundant $subject
variable and calls DIWikiPage::newFromTitle($title) twice; remove the unused
$subject assignment and replace the second DIWikiPage::newFromTitle call by
reusing a single DIWikiPage instance (e.g. create $page =
DIWikiPage::newFromTitle($title) once), then pass $page into
StoreFactory::getStore()->getSemanticData(...) and any later uses (e.g. when
building $semanticData) so the duplicate call and dead $subject are eliminated.
- Around line 55-61: The call to onSmwUpdateDataBefore from onPageSaveComplete
is missing the self:: static reference and therefore attempts to call a
non-existent function; update the call inside onPageSaveComplete to use
self::onSmwUpdateDataBefore(null, $semanticData) so it correctly invokes the
static method defined in the same class (ensure you update the invocation in the
onPageSaveComplete method).
In `@src/SemanticDataUpdate.php`:
- Around line 66-73: Remove the temporary wfDebug calls and the duplicate
property lookup: create a NumericPropertyId once from $title->getText(), call
$this->propertyLookup->getPropertyForId(...) once and store the result in a
local variable, then pass that variable into
$this->newPropertyTranslator($title)->translateProperty(...). Remove the four
wfDebug(...) lines and replace the repeated new NumericPropertyId /
getPropertyForId calls with the two local variables to avoid redundant work.
In `@src/SemanticWikibase.php`:
- Around line 65-77: Store the result of $this->getAllProperties() in a local
variable and iterate that instead of calling getAllProperties() twice; remove
the transient wfDebug debug calls around the registration (the wfDebug that
json_encodes properties and the two wfDebug calls printing property id/type),
keeping only necessary logging if any; update the loop that calls
$propertyRegistry->registerProperty(...) and the foreach over
$property->getAliases() to use the saved variable (e.g., $allProperties) to
avoid duplicate retrieval and noisy logs.
In `@src/SMW/SemanticEntity.php`:
- Around line 68-74: The add(self $entity) method currently only merges
$entity->dataItemsPerProperty, which drops subobjects; update add() to also
merge subobjects by iterating $entity->subobjectsPerProperty and calling
$this->addSubobject($NumericPropertyId, $subobject) (or otherwise merging
entries into $this->subobjectsPerProperty) for each subobject so
functionalMerge() preserves subobjects added via addSubobject(); reference the
add(), addSubobject(), dataItemsPerProperty and subobjectsPerProperty symbols
when making the change.
In `@src/Translation/DataValueTranslator.php`:
- Around line 119-141: The method translateEDTF currently declares it returns
\SMWDITime but can return null (see the early return in translateEDTF when an
EDTF interval has no start/end); change the signature of translateEDTF to return
a nullable type (?\\SMWDITime) and update any callers (notably translate()) to
handle a null result (e.g. check for null and bail or propagate an appropriate
error) so no TypeError occurs at runtime.
- Around line 161-167: The else branch in DataValueTranslator where $edtfValue
falls through to a hardcoded SMWDITime( CM_GREGORIAN, 1970 ) must be changed to
surface unexpected types instead of silently returning epoch; update the block
in the function handling $edtfValue (the branch that currently creates SMWDITime
for unknown types) to either throw a clear exception (e.g.
InvalidArgumentException) or emit a warning via the project's logger including
the actual type (get_class($edtfValue) or gettype) and return a safe null/empty
value; ensure you reference the symbols $edtfValue, ExtDate, ExtDateTime,
Interval and SMWDITime when implementing the change so callers can handle the
new error path.
In `@src/Translation/StatementTranslator.php`:
- Around line 129-141: valueDIToString currently calls get_class($di) and
assumes $di is non-null and that DIWikiPage::getTitle() returns a Title; both
can be null when snakToDataItem returns null. Update valueDIToString to first
check for null and return an empty string or safe placeholder if $di is null;
then when $di is an instance of DIWikiPage, check that $di->getTitle() is
non-null before calling getText() and fall back to a safe string if it is null;
keep the existing SMWDITime branch but guard uses of methods with null checks.
This ensures callers like snakToDataItem -> valueDIToString won't trigger
TypeError/NPE.
- Around line 46-56: Remove the redundant instanceof check in snakToDataItem
(the parameter is already typed PropertyValueSnak) and simplify the flow to
directly handle container vs simple values; additionally fix qualifier handling
by ensuring the container check uses the qualifier snak rather than the parent
statement—when calling container-related logic (e.g.,
containerValueTranslator->supportsStatement(...) and
statementToDataItem/statementToQualifiersDataItemList) pass or construct a
Statement that wraps the qualifier snak (or add/use a supportsSnak method on
containerValueTranslator) so supportsStatement inspects the actual qualifier
snak being translated instead of the main statement; keep references to
snakToDataItem, containerValueTranslator->supportsStatement,
containerValueTranslator->statementToDataItem,
statementToQualifiersDataItemList, and snakWithSimpleDataValueToDataItem to
locate and update the code.
---
Outside diff comments:
In `@composer.json`:
- Around line 2-15: The package identity is inconsistent between composer.json
and README/metadata; update them to a single canonical name and matching
repository URLs: either change the "name" field in composer.json from
"baillyk/semantic-wikibase" to "professional-wiki/semantic-wikibase" and update
"homepage", "source" and "support.issues" URLs to use
https://github.com/ProfessionalWiki/SemanticWikibase, or update README.md
installation instructions and any repository/source/issue URLs to reference
"baillyk/semantic-wikibase" and https://github.com/baillyk/SemanticWikibase so
the "name" field, homepage, source and issues URLs all match the chosen
canonical package identity.
---
Nitpick comments:
In `@src/EntryPoints/HookHandlers.php`:
- Around line 55-77: The new hook handlers onPageSaveComplete and
onMultiContentSave use mixed tabs and spaces; normalize their indentation to
match the file's existing style (replace tabs with the project's preferred
spaces and align all statements and braces consistently), ensuring lines inside
the methods (wfDebug calls, variable assignments like $semanticData, $revision,
$titleFactory, and calls to DIWikiPage::newFromTitle, StoreFactory::getStore,
and SemanticWikibase::getGlobalInstance()->getSemanticDataUpdate()->run) follow
the same indentation level as other methods in this class so the file has a
consistent whitespace style.
In `@src/Translation/DataValueTranslator.php`:
- Around line 119-128: In translateEDTF remove the unused $tvArr assignment and
all debugging-only parser/humanizer plumbing and wfDebug calls: delete the
$tvArr assignment from TimeValueBuilder::edtfToTimeValues and remove
creation/usage of $parser, $parsingResult, $edtfValue, $humanizer and their
wfDebug humanize/isValid calls; retain the essential TimeValueBuilder usage that
produces the actual return \SMWDITime value and any real logic needed to
build/return the result in translateEDTF.
- Around line 38-40: Replace the loose null check in the conditional that logs
debug info—change the comparison of $value in the DataValueTranslator context
from "$value != null" to a strict "$value !== null" so falsy but non-null values
(e.g., 0, '', false) won’t be treated as null; update the conditional containing
wfDebug('swb: translate: '.get_class( $value ).' ptype: '.$propertyType )
accordingly (preserve the wfDebug call and message).
- Around line 22-29: Remove the unused imports to clean up the file: delete the
use statements for MediaWiki\Logger\LoggerFactory and
Wikibase\EDTF\Services\TimeValueBuilder (they are not referenced;
TimeValueBuilder result is unused and LoggerFactory is never called). If you
intended to use them, instead add the appropriate calls where needed (e.g., call
LoggerFactory to obtain a logger or use TimeValueBuilder to build a time value)
and reference those symbols in the methods that need logging or EDTF
construction; otherwise remove the two use lines entirely.
In `@src/Translation/StatementListTranslator.php`:
- Around line 12-16: The file imports unused classes MWException,
SMW\PropertyRegistry, SMW\StoreFactory, and SMW\Store; remove those unused use
statements from StatementListTranslator.php and keep only the necessary imports
(e.g., SMW\Subobject if it’s used). Edit the top-of-file use block to delete the
lines referencing MWException, PropertyRegistry, StoreFactory, and Store so only
actually referenced symbols remain.
- Line 40: Add an explicit int type hint to the $statementNr parameter on the
addStatement method declaration (method: addStatement(SemanticEntity
$semanticEntity, Statement $statement, $statementNr)): change $statementNr to
int $statementNr; also scan/adjust callers of addStatement to ensure they pass
an int (or cast to int) so the signature remains consistent and no type errors
occur at runtime.
- Around line 69-83: Remove the large commented-out qualifier handling block or
replace it with a clear tracked TODO and reference to an issue; specifically,
either delete the commented block that iterates $qualifiers and uses
$this->statementTranslator->snakToDataItem / $statement->getPropertyId() /
$semanticEntity->addPropertyValue, or replace it with a single-line TODO comment
that explains why qualifiers aren’t handled yet and includes an issue ID or
ticket link so future work is tracked (e.g., "TODO: implement qualifier handling
— tracked in ISSUE-123"). Ensure no dead commented code remains in
StatementListTranslator.php.
- Around line 31-34: The file uses tabs for indentation except for the two lines
initializing and incrementing the counter in StatementListTranslator.php; change
the leading spaces on the "$i = 1;" line and the "$i++;" line to tabs so they
match the file's tab indentation style, preserving the current logic (the
foreach over $statements->getBestStatements()->getByRank(...) that calls
$this->addStatement($semanticEntity, $statement, $i)).
- Around line 87-89: Rename the method NumericPropertyIdForStatement to
camelCase numericPropertyIdForStatement in the StatementListTranslator class,
keeping its signature (private function numericPropertyIdForStatement(Statement
$statement): string) and behavior unchanged; then update every call site that
invokes NumericPropertyIdForStatement (including the two internal calls in the
same class) to use numericPropertyIdForStatement to comply with PHP method
naming conventions.
In `@src/Translation/StatementTranslator.php`:
- Around line 89-93: Remove the dead/debug code that creates unused variables
$prop (DIProperty), $propertyDV
(DataValueFactory::getInstance()->newPropertyValueByLabel("value")) and
$propertyDI ($propertyDV->getDataItem()) and the wfDebug call in
StatementTranslator::translate (the block shown around the commented
$statementObj usage); either delete those three variable declarations and the
wfDebug line and the commented-out statement, or if the commented line is
intended to be active, replace it with the correct, intentional usage instead of
leaving the unused variables — ensure no references to $prop, $propertyDV, or
$propertyDI remain.
- Around line 67-125: The method statementToSubobjects in class
StatementTranslator is missing a type hint for the $statementNr parameter and
uses many hardcoded string literals; update the signature to type-hint
$statementNr as int and refactor all repeated magic strings (e.g.
'subObjectType', 'statement', 'qualifier', 'property_name', 'property_wb',
'property_smw', 'value', etc.) into descriptive class constants (e.g. const
SUBOBJECT_TYPE, CONST_SUBOBJ_STATEMENT, CONST_SUBOBJ_QUALIFIER, CONST_PROP_NAME,
CONST_PROP_WB_PREFIX, CONST_PROP_SMW_PREFIX, CONST_VALUE) and replace all inline
literals in statementToSubobjects (and any related methods like
snakToDataItem/valueDIToString usages) with those constants so callers like
DataValueFactory::getInstance()->newDataValueByText(...) use the constants
instead of strings.
In `@src/Translation/UserDefinedProperties.php`:
- Around line 64-65: Rename the helper parameter in idFromWikibaseString from
$NumericPropertyId to a lower-camel variable like $numericPropertyId and update
any references inside the function body (the concatenation returning '___SWB_' .
$NumericPropertyId) to use $numericPropertyId so the local variable follows
standard lower-camel naming and avoids confusion with class/type names.
In `@tests/Unit/Translation/ItemTranslatorTest.php`:
- Around line 138-147: Rename the parameter $NumericPropertyId to follow PHP
camelCase (e.g. $numericPropertyId) in the method
assertHasSubobjectWithPropertyValue and update all uses inside the method (the
DIProperty construction and any other references) accordingly; also check and
update any local call sites within this test class that pass the old-named
argument so the symbol rename remains consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
composer.jsonextension.jsonsrc/EntryPoints/HookHandlers.phpsrc/SMW/SemanticEntity.phpsrc/SemanticDataUpdate.phpsrc/SemanticWikibase.phpsrc/Translation/DataValueTranslator.phpsrc/Translation/ItemTranslator.phpsrc/Translation/PropertyTranslator.phpsrc/Translation/PropertyTypeTranslator.phpsrc/Translation/StatementListTranslator.phpsrc/Translation/StatementTranslator.phpsrc/Translation/UserDefinedProperties.phptests/TestFactory.phptests/Unit/Translation/ContainerValueTranslatorTest.phptests/Unit/Translation/DataValueTranslatorTest.phptests/Unit/Translation/ItemTranslatorTest.phptests/Unit/Translation/PropertyTranslatorTest.phptests/Unit/Translation/StatementTranslatorTest.php
Comment on lines
+31
to
+35
| public static function onParserFirstCallInit( Parser $parser ) { | ||
| # getInstance() will call initProperties() which aktivates existing hook onSmwInitProperties() | ||
| wfDebug( __METHOD__ . "SWB: onParserFirstCallInit" ); | ||
| PropertyRegistry::getInstance(); | ||
| } |
There was a problem hiding this comment.
Typo in comment.
Line 32: "aktivates" should be "activates".
📝 Proposed fix
public static function onParserFirstCallInit( Parser $parser ) {
- # getInstance() will call initProperties() which aktivates existing hook onSmwInitProperties()
+ # getInstance() will call initProperties() which activates existing hook onSmwInitProperties()
wfDebug( __METHOD__ . "SWB: onParserFirstCallInit" );
PropertyRegistry::getInstance();
}🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 31-31: Avoid unused parameters such as '$parser'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EntryPoints/HookHandlers.php` around lines 31 - 35, The comment in the
onParserFirstCallInit method contains a typo: change "aktivates" to "activates"
in the inline comment that references getInstance() calling initProperties()
which activates the existing hook onSmwInitProperties(); update the comment text
near PropertyRegistry::getInstance() inside the onParserFirstCallInit function
accordingly.
Comment on lines
+55
to
+61
| public static function onPageSaveComplete( WikiPage $wikiPage, MediaWiki\User\UserIdentity $user, string $summary, int $flags, MediaWiki\Revision\RevisionRecord $revisionRecord, MediaWiki\Storage\EditResult $editResult ) { | ||
| wfDebug( __METHOD__ . "SWB: onPageSaveComplete" ); | ||
| // Access the semantic data of the page via the Semantic MediaWiki API | ||
| $semanticData = \SMW\MediaWiki\Hooks\ParserHooks::getSemanticDataForPage($wikiPage); | ||
|
|
||
| onSmwUpdateDataBefore(null, $semanticData); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the actual file and the specific lines
cat -n src/EntryPoints/HookHandlers.php | sed -n '50,70p'Repository: ProfessionalWiki/SemanticWikibase
Length of output: 1374
🏁 Script executed:
# Check if onSmwUpdateDataBefore is defined in the class as static
rg -A 5 'function onSmwUpdateDataBefore' src/EntryPoints/HookHandlers.phpRepository: ProfessionalWiki/SemanticWikibase
Length of output: 326
🏁 Script executed:
# Search for all function definitions in HookHandlers.php to understand structure
rg -n 'public\s+static\s+function' src/EntryPoints/HookHandlers.phpRepository: ProfessionalWiki/SemanticWikibase
Length of output: 927
🏁 Script executed:
# Search for ParserHooks class definition
find . -name "*.php" -type f | xargs rg -l "class ParserHooks"Repository: ProfessionalWiki/SemanticWikibase
Length of output: 61
🏁 Script executed:
# Search for getSemanticDataForPage method
rg -n "getSemanticDataForPage" --type phpRepository: ProfessionalWiki/SemanticWikibase
Length of output: 205
🏁 Script executed:
# Check if there are any imports or references to SMW\MediaWiki\Hooks\ParserHooks
rg "ParserHooks" src/EntryPoints/HookHandlers.php -A 2 -B 2Repository: ProfessionalWiki/SemanticWikibase
Length of output: 354
Fix critical fatal error: missing self:: prefix on static method call.
Line 60 calls onSmwUpdateDataBefore(null, $semanticData) without the self:: prefix. Since onSmwUpdateDataBefore is a static method defined in the same class (line 48), this will cause a fatal "Call to undefined function" error at runtime.
Proposed fix
public static function onPageSaveComplete( WikiPage $wikiPage, MediaWiki\User\UserIdentity $user, string $summary, int $flags, MediaWiki\Revision\RevisionRecord $revisionRecord, MediaWiki\Storage\EditResult $editResult ) {
wfDebug( __METHOD__ . "SWB: onPageSaveComplete" );
// Access the semantic data of the page via the Semantic MediaWiki API
$semanticData = \SMW\MediaWiki\Hooks\ParserHooks::getSemanticDataForPage($wikiPage);
- onSmwUpdateDataBefore(null, $semanticData);
+ self::onSmwUpdateDataBefore(null, $semanticData);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static function onPageSaveComplete( WikiPage $wikiPage, MediaWiki\User\UserIdentity $user, string $summary, int $flags, MediaWiki\Revision\RevisionRecord $revisionRecord, MediaWiki\Storage\EditResult $editResult ) { | |
| wfDebug( __METHOD__ . "SWB: onPageSaveComplete" ); | |
| // Access the semantic data of the page via the Semantic MediaWiki API | |
| $semanticData = \SMW\MediaWiki\Hooks\ParserHooks::getSemanticDataForPage($wikiPage); | |
| onSmwUpdateDataBefore(null, $semanticData); | |
| } | |
| public static function onPageSaveComplete( WikiPage $wikiPage, MediaWiki\User\UserIdentity $user, string $summary, int $flags, MediaWiki\Revision\RevisionRecord $revisionRecord, MediaWiki\Storage\EditResult $editResult ) { | |
| wfDebug( __METHOD__ . "SWB: onPageSaveComplete" ); | |
| // Access the semantic data of the page via the Semantic MediaWiki API | |
| $semanticData = \SMW\MediaWiki\Hooks\ParserHooks::getSemanticDataForPage($wikiPage); | |
| self::onSmwUpdateDataBefore(null, $semanticData); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 55-55: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
[warning] 55-55: Avoid unused parameters such as '$summary'. (undefined)
(UnusedFormalParameter)
[warning] 55-55: Avoid unused parameters such as '$flags'. (undefined)
(UnusedFormalParameter)
[warning] 55-55: Avoid unused parameters such as '$revisionRecord'. (undefined)
(UnusedFormalParameter)
[warning] 55-55: Avoid unused parameters such as '$editResult'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EntryPoints/HookHandlers.php` around lines 55 - 61, The call to
onSmwUpdateDataBefore from onPageSaveComplete is missing the self:: static
reference and therefore attempts to call a non-existent function; update the
call inside onPageSaveComplete to use self::onSmwUpdateDataBefore(null,
$semanticData) so it correctly invokes the static method defined in the same
class (ensure you update the invocation in the onPageSaveComplete method).
Comment on lines
+63
to
+77
| public static function onMultiContentSave( RenderedRevision $renderedRevision, UserIdentity $user, CommentStoreComment $summary, $flags, Status $hookStatus ) { | ||
| wfDebug( __METHOD__ . "SWB: onMultiContentSave" ); | ||
| $revision = $renderedRevision->getRevision(); | ||
|
|
||
| $titleFactory = MediaWikiServices::getInstance()->getTitleFactory(); | ||
| $title = $titleFactory->newFromLinkTarget($revision->getPageAsLinkTarget()); | ||
| wfDebug( __METHOD__ . "SWB: onMultiContentSave...title:".$title ); | ||
| $subject = DIWikiPage::newFromTitle( $title ); | ||
| #$new_content = $revision->getContent(SlotRecord::MAIN, RevisionRecord::RAW)->getNativeData(); | ||
| $semanticData = StoreFactory::getStore()->getSemanticData( DIWikiPage::newFromTitle( $title ) ); | ||
| wfDebug( __METHOD__ . "SWB: onMultiContentSave: ".json_encode($semanticData) ); | ||
| SemanticWikibase::getGlobalInstance()->getSemanticDataUpdate()->run( $semanticData ); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Redundant variable and duplicated call.
- Line 70 assigns
$subjectwhich is never used (as flagged by static analysis). DIWikiPage::newFromTitle($title)is called twice: at line 70 (unused result) and again at line 72.
🔧 Proposed fix to remove dead code
public static function onMultiContentSave( RenderedRevision $renderedRevision, UserIdentity $user, CommentStoreComment $summary, $flags, Status $hookStatus ) {
wfDebug( __METHOD__ . "SWB: onMultiContentSave" );
$revision = $renderedRevision->getRevision();
$titleFactory = MediaWikiServices::getInstance()->getTitleFactory();
$title = $titleFactory->newFromLinkTarget($revision->getPageAsLinkTarget());
wfDebug( __METHOD__ . "SWB: onMultiContentSave...title:".$title );
- $subject = DIWikiPage::newFromTitle( $title );
- #$new_content = $revision->getContent(SlotRecord::MAIN, RevisionRecord::RAW)->getNativeData();
- $semanticData = StoreFactory::getStore()->getSemanticData( DIWikiPage::newFromTitle( $title ) );
+ $semanticData = StoreFactory::getStore()->getSemanticData( DIWikiPage::newFromTitle( $title ) );
wfDebug( __METHOD__ . "SWB: onMultiContentSave: ".json_encode($semanticData) );
SemanticWikibase::getGlobalInstance()->getSemanticDataUpdate()->run( $semanticData );
return true;
} 🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 63-63: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
[warning] 63-63: Avoid unused parameters such as '$summary'. (undefined)
(UnusedFormalParameter)
[warning] 63-63: Avoid unused parameters such as '$flags'. (undefined)
(UnusedFormalParameter)
[warning] 63-63: Avoid unused parameters such as '$hookStatus'. (undefined)
(UnusedFormalParameter)
[warning] 70-70: Avoid unused local variables such as '$subject'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EntryPoints/HookHandlers.php` around lines 63 - 77, The
onMultiContentSave handler creates a redundant $subject variable and calls
DIWikiPage::newFromTitle($title) twice; remove the unused $subject assignment
and replace the second DIWikiPage::newFromTitle call by reusing a single
DIWikiPage instance (e.g. create $page = DIWikiPage::newFromTitle($title) once),
then pass $page into StoreFactory::getStore()->getSemanticData(...) and any
later uses (e.g. when building $semanticData) so the duplicate call and dead
$subject are eliminated.
Comment on lines
+66
to
73
| wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($title)); | ||
| wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($title->getText())); | ||
| wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode(new NumericPropertyId( $title->getText() ))); | ||
| wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($this->propertyLookup->getPropertyForId( new NumericPropertyId( $title->getText() ) ))); | ||
|
|
||
| return $this->newPropertyTranslator( $title )->translateProperty( | ||
| $this->propertyLookup->getPropertyForId( new PropertyId( $title->getText() ) ) | ||
| $this->propertyLookup->getPropertyForId( new NumericPropertyId( $title->getText() ) ) | ||
| ); |
There was a problem hiding this comment.
Remove temporary debug logging and avoid duplicate property lookups.
Lines 66-70 add verbose debug output and perform an extra getPropertyForId call before Line 72 repeats the same lookup. This adds overhead on every property-page update path.
Suggested cleanup
private function getSemanticEntityForPropertyTitle( Title $title ): SemanticEntity {
- wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($title));
- wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($title->getText()));
- wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode(new NumericPropertyId( $title->getText() )));
- wfDebug(__METHOD__. "swb: getSemanticEntity:".json_encode($this->propertyLookup->getPropertyForId( new NumericPropertyId( $title->getText() ) )));
-
- return $this->newPropertyTranslator( $title )->translateProperty(
- $this->propertyLookup->getPropertyForId( new NumericPropertyId( $title->getText() ) )
- );
+ $propertyId = new NumericPropertyId( $title->getText() );
+ $property = $this->propertyLookup->getPropertyForId( $propertyId );
+
+ return $this->newPropertyTranslator( $title )->translateProperty( $property );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SemanticDataUpdate.php` around lines 66 - 73, Remove the temporary
wfDebug calls and the duplicate property lookup: create a NumericPropertyId once
from $title->getText(), call $this->propertyLookup->getPropertyForId(...) once
and store the result in a local variable, then pass that variable into
$this->newPropertyTranslator($title)->translateProperty(...). Remove the four
wfDebug(...) lines and replace the repeated new NumericPropertyId /
getPropertyForId calls with the two local variables to avoid redundant work.
Comment on lines
+65
to
77
| wfDebug("SWB: register properties ".json_encode($this->getAllProperties())); | ||
| foreach ( $this->getAllProperties() as $property ) { | ||
| $propertyRegistry->registerProperty( | ||
| $property->getId(), | ||
| $property->getType(), | ||
| $property->getLabel(), | ||
| true, | ||
| false | ||
| true, #is_visible | ||
| true, #is_annotable | ||
| false #is_declarative | ||
| ); | ||
|
|
||
| wfDebug("SWB: register property ".$property->getId()); | ||
| wfDebug("SWB: register property ".$property->getType()); | ||
| foreach ( $property->getAliases() as $alias ) { |
There was a problem hiding this comment.
Avoid duplicate property retrieval and remove transient debug logs.
Line 65 computes getAllProperties() for logging, and Line 66 computes it again for iteration. Also, Lines 65/75/76 add noisy debug output in a core registration path.
Suggested cleanup
public function registerProperties( PropertyRegistry $propertyRegistry ) {
- wfDebug("SWB: register properties ".json_encode($this->getAllProperties()));
- foreach ( $this->getAllProperties() as $property ) {
+ $properties = $this->getAllProperties();
+ foreach ( $properties as $property ) {
$propertyRegistry->registerProperty(
$property->getId(),
$property->getType(),
$property->getLabel(),
true, `#is_visible`
true, `#is_annotable`
false `#is_declarative`
);
- wfDebug("SWB: register property ".$property->getId());
- wfDebug("SWB: register property ".$property->getType());
foreach ( $property->getAliases() as $alias ) {
$propertyRegistry->registerPropertyAlias( $property->getId(), $alias );
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SemanticWikibase.php` around lines 65 - 77, Store the result of
$this->getAllProperties() in a local variable and iterate that instead of
calling getAllProperties() twice; remove the transient wfDebug debug calls
around the registration (the wfDebug that json_encodes properties and the two
wfDebug calls printing property id/type), keeping only necessary logging if any;
update the loop that calls $propertyRegistry->registerProperty(...) and the
foreach over $property->getAliases() to use the saved variable (e.g.,
$allProperties) to avoid duplicate retrieval and noisy logs.
Comment on lines
68
to
74
| public function add( self $entity ): void { | ||
| foreach ( $entity->dataItemsPerProperty as $propertyId => $dataItems ) { | ||
| foreach ( $entity->dataItemsPerProperty as $NumericPropertyId => $dataItems ) { | ||
| foreach ( $dataItems as $dataItem ) { | ||
| $this->addPropertyValue( $propertyId, $dataItem ); | ||
| $this->addPropertyValue( $NumericPropertyId, $dataItem ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Subobjects are dropped during entity merge.
add() (Line 68+) merges only dataItemsPerProperty. Any subobjects added via addSubobject() are not merged, so functionalMerge() silently loses them.
Suggested fix
public function add( self $entity ): void {
foreach ( $entity->dataItemsPerProperty as $NumericPropertyId => $dataItems ) {
foreach ( $dataItems as $dataItem ) {
$this->addPropertyValue( $NumericPropertyId, $dataItem );
}
}
+
+ foreach ( $entity->subObjectsPerProperty as $NumericPropertyId => $subobjects ) {
+ foreach ( $subobjects as $subobject ) {
+ $this->addSubobject( $NumericPropertyId, $subobject );
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SMW/SemanticEntity.php` around lines 68 - 74, The add(self $entity)
method currently only merges $entity->dataItemsPerProperty, which drops
subobjects; update add() to also merge subobjects by iterating
$entity->subobjectsPerProperty and calling
$this->addSubobject($NumericPropertyId, $subobject) (or otherwise merging
entries into $this->subobjectsPerProperty) for each subobject so
functionalMerge() preserves subobjects added via addSubobject(); reference the
add(), addSubobject(), dataItemsPerProperty and subobjectsPerProperty symbols
when making the change.
Comment on lines
+119
to
+141
| private function translateEDTF( String $value ): \SMWDITime { | ||
| wfDebug( 'swb: translate edtf' ); | ||
| $tvb = new TimeValueBuilder( EdtfFactory::newParser() ); | ||
| $tvArr = $tvb->edtfToTimeValues( $value ); | ||
| $parser = \EDTF\EdtfFactory::newParser(); | ||
| $parsingResult = $parser->parse($value); | ||
| wfDebug($parsingResult->isValid()); // true | ||
| $edtfValue = $parsingResult->getEdtfValue(); // \EDTF\EdtfValue | ||
| $humanizer = \EDTF\EdtfFactory::newHumanizerForLanguage( 'en' ); | ||
| wfDebug($humanizer->humanize($edtfValue)); // string | ||
| wfDebug(get_class($edtfValue)); // string | ||
| $result = null; | ||
|
|
||
| if ( $edtfValue instanceof Interval ) { | ||
| if($edtfValue->hasStartDate()){ | ||
| $edtfValue = $edtfValue->getStartDate(); | ||
| } else if ($edtfValue->hasEndDate()){ | ||
| $edtfValue = $edtfValue->getEndDate(); | ||
| } else { | ||
| wfDebug('ERROR: unable to translate empty edtf interval to smw date'); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Type error: Method returns null but declares return type \SMWDITime.
Line 139 returns null when an EDTF interval has neither start nor end date, but the method signature declares \SMWDITime as the return type. This will cause a TypeError at runtime.
🐛 Proposed fix - change return type to nullable
- private function translateEDTF( String $value ): \SMWDITime {
+ private function translateEDTF( String $value ): ?\SMWDITime {Additionally, the caller in translate() should handle the null return to avoid propagating issues.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private function translateEDTF( String $value ): \SMWDITime { | |
| wfDebug( 'swb: translate edtf' ); | |
| $tvb = new TimeValueBuilder( EdtfFactory::newParser() ); | |
| $tvArr = $tvb->edtfToTimeValues( $value ); | |
| $parser = \EDTF\EdtfFactory::newParser(); | |
| $parsingResult = $parser->parse($value); | |
| wfDebug($parsingResult->isValid()); // true | |
| $edtfValue = $parsingResult->getEdtfValue(); // \EDTF\EdtfValue | |
| $humanizer = \EDTF\EdtfFactory::newHumanizerForLanguage( 'en' ); | |
| wfDebug($humanizer->humanize($edtfValue)); // string | |
| wfDebug(get_class($edtfValue)); // string | |
| $result = null; | |
| if ( $edtfValue instanceof Interval ) { | |
| if($edtfValue->hasStartDate()){ | |
| $edtfValue = $edtfValue->getStartDate(); | |
| } else if ($edtfValue->hasEndDate()){ | |
| $edtfValue = $edtfValue->getEndDate(); | |
| } else { | |
| wfDebug('ERROR: unable to translate empty edtf interval to smw date'); | |
| return null; | |
| } | |
| } | |
| private function translateEDTF( String $value ): ?\SMWDITime { | |
| wfDebug( 'swb: translate edtf' ); | |
| $tvb = new TimeValueBuilder( EdtfFactory::newParser() ); | |
| $tvArr = $tvb->edtfToTimeValues( $value ); | |
| $parser = \EDTF\EdtfFactory::newParser(); | |
| $parsingResult = $parser->parse($value); | |
| wfDebug($parsingResult->isValid()); // true | |
| $edtfValue = $parsingResult->getEdtfValue(); // \EDTF\EdtfValue | |
| $humanizer = \EDTF\EdtfFactory::newHumanizerForLanguage( 'en' ); | |
| wfDebug($humanizer->humanize($edtfValue)); // string | |
| wfDebug(get_class($edtfValue)); // string | |
| $result = null; | |
| if ( $edtfValue instanceof Interval ) { | |
| if($edtfValue->hasStartDate()){ | |
| $edtfValue = $edtfValue->getStartDate(); | |
| } else if ($edtfValue->hasEndDate()){ | |
| $edtfValue = $edtfValue->getEndDate(); | |
| } else { | |
| wfDebug('ERROR: unable to translate empty edtf interval to smw date'); | |
| return null; | |
| } | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 122-122: Avoid unused local variables such as '$tvArr'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/DataValueTranslator.php` around lines 119 - 141, The method
translateEDTF currently declares it returns \SMWDITime but can return null (see
the early return in translateEDTF when an EDTF interval has no start/end);
change the signature of translateEDTF to return a nullable type (?\\SMWDITime)
and update any callers (notably translate()) to handle a null result (e.g. check
for null and bail or propagate an appropriate error) so no TypeError occurs at
runtime.
Comment on lines
+161
to
+167
| } else { | ||
| $result = new \SMWDITime( | ||
| SMWDITime::CM_GREGORIAN, | ||
| 1970 | ||
| ); | ||
|
|
||
| } |
There was a problem hiding this comment.
Silent fallback to epoch date (1970) can cause data integrity issues.
When $edtfValue is neither ExtDate, ExtDateTime, nor Interval, the code silently falls back to January 1, 1970. This masks parsing failures and corrupts data without any indication to the user.
Consider logging a warning or throwing an exception for unexpected types.
🔧 Proposed fix
} else {
- $result = new \SMWDITime(
- SMWDITime::CM_GREGORIAN,
- 1970
- );
-
+ wfDebug( 'swb: WARNING - Unhandled EDTF type: ' . get_class($edtfValue) . ', falling back to epoch' );
+ $result = new \SMWDITime(
+ SMWDITime::CM_GREGORIAN,
+ 1970
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else { | |
| $result = new \SMWDITime( | |
| SMWDITime::CM_GREGORIAN, | |
| 1970 | |
| ); | |
| } | |
| } else { | |
| wfDebug( 'swb: WARNING - Unhandled EDTF type: ' . get_class($edtfValue) . ', falling back to epoch' ); | |
| $result = new \SMWDITime( | |
| SMWDITime::CM_GREGORIAN, | |
| 1970 | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/DataValueTranslator.php` around lines 161 - 167, The else
branch in DataValueTranslator where $edtfValue falls through to a hardcoded
SMWDITime( CM_GREGORIAN, 1970 ) must be changed to surface unexpected types
instead of silently returning epoch; update the block in the function handling
$edtfValue (the branch that currently creates SMWDITime for unknown types) to
either throw a clear exception (e.g. InvalidArgumentException) or emit a warning
via the project's logger including the actual type (get_class($edtfValue) or
gettype) and return a safe null/empty value; ensure you reference the symbols
$edtfValue, ExtDate, ExtDateTime, Interval and SMWDITime when implementing the
change so callers can handle the new error path.
Comment on lines
+46
to
56
| public function snakToDataItem( PropertyValueSnak $snak, Statement $statement, DIWikiPage $subject ): ?SMWDataItem { | ||
| if ( !( $snak instanceof PropertyValueSnak ) ) { | ||
| return null; | ||
| } | ||
|
|
||
| if ( $this->containerValueTranslator->supportsStatement( $statement ) ) { | ||
| return $this->containerValueTranslator->statementToDataItem( $statement, $subject ); | ||
| } | ||
|
|
||
| return $this->snakWithSimpleDataValueToDataItem( $mainSnak ); | ||
| return $this->snakWithSimpleDataValueToDataItem( $snak ); | ||
| } |
There was a problem hiding this comment.
Redundant instanceof check and logic issue with qualifiers.
-
Line 47: The type hint already declares
$snakasPropertyValueSnak, so theinstanceof PropertyValueSnakcheck will always be true (or PHP would have thrown a TypeError before reaching this code). -
Line 51: When processing qualifiers via
statementToQualifiersDataItemList, this method passes the qualifier snak but checkssupportsStatement($statement)which inspects the main snak of the statement, not the qualifier snak being translated. This may cause incorrect behavior for qualifier translation.
🔧 Proposed fix
- public function snakToDataItem( PropertyValueSnak $snak, Statement $statement, DIWikiPage $subject ): ?SMWDataItem {
- if ( !( $snak instanceof PropertyValueSnak ) ) {
+ public function snakToDataItem( $snak, Statement $statement, DIWikiPage $subject ): ?SMWDataItem {
+ if ( !( $snak instanceof PropertyValueSnak ) ) {
return null;
}
- if ( $this->containerValueTranslator->supportsStatement( $statement ) ) {
+ // Only use container translator for main snak, not qualifiers
+ if ( $snak === $statement->getMainSnak() && $this->containerValueTranslator->supportsStatement( $statement ) ) {
return $this->containerValueTranslator->statementToDataItem( $statement, $subject );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function snakToDataItem( PropertyValueSnak $snak, Statement $statement, DIWikiPage $subject ): ?SMWDataItem { | |
| if ( !( $snak instanceof PropertyValueSnak ) ) { | |
| return null; | |
| } | |
| if ( $this->containerValueTranslator->supportsStatement( $statement ) ) { | |
| return $this->containerValueTranslator->statementToDataItem( $statement, $subject ); | |
| } | |
| return $this->snakWithSimpleDataValueToDataItem( $mainSnak ); | |
| return $this->snakWithSimpleDataValueToDataItem( $snak ); | |
| } | |
| public function snakToDataItem( $snak, Statement $statement, DIWikiPage $subject ): ?SMWDataItem { | |
| if ( !( $snak instanceof PropertyValueSnak ) ) { | |
| return null; | |
| } | |
| // Only use container translator for main snak, not qualifiers | |
| if ( $snak === $statement->getMainSnak() && $this->containerValueTranslator->supportsStatement( $statement ) ) { | |
| return $this->containerValueTranslator->statementToDataItem( $statement, $subject ); | |
| } | |
| return $this->snakWithSimpleDataValueToDataItem( $snak ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/StatementTranslator.php` around lines 46 - 56, Remove the
redundant instanceof check in snakToDataItem (the parameter is already typed
PropertyValueSnak) and simplify the flow to directly handle container vs simple
values; additionally fix qualifier handling by ensuring the container check uses
the qualifier snak rather than the parent statement—when calling
container-related logic (e.g., containerValueTranslator->supportsStatement(...)
and statementToDataItem/statementToQualifiersDataItemList) pass or construct a
Statement that wraps the qualifier snak (or add/use a supportsSnak method on
containerValueTranslator) so supportsStatement inspects the actual qualifier
snak being translated instead of the main statement; keep references to
snakToDataItem, containerValueTranslator->supportsStatement,
containerValueTranslator->statementToDataItem,
statementToQualifiersDataItemList, and snakWithSimpleDataValueToDataItem to
locate and update the code.
Comment on lines
+129
to
+141
| private static function valueDIToString($di) : string { | ||
| wfDebug('smw: translate DI: '.get_class($di)); | ||
| if( $di instanceof DIWikiPage ) { | ||
| return $di->getTitle()->getText().""; | ||
| } else if ( $di instanceof SMWDITime){ | ||
| wfDebug('time:'.$di->getSerialization()); | ||
| return $di->asDateTime()->format( 'Y-m-d H:i:s' ); | ||
| #return "01.01.1700"; | ||
| } | ||
| wfdebug('default'); | ||
| return $di.""; | ||
|
|
||
| } |
There was a problem hiding this comment.
Missing null check causes potential NPE.
snakToDataItem can return null (line 48), but valueDIToString doesn't handle null input. This will cause a TypeError when calling get_class($di) on null (lines 77, 109 pass the result of snakToDataItem directly).
Additionally, DIWikiPage::getTitle() at line 132 can return null, which would cause an NPE when calling getText().
🐛 Proposed fix with null safety
private static function valueDIToString($di) : string {
+ if ( $di === null ) {
+ return '';
+ }
wfDebug('smw: translate DI: '.get_class($di));
if( $di instanceof DIWikiPage ) {
- return $di->getTitle()->getText()."";
+ $title = $di->getTitle();
+ return $title !== null ? $title->getText() : $di->getDBkey();
} else if ( $di instanceof SMWDITime){
wfDebug('time:'.$di->getSerialization());
return $di->asDateTime()->format( 'Y-m-d H:i:s' );
- `#return` "01.01.1700";
}
wfdebug('default');
return $di."";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static function valueDIToString($di) : string { | |
| wfDebug('smw: translate DI: '.get_class($di)); | |
| if( $di instanceof DIWikiPage ) { | |
| return $di->getTitle()->getText().""; | |
| } else if ( $di instanceof SMWDITime){ | |
| wfDebug('time:'.$di->getSerialization()); | |
| return $di->asDateTime()->format( 'Y-m-d H:i:s' ); | |
| #return "01.01.1700"; | |
| } | |
| wfdebug('default'); | |
| return $di.""; | |
| } | |
| private static function valueDIToString($di) : string { | |
| if ( $di === null ) { | |
| return ''; | |
| } | |
| wfDebug('smw: translate DI: '.get_class($di)); | |
| if( $di instanceof DIWikiPage ) { | |
| $title = $di->getTitle(); | |
| return $title !== null ? $title->getText() : $di->getDBkey(); | |
| } else if ( $di instanceof SMWDITime){ | |
| wfDebug('time:'.$di->getSerialization()); | |
| return $di->asDateTime()->format( 'Y-m-d H:i:s' ); | |
| } | |
| wfdebug('default'); | |
| return $di.""; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 129-141: Avoid unused private methods such as 'valueDIToString'. (undefined)
(UnusedPrivateMethod)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Translation/StatementTranslator.php` around lines 129 - 141,
valueDIToString currently calls get_class($di) and assumes $di is non-null and
that DIWikiPage::getTitle() returns a Title; both can be null when
snakToDataItem returns null. Update valueDIToString to first check for null and
return an empty string or safe placeholder if $di is null; then when $di is an
instance of DIWikiPage, check that $di->getTitle() is non-null before calling
getText() and fall back to a safe string if it is null; keep the existing
SMWDITime branch but guard uses of methods with null checks. This ensures
callers like snakToDataItem -> valueDIToString won't trigger TypeError/NPE.
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
1 participant
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.
updated to ensure compatibility with mediawiki 1.43
tested environment:
MW: 1.43
SMW: ^6.0.0
SRF: ~5.2
WikibaseLocalMedia: ~2
WikibaseEdtf: ^3.0.0
Maps: ~12.0
WIkibase: REL1_43
the following Wikibase Property datatypes where succesfully tested for functionality:
Summary by CodeRabbit
New Features
Chores