Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Upgrade nikic/php-parser to v5+ #63

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

Open
BrianHenryIE wants to merge 8 commits into voku:master
base: master
Choose a base branch
Loading
from BrianHenryIE:upgrade-phpparser-5

Conversation

@BrianHenryIE
Copy link

@BrianHenryIE BrianHenryIE commented Jun 1, 2025 •
edited
Loading

PR is to upgrade to php-parser 5.x.

I made some DX changes on my way to updating php-parser library, I will cherry-pick these commits as independent PRs if you prefer.

Tangential commits:

Proposed Changes

These are the guts of the PR, and much less work than I expected: c55dc7a...a436917

php-parser has a UPGRADE-5.0 guide. I've added checkboxes for each heading it has to try make reviewing as easy as possible:

Screenshot 2025年05月31日 at 7 35 46 PM

c55dc7a Instantiate parser via ::createForNewestSupportedVersion()

The createForNewestSupportedVersion() and creatForHostVersion() are available since PHP-Parser 4.18.0, to allow libraries to support PHP-Parser 4 and 5 at the same time more easily.

composer require "nikic/php-parser":"^4.18 || ^5" (in BrianHenryIE@846e011, BrianHenryIE@63113e2)

I don't think this is a breaking change, but I haven't used this widely enough to be sure.

Copy link

sonarqubecloud bot commented Jun 1, 2025

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-parser requirement in composer.json to allow versions ^4.18 || ^5.
  • PHP Parser Instantiation: Changes the parser creation logic in PhpCodeParser to use ParserFactory::createForNewestSupportedVersion() for better compatibility across php-parser versions.
  • PHP Parser API Compatibility: Adds checks (method_exists) for deprecated or changed methods like getLine (now getStartLine) and getParts in BasePHPElement and Utils to ensure compatibility with both php-parser v4 and v5 APIs.
  • Test Coverage Script: Adds a test-coverage script to composer.json for easily generating and viewing code coverage reports via PHPUnit.
  • CPU Core Detection Improvement: Enhances the getCpuCores utility function to handle nproc output more robustly (redirecting stderr) and adds support for detecting cores on macOS using sysctl.

Changelog

Click here to see the changelog
  • .gitignore
    • Add entry /.phpunit.result/ to ignore the directory used for phpunit results (line 8).
  • composer.json
    • Update nikic/php-parser dependency from ~4.16 to ^4.18 || ^5 (line 27).
    • Add scripts and scripts-descriptions sections, including the test-coverage script for running PHPUnit with coverage (lines 42-52).
  • src/voku/SimplePhpParser/Model/BasePHPElement.php
    • Modify prepareNode to use getStartLine() if available, falling back to getLine() for compatibility with older php-parser versions (lines 100-107).
  • src/voku/SimplePhpParser/Parsers/Helper/Utils.php
    • In getPhpParserValueFromNode, add method_exists check for getParts() before calling it, accommodating changes in php-parser v5 (lines 115-117).
    • In getCpuCores, redirect stderr to stdout for the nproc command to prevent errors from being printed (line 473).
    • Add logic to getCpuCores to detect CPU cores on macOS using sysctl -n hw.ncpu (lines 503-514).
  • src/voku/SimplePhpParser/Parsers/PhpCodeParser.php
    • Remove unused use PhpParser\Lexer\Emulative; statement (line 8).
    • Change parser instantiation in process to use (new ParserFactory())->createForNewestSupportedVersion() (line 202).
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

  1. 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. ↩

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of PhpParser\Node\Name objects 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 of Name nodes.

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.

Comment on lines +115 to +117
$value = method_exists($node->value->name,'getParts')
? implode('\\', $node->value->name->getParts())
: $node->value->name->name;
Copy link

@gemini-code-assist gemini-code-assist bot Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

  1. PHP-Parser v5 Name Handling: For php-parser v5, PhpParser\Node\Name objects (including FullyQualified, Relative, etc.) provide a toString() method for reliable string conversion. Using $node->value->name->name can be problematic because:

    • For fully qualified names (e.g., new \My\Namespace\MyClass()), the name property might not exist on the FullyQualified object, 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 any Name node in v5.

  2. Original Elvis Operator: It appears the original Elvis operator ?: $node->value->name->name (which was part of implode(...) ?: $node->value->name->name) has been removed from the v4 path in this PR (the true branch of the ternary). If implode('\\', $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

BrianHenryIE reacted with eyes emoji
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

AltStyle によって変換されたページ (->オリジナル) /