phpinfo improvements - #99
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Brotli module information output now reports extension, library, dictionary, and APCu serializer status. The information test captures this output and validates expected values across Brotli and APCu configurations. ChangesBrotli information reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/info.phpt`:
- Around line 72-98: Update the MIME-check section in tests/info.phpt to execute
validation before printing success: scan all entries in $lines for the “Built-in
output compression exclusions => ” row rather than using fixed indices, parse
its MIME values, report invalid values or a missing row as failures, and print
“MIMEs OK” only when the row exists and every MIME matches the expected format.
- Line 39: Update the assertion around the dictionary-status check in
tests/info.phpt to derive the expected enabled or disabled value from the
configured BROTLI_DICTIONARY_SUPPORT setting, then require an exact match
against $lines[3] instead of accepting either state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aca54509-8114-48f3-995a-c8b7f2c7879d
📒 Files selected for processing (2)
brotli.ctests/info.phpt
| echo "MIMEs OK\n"; | ||
| /* | ||
| $search = 'Built-in output compression exclusions => '; | ||
| $mimeIndex = null; | ||
| // could be 5 or 6 depending on apcu | ||
| foreach([5, 6] as $index) { | ||
| if (isset($lines[$index]) && substr($lines[$index], 0, 42) == $search) { | ||
| $mimeIndex = $index; | ||
| } | ||
| } | ||
| if ($mimeIndex) { | ||
| $types = explode(', ', substr($lines[$mimeIndex], 42)); | ||
| $invalidTypes = []; | ||
| foreach($types as $type) { | ||
| if (!preg_match('/^([a-z]+)\/(([a-z0-9\-\.]+)|\*)$/', $type)) { | ||
| $invalidTypes[] = $type; | ||
| } | ||
| } | ||
| if ($invalidTypes) { | ||
| echo "Fail: " . implode(", ", $invalidTypes) . "\n"; | ||
| } else { | ||
| echo "MIMEs OK\n"; | ||
| } | ||
| } else { | ||
| echo "Fail\n"; | ||
| } | ||
| */ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore MIME validation before reporting success.
Line 72 always prints "MIMEs OK". The commented validation no longer detects a missing or invalid MIME row. Restore the validation and scan $lines for the MIME row instead of relying on a fixed index, because the APCu rows vary by build state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/info.phpt` around lines 72 - 98, Update the MIME-check section in
tests/info.phpt to execute validation before printing success: scan all entries
in $lines for the “Built-in output compression exclusions => ” row rather than
using fixed indices, parse its MIME values, report invalid values or a missing
row as failures, and print “MIMEs OK” only when the row exists and every MIME
matches the expected format.
e990d20 to
80884c9
Compare
- "Brotli support enabled" doesn't provide any information, so just remove it - show if brotli is bundled or not - show if brotli is the active serializer for apcu or if apcu isn't loaded - rename serializer ABI to interface - if apcu support wasn't built, show that it could have been built
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| echo ($lines[4] == 'APCu serializer support => not built') ? "Apcu OK\n" : "Fail: not built\n"; | ||
| } | ||
|
|
||
| echo "MIMEs OK\n"; |
There was a problem hiding this comment.
Isn't the validation logic left commented out?
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests