Conversation
WalkthroughThis PR updates CI to test PHP 8.3–8.5, modernizes GitHub Actions versions, raises the package PHP requirement to >=8.0, adjusts dependencies, refactors PHPUnit config, and changes AntiXSS numeric-casting logic and test reflection handling to avoid casting errors on PHP 8.5. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
🔇 Additional comments (1)
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 @zoispag, 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 significantly enhances the project's compatibility with modern PHP versions, specifically up to PHP 8.5, by updating core dependencies and modernizing the continuous integration pipeline. It also includes important configuration updates for PHPUnit and minor code improvements for numeric string handling, ensuring better maintainability and future-proofing. Highlights
Ignored Files
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 does a great job of updating dependencies and CI workflows for PHP 8.5 compatibility. The modernization of phpunit.xml is also a welcome improvement. However, there are a few critical issues in the test files that need to be addressed. Specifically, changes to reflection helpers will break the test suite, and a valid test case has been removed, reducing coverage. Additionally, a backup file has been committed to the repository, which should be removed to avoid clutter.
| $reflection = new \ReflectionObject($object); | ||
| $method = $reflection->getMethod($methodName); | ||
| $method->setAccessible(true); | ||
|
|
||
| return $method->invokeArgs($object, $parameters); |
There was a problem hiding this comment.
The call to $method->setAccessible(true); has been removed from this reflection helper. This is a critical change that will break tests. This line is necessary to allow the testing of private and protected methods, such as _remove_evil_attributes. Without it, invokeArgs will throw a ReflectionException for non-public methods. Please restore this line.
$reflection = new \ReflectionObject($object);
$method = $reflection->getMethod($methodName);
$method->setAccessible(true);
return $method->invokeArgs($object, $parameters);| $reflection = new \ReflectionObject($object); | ||
| $property = $reflection->getProperty($propertyName); | ||
| $property->setAccessible(true); | ||
|
|
||
| return $property->getValue($object); |
There was a problem hiding this comment.
Similar to the invokeMethod helper, removing $property->setAccessible(true); here is a critical error. This line is required to access private or protected properties during testing. Its removal will cause tests that rely on inspecting these properties (like _xss_found) to fail. Please re-add this call.
$reflection = new \ReflectionObject($object);
$property = $reflection->getProperty($propertyName);
$property->setAccessible(true);
return $property->getValue($object);
phpunit.xml.bak
Outdated
| <phpunit bootstrap="tests/bootstrap.php"> | ||
| <testsuite name="Anti-XSS Test Suite"> | ||
| <directory>tests</directory> | ||
| </testsuite> | ||
| <filter> | ||
| <whitelist processUncoveredFilesFromWhitelist="true"> | ||
| <directory suffix=".php">./src/</directory> | ||
| </whitelist> | ||
| </filter> | ||
| <logging> | ||
| <log type="coverage-clover" target="build/logs/clover.xml"/> | ||
| </logging> | ||
| </phpunit> |
There was a problem hiding this comment.
This backup file should not be committed to the repository. Version control systems like Git are designed to keep track of file history, making backup files redundant and adding unnecessary clutter to the project. If you need to revert to the old configuration, you can do so through Git's history.
src/voku/helper/AntiXSS.php
Outdated
| (string) $strInt === $str | ||
| || | ||
| (string) $strFloat === $str | ||
| (\is_numeric($str) && ((string) (int) $str === $str || (string) (float) $str === $str)) |
There was a problem hiding this comment.
The logic here is sound, but the extra parentheses around the \is_numeric(...) condition are redundant and can be removed for better readability. The || operator has lower precedence than &&, so the expression will be evaluated correctly without them.
\is_numeric($str) && ((string) (int) $str === $str || (string) (float) $str === $str)| '<a href="https://mbd.baidu.com/newspage/data/landingsuper?context=%7B%22nid%22%3A%22news_15446515888862039806%22%7D&n_type=0&p_from=1" target="_blank">Valid Link</a>' => '<a href="https://mbd.baidu.com/newspage/data/landingsuper?context=%7B%22nid%22%3A%22news_15446515888862039806%22%7D&n_type=0&p_from=1" target="_blank">Valid Link</a>', | ||
| '' => '', | ||
| ' ' => ' ', | ||
| null => '', |
There was a problem hiding this comment.
This test case for null input appears to have been removed. Since xss_clean(null) is expected to return an empty string, this is a valid and useful test case that ensures correct behavior for this input. Removing it reduces test coverage. Please consider re-adding it.
![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
🤖 Fix all issues with AI agents
In @phpunit.xml:
- Around line 1-14: The phpunit.xml uses the PHPUnit 9.3-style
<coverage>/<include> blocks which are incompatible with PHPUnit 6/7 used on PHP
7.0-7.2; update the repo to supply a config compatible with both versions by
either restoring the older phpunit.xml.bak format (using <filter><whitelist>
elements) alongside the current phpunit.xml, or produce a phpunit.xml.dist that
uses the legacy <filter><whitelist> syntax while keeping the new file for
PHPUnit 9+, and ensure composer.json's phpunit/phpunit constraint
(~6.0||~7.0||~9.0) is honored by CI so jobs on PHP 7.0-7.2 will load the legacy
config.
🧹 Nitpick comments (3)
phpunit.xml.bak (1)
1-13: Backup file retained for reference.This backup preserves the legacy PHPUnit configuration with deprecated
<filter>/<whitelist>and<logging>/<log>syntax. Consider removing this file after confirming the newphpunit.xmlworks correctly across all PHP versions, or add it to.gitignoreto prevent accidental commits.phpunit.xml (1)
14-14: Remove empty<logging/>element.The empty
<logging/>element serves no purpose and can be removed.Suggested fix
</testsuite> - <logging/> </phpunit>composer.json (1)
24-32: Using a fork with a development branch is necessary but document the rationale.The official
voku/portable-utf8package (version 6.0.13) only supports PHP >=7.0.0 and lacks PHP 8.4+ compatibility. Thekyosenergy/portable-utf8fork is actively maintained and explicitly designed for PHP 8.4 compatibility, making this workaround justified.However, add documentation explaining this dependency choice (in a comment or README) so future maintainers understand the reason. Additionally, monitor the official package for updates and plan a migration back once PHP 8.4+ support is released.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci.ymlcomposer.jsonphpunit.xmlphpunit.xml.baksrc/voku/helper/AntiXSS.phptests/XssTest.php
💤 Files with no reviewable changes (1)
- tests/XssTest.php
⏰ 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 (5)
src/voku/helper/AntiXSS.php (1)
656-667: LGTM! Theis_numeric()guard prevents PHP 8.5 deprecation warnings.The conditional now short-circuits before casting non-numeric strings, avoiding the "implicit conversion from float-string to int" deprecation introduced in PHP 8.5. The logic correctly:
- Skips processing for pure numeric strings (integers and floats)
- Avoids unnecessary casts on non-numeric input
- Preserves the original behavior for XSS detection
.github/workflows/ci.yml (4)
49-58: LGTM! Proper migration from deprecatedset-outputtoGITHUB_OUTPUT.The change from
::set-outputto>> $GITHUB_OUTPUTfollows GitHub's recommended approach after the deprecation notice.
94-101: Verify action versions are available.actions/upload-artifact@v6is available and runs on Node.js 24, requiring a minimum Actions Runner version of 2.327.1. The versions are valid.
38-39:actions/checkout@v6exists and is stable. Version 6 has been released with stable versions including v6.0.0 and v6.0.1. The reference@v6is valid and appropriate for use in the workflow.Likely an incorrect or invalid review comment.
28-30: No action needed.shivammathur/setup-php@v2explicitly supports PHP 8.5 (documented range: PHP 5.3–8.6), so the workflow matrix entries for PHP 8.3, 8.4, and 8.5 are correct. Nocontinue-on-errorflag is required.Likely an incorrect or invalid review comment.
| <?xml version="1.0"?> | ||
| <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" bootstrap="tests/bootstrap.php" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"> | ||
| <coverage processUncoveredFiles="true"> | ||
| <include> | ||
| <directory suffix=".php">./src/</directory> | ||
| </include> | ||
| <report> | ||
| <clover outputFile="build/logs/clover.xml"/> | ||
| </report> | ||
| </coverage> | ||
| <testsuite name="Anti-XSS Test Suite"> | ||
| <directory>tests</directory> | ||
| </testsuite> | ||
| <logging/> |
There was a problem hiding this comment.
PHPUnit configuration may not be compatible with PHP 7.0-7.2.
This configuration uses PHPUnit 9.3+ schema with <coverage> and <include> blocks. However, composer.json allows phpunit ~6.0 || ~7.0 || ~9.0, and the CI matrix tests PHP 7.0-7.2 which require PHPUnit 6.x or 7.x (PHPUnit 9 requires PHP 7.3+).
PHPUnit 6/7 uses the old <filter>/<whitelist> syntax from phpunit.xml.bak. This means:
- PHP 7.0-7.2 CI jobs may fail or produce warnings about unrecognized configuration elements
Consider keeping both config files or using PHPUnit's ability to auto-detect configuration format.
#!/bin/bash
# Verify PHPUnit version constraints in composer.json
cat composer.json | jq '.["require-dev"]["phpunit/phpunit"]'🤖 Prompt for AI Agents
In @phpunit.xml around lines 1 - 14, The phpunit.xml uses the PHPUnit 9.3-style
<coverage>/<include> blocks which are incompatible with PHPUnit 6/7 used on PHP
7.0-7.2; update the repo to supply a config compatible with both versions by
either restoring the older phpunit.xml.bak format (using <filter><whitelist>
elements) alongside the current phpunit.xml, or produce a phpunit.xml.dist that
uses the legacy <filter><whitelist> syntax while keeping the new file for
PHPUnit 9+, and ensure composer.json's phpunit/phpunit constraint
(~6.0||~7.0||~9.0) is honored by CI so jobs on PHP 7.0-7.2 will load the legacy
config.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @phpunit.xml:
- Around line 1-13: The phpunit.xml uses PHPUnit 9.3+ coverage syntax
(<coverage>, <include>, <report>) which is incompatible with PHPUnit 7.x
installed on PHP 7.0/7.1 in CI; create a legacy configuration (e.g.,
phpunit-legacy.xml) that replaces the modern <coverage> block with the legacy
<filter>/<whitelist> structure and same <directory> entries, then update the CI
workflow to select phpunit-legacy.xml when running jobs on PHP 7.0 and 7.1 (or
alternatively split configs and constrain which config each PHP job uses).
🧹 Nitpick comments (1)
phpunit.xml (1)
14-14: Consider removing the empty logging element.The empty
<logging/>element serves no functional purpose and can be removed to simplify the configuration.♻️ Proposed cleanup
</testsuite> - <logging/> - </phpunit>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
phpunit.xml
⏰ 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
- Add is_numeric() guard to prevent ErrorException on invalid numeric strings in PHP 8.5 when casting to int - Conditionally call setAccessible() only on PHP < 8.1 to avoid deprecation warnings while maintaining PHP 8.0 compatibility - Remove redundant parentheses in condition
|
If only bots could actually merge fixes 🥲 |
2 participants
This pull request updates dependencies, modernizes CI workflows, improves compatibility, and refines configuration files for better maintainability and PHP version support. The most important changes are grouped below:
CI/CD and Dependency Updates:
actions/checkout,shivammathur/setup-php,actions/cache,codecov/codecov-action, andactions/upload-artifactto their latest major versions. Also modernized the way composer cache directory is set in the workflow. [1] [2] [3] [4]composer.jsonto use a development branch (dev-php84) ofvoku/portable-utf8for improved PHP 8.4+ compatibility, and added a VCS repository entry for it.Configuration and Coverage:
phpunit.xmlto use the latest schema and configuration style, moving coverage and logging settings to the new format, and added a backup configuration filephpunit.xml.bakwith the previous settings. [1] [2]Code and Test Improvements:
_domethod ofAntiXSS.phpby usingis_numeric()for more robust checks.nullinput intestNoXss()and removed unnecessary usage ofsetAccessible(true)in reflection helper methods. [1] [2] [3]This change is
Closes #166
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.