Fix deprecated optional being before required params #5941
Conversation
WalkthroughThe pull request introduces changes to several method signatures across multiple classes, primarily focusing on the removal of default values for specific parameters. This modification enforces that certain parameters must now be explicitly provided when calling these methods, impacting how existing code interacts with these methods. The changes affect the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5941 +/- ##
=========================================
Coverage 56.74% 56.74%
Complexity 19277 19277
=========================================
Files 983 983
Lines 68862 68862
=========================================
Hits 39075 39075
Misses 29787 29787 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/SQLStore/EntityStore/SemanticDataLookup.php (1)
129-129: Inconsistent parameter type check
The method signature now mandates$dataItemto be passed (albeit nullable), yet it throws aRuntimeExceptionunless$dataItemis an instance ofDIWikiPage. This is contradictory to allowingnullor otherDataItemtypes. Consider adjusting the signature or the type check to maintain a coherent contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Elastic/QueryEngine/ConditionBuilder.php(1 hunks)src/Query/DescriptionBuilders/DescriptionBuilder.php(1 hunks)src/SPARQLStore/QueryEngine/QueryResultFactory.php(1 hunks)src/SQLStore/EntityStore/CachingSemanticDataLookup.php(4 hunks)src/SQLStore/EntityStore/EntityLookup.php(1 hunks)src/SQLStore/EntityStore/SemanticDataLookup.php(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/SPARQLStore/QueryEngine/QueryResultFactory.php
[warning] 62-62: src/SPARQLStore/QueryEngine/QueryResultFactory.php#L62
Added line #L62 was not covered by tests
src/Elastic/QueryEngine/ConditionBuilder.php
[warning] 373-373: src/Elastic/QueryEngine/ConditionBuilder.php#L373
Added line #L373 was not covered by tests
🔇 Additional comments (8)
src/SPARQLStore/QueryEngine/QueryResultFactory.php (1)
62-62: Add test coverage for new required parameterLine 62 is newly introduced without test coverage, as indicated by the static analysis tool. Consider adding a focused test to ensure stability and prevent regressions, especially for scenarios where
$repositoryResultmight benull.Would you like me to provide a sample test case or open a new issue to help track this coverage need?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 62-62: src/SPARQLStore/QueryEngine/QueryResultFactory.php#L62
Added line #L62 was not covered by testssrc/Query/DescriptionBuilders/DescriptionBuilder.php (1)
118-118: Confirm correct usage in calling codeBy removing the default value for
$property, this parameter must always be provided. Ensure that all internal and external calls are updated to avoid runtime errors.src/SQLStore/EntityStore/CachingSemanticDataLookup.php (4)
209-209: Verify caller compliance with new required$dataItemSince
$dataItemno longer has a default value, please confirm that all invocations ofprefetchDataFromTableprovide a non-omitted$dataItem.
254-254: Verify all calls tofetchSemanticDataFromTableWith the default value removed, ensure that all places passing
$dataItemdo so explicitly. This helps avoid unintended null references.
268-268: Check for null usage ingetSemanticDataThis parameter’s default value has been removed. Confirm that dependent code now supplies
$dataItemcorrectly in every scenario.
288-288: Ensure$dataItemis always provided tofetchFromCacheLine 288 removes the default value for
$dataItem. Confirm all call sites supply a non-omitted$dataItemto prevent runtime errors.src/Elastic/QueryEngine/ConditionBuilder.php (1)
373-373: Enhance test coverage forfindHierarchyMembersThis new required parameter
$dataItemis not tested, according to static analysis. Consider adding a dedicated test to verify correct handling (including null scenarios).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 373-373: src/Elastic/QueryEngine/ConditionBuilder.php#L373
Added line #L373 was not covered by testssrc/SQLStore/EntityStore/EntityLookup.php (1)
240-240: Potential BC break due to removal of default param value
Changing the$subjectparameter from having a default value ofnullto requiring an explicit value can break consumers that previously omitted this argument. Verify and update all call sites to ensure they pass this parameter if needed.To confirm, run the following script to locate all invocations of
getPropertyValues:
|
I don't think this is the right approach. The deprecation warning occurs because in PHP, optional parameters (those with a default value) should not precede required parameters. This is because it can create ambiguity when calling the function. To fix this properly, you should reorder the parameters so that the required ones come first. Codecov commented on some of the changes not covered by tests. We should check this. I would feel much better if we had tests for these cases. |
I did consider that but that would be a breaking change (although acceptable as we're going to a 5.0 release). I found that doing this (the current pull) fixes the deprecation notice and keeps the current behaviour making it a noop. I'm happy to change the params around if that's what you want but I think that would be more work as you'd have to go digging into the code to find where it's used and fix it there as well. @gesinn-it-gea thoughts? |
|
@JeroenDeDauw what's your opinion? |
Fixes #5939
Summary by CodeRabbit
Summary by CodeRabbit
EntityLookupinterface to require explicit$subjectparameter.ConditionBuilderclass to require explicit$dataItemparameter.DescriptionBuilderclass to require explicit$propertyparameter.QueryResultFactoryclass to require explicit$repositoryResultparameter.CachingSemanticDataLookupclass to require explicit$dataItemparameter.SemanticDataLookupclass to require explicit$dataItemparameter.