-
-
Notifications
You must be signed in to change notification settings - Fork 33
Fix deprecation warning on PHP 8.5 #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds PHP 8.5 to CI test matrix and changes AccessHelper::keyExists to treat null keys as empty strings when checking array keys. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AccessHelper
participant PHP_Array
Caller->>AccessHelper: keyExists(key)
alt key is null
AccessHelper->>AccessHelper: normalize key -> ($key ?? '')
Note right of AccessHelper `#E8F3FF`: converts null to empty string
AccessHelper->>PHP_Array: array_key_exists('', collection)
else key not null
AccessHelper->>PHP_Array: array_key_exists(key, collection)
end
PHP_Array-->>AccessHelper: boolean
AccessHelper-->>Caller: boolean
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
============================================
+ Coverage 89.36% 89.84% +0.47%
- Complexity 202 223 +21
============================================
Files 11 11
Lines 395 443 +48
============================================
+ Hits 353 398 +45
- Misses 42 45 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/AccessHelper.php (1)
40-40: Fix is correct, but consider applying null guards consistently for code clarity.The change at line 40 correctly addresses the PHP 8.5 deprecation of
array_key_exists()with null keys. The verification confirms an inconsistency: whilekeyExistsnow guards against null with$key ?? '', the methodsgetValue(lines 73, 78),setValue(line 127), andunsetValue(line 141) access array keys directly without null guards.This inconsistency is not functionally problematic—PHP implicitly converts null to empty string during direct array access, so the code will work correctly. However, for consistency and code clarity, these methods could also use the explicit null guard pattern when accessing arrays directly:
- Line 73:
$return = $collection[$key ?? ''];- Line 78:
$return = $collection[$key ?? ''];- Line 127:
return $collection[$key ?? ''] = $value;- Line 141:
unset($collection[$key ?? '']);This makes the null-handling behavior explicit and uniform across the class.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/Test.yml(1 hunks)src/AccessHelper.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/AccessHelper.php (1)
src/JSONPath.php (1)
key(224-227)
🔇 Additional comments (1)
.github/workflows/Test.yml (1)
26-27: PHP 8.5 added to test matrix successfully.Adding PHP 8.5 to the test matrix is appropriate given the deprecation fix. The PR objectives confirm that tests pass on 8.5 after the changes.
Note that
continue-on-erroron line 27 is still tied only to PHP 8.4, not 8.5. This means failures on 8.5 will cause the workflow to fail. If PHP 8.5 is still experimental and failures should be tolerated, consider updating the condition to:continue-on-error: ${{ matrix.php == '8.4' || matrix.php == '8.5' }}Otherwise, the current configuration is correct if you want 8.5 to be treated as a stable, required version.
|
Thanks :) |
🔀 Pull Request
What does this PR do?
Fix deprecation warning when running unit tests on PHP 8.5.
This was the only warning that popped up.
Null coalescing operator
??was added in PHP 7.0, so there are no backwards compatibility issues.Test Plan
I enabled workflow to run tests on PHP 8.5.
Output before
Output after
Summary by CodeRabbit
Chores
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.