Conversation
WalkthroughThis PR refactors observer registration in HtmlMin from SplObjectStorage's attach() method to direct array-style assignment, adds a null-safety check in protectTagHelper to prevent null dereference, and changes attribute removal in HtmlMinDomObserverOptimizeAttributes from null assignment to the removeAttribute() method. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Summary of ChangesHello @tfedor, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on resolving PHP 8.5 deprecation warnings and stabilizing the codebase by fixing several related test failures. The changes involve updating how observers are attached, adding null checks for DOM elements, and refining the process of removing HTML attributes, ensuring better compatibility with newer PHP versions and improving overall code reliability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses deprecation warnings for PHP 8.5 and fixes several tests. The changes are well-targeted and correct.
The update from SplObjectStorage::attach() to array syntax is the correct way to handle the PHP 8.5 deprecation. The added null check for $parentNode correctly prevents a potential fatal error and likely fixes some of the failing tests mentioned. The switch to removeAttribute() is also a good improvement for code clarity.
I have one minor suggestion to use the nullsafe operator to make one of the new checks more concise. Overall, this is a great set of improvements.
|
|
||
| $parentNode = $element->parentNode(); | ||
| if ($parentNode->nodeValue !== null) { | ||
| if (!is_null($parentNode) && $parentNode->nodeValue !== null) { |
There was a problem hiding this comment.
While this check is correct and fixes a potential null access error, you can make it more concise by using the nullsafe operator (?->), which was introduced in PHP 8.0. It simplifies checks for null before property or method access.
if ($parentNode?->nodeValue !== null) {There was a problem hiding this comment.
Intentional to not break PHP 7 compatibility
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/voku/helper/HtmlMin.php (1)
1716-1720: Good null-safety improvement.The null check for
$parentNodebefore accessingnodeValueprevents potential null dereference when elements don't have a parent node.Consider using direct comparison for consistency with PHP idioms:
- if (!is_null($parentNode) && $parentNode->nodeValue !== null) { + if ($parentNode !== null && $parentNode->nodeValue !== null) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/voku/helper/HtmlMin.php(2 hunks)src/voku/helper/HtmlMinDomObserverOptimizeAttributes.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (2)
src/voku/helper/HtmlMinDomObserverOptimizeAttributes.php (1)
143-146: LGTM! Proper use ofremoveAttribute()for attribute removal.Using
removeAttribute()instead of setting the property to null is the correct DOM API for attribute removal and addresses PHP 8.5 deprecation warnings related to dynamic property access patterns.src/voku/helper/HtmlMin.php (1)
350-353: Use array-style assignment instead ofattach()for SplObjectStorage compatibility with PHP 8.5.In PHP 8.5.0,
SplObjectStorage::attach(),contains(), anddetach()methods are deprecated. The array-style syntax ($storage[$object] = $value,isset($storage[$object]), andunset($storage[$object])) should be used instead. The change on line 352 correctly implements this by using$this->domLoopObservers[$observer] = $observer;rather than the deprecatedattach()method.
|
The author of this library (Lars) seems to have disappeared. Not responding here, and no recent activity on his social channels. Hopefully Lars is okay. As for this library, we may need to switch to a forked copy to keep it maintained. |
2 participants
Fixes #
Fix PHP 8.5 deprecation warnings #109, and about 9 tests that were failing for me. There are still 5 tests that fail, but I think those are coming from the dependency.
This change is
Summary by CodeRabbit
Bug Fixes
Performance Improvements
✏️ Tip: You can customize this high-level summary in your review settings.