Upgrade nikic/php-parser to v5+#63
Conversation
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @BrianHenryIE, 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!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. The primary goal of this PR, authored by BrianHenryIE, is to upgrade the nikic/php-parser dependency to support version 5.x, while maintaining compatibility with recent 4.x versions. The author notes that some commits are tangential to the main upgrade (related to CPU core detection and test coverage scripts) and could potentially be cherry-picked into separate PRs. The core changes involve updating the dependency constraint, adjusting how the parser is instantiated to use the newest supported version, and adding compatibility checks for methods (getLine, getParts) that have changed or been deprecated in php-parser v5. Additionally, the PR includes changes to improve CPU core detection, particularly for macOS, and adds a convenient composer script for running test coverage.
Highlights
- Dependency Upgrade: Updates the
nikic/php-parserrequirement incomposer.jsonto allow versions^4.18 || ^5. - PHP Parser Instantiation: Changes the parser creation logic in
PhpCodeParserto useParserFactory::createForNewestSupportedVersion()for better compatibility acrossphp-parserversions. - PHP Parser API Compatibility: Adds checks (
method_exists) for deprecated or changed methods likegetLine(nowgetStartLine) andgetPartsinBasePHPElementandUtilsto ensure compatibility with bothphp-parserv4 and v5 APIs. - Test Coverage Script: Adds a
test-coveragescript tocomposer.jsonfor easily generating and viewing code coverage reports via PHPUnit. - CPU Core Detection Improvement: Enhances the
getCpuCoresutility function to handlenprocoutput more robustly (redirecting stderr) and adds support for detecting cores on macOS usingsysctl.
Changelog
Click here to see the changelog
- .gitignore
- Add entry
/.phpunit.result/to ignore the directory used for phpunit results (line 8).
- Add entry
- composer.json
- Update
nikic/php-parserdependency from~4.16to^4.18 || ^5(line 27). - Add
scriptsandscripts-descriptionssections, including thetest-coveragescript for running PHPUnit with coverage (lines 42-52).
- Update
- src/voku/SimplePhpParser/Model/BasePHPElement.php
- Modify
prepareNodeto usegetStartLine()if available, falling back togetLine()for compatibility with olderphp-parserversions (lines 100-107).
- Modify
- src/voku/SimplePhpParser/Parsers/Helper/Utils.php
- In
getPhpParserValueFromNode, addmethod_existscheck forgetParts()before calling it, accommodating changes inphp-parserv5 (lines 115-117). - In
getCpuCores, redirect stderr to stdout for thenproccommand to prevent errors from being printed (line 473). - Add logic to
getCpuCoresto detect CPU cores on macOS usingsysctl -n hw.ncpu(lines 503-514).
- In
- src/voku/SimplePhpParser/Parsers/PhpCodeParser.php
- Remove unused
use PhpParser\Lexer\Emulative;statement (line 8). - Change parser instantiation in
processto use(new ParserFactory())->createForNewestSupportedVersion()(line 202).
- Remove unused
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
Parse the code tree,
Upgrade the library,
New versions arrive.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively upgrades nikic/php-parser to support v5.x, which is a crucial update. The changes made for compatibility, such as updating the parser factory usage and handling deprecated methods like getLine() and getParts(), are well-targeted. The DX improvements, including the test coverage script and macOS core detection, are also valuable additions.
I've found one area in src/voku/SimplePhpParser/Parsers/Helper/Utils.php concerning the name representation handling for php-parser v5 that could be improved for robustness. Details are in the specific comment.
Overall, great work on tackling this upgrade!
Summary of Findings
- PHP-Parser v5 Name Representation Handling: In
src/voku/SimplePhpParser/Parsers/Helper/Utils.php, the fallback logic for obtaining string representation ofPhpParser\Node\Nameobjects for PHP-Parser v5 uses$node->value->name->name. This is not robust for fully qualified names. It's recommended to use$node->value->name->toString()for PHP-Parser v5 compatibility, as it correctly handles all types ofNamenodes.
Merge Readiness
The pull request is a significant step forward in modernizing the dependency on php-parser. However, there is a high-severity issue identified in src/voku/SimplePhpParser/Parsers/Helper/Utils.php related to name resolution with PHP-Parser v5 that should be addressed to ensure correctness. Once this is resolved, the PR should be in good shape for merging. As a reviewer, I am not authorized to approve pull requests, so please ensure further review and approval by authorized maintainers after addressing the feedback.
| $value = method_exists($node->value->name,'getParts') | ||
| ? implode('\\', $node->value->name->getParts()) | ||
| : $node->value->name->name; |
There was a problem hiding this comment.
This change correctly adapts for php-parser v5 by checking for getParts(). However, the fallback : $node->value->name->name for the v5 path might not be robust for all cases, especially for fully qualified names.
-
PHP-Parser v5 Name Handling: For
php-parserv5,PhpParser\Node\Nameobjects (includingFullyQualified,Relative, etc.) provide atoString()method for reliable string conversion. Using$node->value->name->namecan be problematic because:- For fully qualified names (e.g.,
new \My\Namespace\MyClass()), thenameproperty might not exist on theFullyQualifiedobject, or it might only contain the last part of the name, not the full FQN. $node->value->name->toString()is the recommended way to get the complete string representation of anyNamenode in v5.
- For fully qualified names (e.g.,
-
Original Elvis Operator: It appears the original Elvis operator
?: $node->value->name->name(which was part ofimplode(...) ?: $node->value->name->name) has been removed from the v4 path in this PR (thetruebranch of the ternary). Ifimplode('\\', $node->value->name->getParts())results in an empty string, the behavior for the v4 path will now differ from the original code. Was this change in behavior for the v4 path intentional?
For the v5 path, could we use $node->value->name->toString() for more reliable name resolution? This would align better with the php-parser v5 API.
$value = method_exists($node->value->name,'getParts')
? implode('\\', $node->value->name->getParts()) // v4 path (Note: original Elvis operator was removed in this PR)
: $node->value->name->toString(); // v5 path: use toString() for robust name resolution
Agent-Logs-Url: https://github.com/voku/Simple-PHP-Code-Parser/sessions/36a711fc-c894-41e1-bbe1-415f9b714722 Co-authored-by: voku <264695+voku@users.noreply.github.com>
WalkthroughThe pull request adds testing infrastructure improvements by introducing a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #63 +/- ##
============================================
- Coverage 82.72% 82.37% -0.36%
- Complexity 691 828 +137
============================================
Files 18 20 +2
Lines 1719 1974 +255
============================================
+ Hits 1422 1626 +204
- Misses 297 348 +51 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 46: The script line that checks XDEBUG_MODE currently enforces an exact
match and fails for comma-separated modes; update the check in the composer.json
scripts entry (the line containing the if [ "$XDEBUG_MODE" != "coverage" ] ...
message) to test whether the environment variable contains the substring
"coverage" (e.g., using a shell pattern or string search) instead of requiring
an exact equality, and keep the same error message and exit behavior if
"coverage" is not present.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d31d061-ad53-49e4-bf94-6abd7684041a
📒 Files selected for processing (4)
.gitignorecomposer.jsonsrc/voku/SimplePhpParser/Model/BasePHPElement.phpsrc/voku/SimplePhpParser/Parsers/Helper/Utils.php
| "scripts": { | ||
| "test-coverage": [ | ||
| "Composer\\Config::disableProcessTimeout", | ||
| "if [ \"$XDEBUG_MODE\" != \"coverage\" ]; then echo \"Run with 'XDEBUG_MODE=coverage composer test-coverage'\"; exit 1; fi;", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does XDEBUG_MODE support comma-separated modes (for example, "develop,coverage"), and does that still enable coverage collection?
💡 Result:
Yes, XDEBUG_MODE supports comma-separated modes such as "develop,coverage", and this enables coverage collection along with development helpers.
Citations:
- 1: https://m.academy/lessons/configure-modes-xdebug/
- 2: https://dev.to/robertobutti/test-coverage-did-you-set-xdebugs-coverage-mode-ij9
- 3: https://m.academy/lessons/configure-xdebug-modes
- 4: https://github.com/xdebug/xdebug/blob/master/xdebug.ini
- 5: https://stackoverflow.com/questions/78503179/why-do-my-phpunit-tests-in-netbeans-run-but-dont-result-in-any-code-coverage
- 6: https://xdebug.org/docs/basic
Allow coverage as one mode, not only exact match.
Line 46 currently fails when XDEBUG_MODE includes multiple modes (for example develop,coverage), even though coverage is enabled. Xdebug supports comma-separated mode values, and coverage collection works with such configurations.
Suggested fix
- "if [ \"$XDEBUG_MODE\" != \"coverage\" ]; then echo \"Run with 'XDEBUG_MODE=coverage composer test-coverage'\"; exit 1; fi;",
+ "if ! printf '%s' \"${XDEBUG_MODE:-}\" | grep -Eq '(^|,)coverage(,|$)'; then echo \"Run with 'XDEBUG_MODE=coverage composer test-coverage'\"; exit 1; fi;",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "if [ \"$XDEBUG_MODE\" != \"coverage\" ]; then echo \"Run with 'XDEBUG_MODE=coverage composer test-coverage'\"; exit 1; fi;", | |
| "if ! printf '%s' \"${XDEBUG_MODE:-}\" | grep -Eq '(^|,)coverage(,|$)'; then echo \"Run with 'XDEBUG_MODE=coverage composer test-coverage'\"; exit 1; fi;", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` at line 46, The script line that checks XDEBUG_MODE currently
enforces an exact match and fails for comma-separated modes; update the check in
the composer.json scripts entry (the line containing the if [ "$XDEBUG_MODE" !=
"coverage" ] ... message) to test whether the environment variable contains the
substring "coverage" (e.g., using a shell pattern or string search) instead of
requiring an exact equality, and keep the same error message and exit behavior
if "coverage" is not present.
|
The dependencies are updated and we will replace hand-rolled getCpuCores() with fidry/cpu-core-counter so that I will close this Pull Request PS: thanks for pushing thisn😇 |
2 participants
PR is to upgrade to
php-parser5.x.I made some DX changes on my way to updating
php-parserlibrary, I will cherry-pick these commits as independent PRs if you prefer.Tangential commits:
nproc– When running tests on MacOS, the stderr output was always printed, so I redirect it to stdio which is then parsed as normalnproctest-coverageComposer script – I wanted an easy way to see the current coverage HTML report so I might identify anywhere that my changes need new tests. Current coverage is 80%.Proposed Changes
These are the guts of the PR, and much less work than I expected: c55dc7a...a436917
php-parserhas a UPGRADE-5.0 guide. I've added checkboxes for each heading it has to try make reviewing as easy as possible:c55dc7a Instantiate parser via
::createForNewestSupportedVersion()composer require "nikic/php-parser":"^4.18 || ^5"(in BrianHenryIE@846e011, BrianHenryIE@63113e2)ExceptionandReflectionException, so I don't think there's any changes heremethod_existsfor deprecated::getParts()Scalarand didn't find anything relevantmodifier/iis not used in this project's codeNode::getLine()addressed in a436917I don't think this is a breaking change, but I haven't used this widely enough to be sure.
Summary by CodeRabbit
New Features
test-coveragecomposer command for generating code coverage reports in multiple formats including HTML output.Bug Fixes
Documentation
Chores