Skip to content

Conversation

rhoerr
Copy link
Contributor

@rhoerr rhoerr commented Apr 11, 2025

The current merged php-classes.phar fails to actually run due to an error in usage of the packaged version of nikic/php-parser.

Call to undefined method PhpParser\ParserFactory::create()

This PR:

  • Updates that package to 5.4 (latest) for up to date PHP compatibility;
  • corrects the constructor usage;
  • updates the docker image PHP build version to 8.3;
  • adds a composer install to build-phar so it's not dependent on knowing to run that separately prior to build;
  • updates version numbers;
  • and rebuilds the phar itself.

Following changes, the test suite completes successfully:

ryan@mage-os-rhoerr:/var/www/mage-os/mage-os.php-dependency-list$ sudo bin/run-regression-tests 
+ DOCKER_IMAGE=php-dependency-list-env:1.1.0
++ docker images -q php-dependency-list-env:1.1.0
+ [[ d5eb6975eb67 == '' ]]
++ pwd
+ docker run -t --volume /var/www/mage-os/mage-os.php-dependency-list:/usr/src php-dependency-list-env:1.1.0 bash -c 'composer install; exit 0'
Composer could not detect the root package (mage-os/php-dependency-list) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Nothing to install, update or remove
Generating autoload files
27 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
++ pwd
+ docker run -t --volume /var/www/mage-os/mage-os.php-dependency-list:/usr/src php-dependency-list-env:1.1.0 bash -c './vendor/bin/phpunit ./tests --testdox; exit $?'
PHPUnit 9.6.22 by Sebastian Bergmann and contributors.

List Files From File List (MageOs\PhpDependencyList\ListFilesFromFileList)
 ✔ Correct file paths are returned with data set "Test Non existent filepath"
 ✔ Correct file paths are returned with data set "Specify same directory twice"
 ✔ Correct file paths are returned with data set "Just look for PHP files recursivley"
 ✔ Correct file paths are returned with data set "Just look for PHP files"
 ✔ Correct file paths are returned with data set "Just look for di.xml files"
 ✔ Correct file paths are returned with data set "Just look for composer.json files"
 ✔ Correct file paths are returned with data set "Just look for module.xml files"
 ✔ Correct file paths are returned with data set "Just look for all files"

List Files From Files (MageOs\PhpDependencyList\ListFilesFromFiles)
 ✔ Correct file paths are returned with data set "Test Non existent filepath"
 ✔ Correct file paths are returned with data set "Specify same directory twice"
 ✔ Correct file paths are returned with data set "Just look for PHP files recursivley"
 ✔ Correct file paths are returned with data set "Just look for PHP files"
 ✔ Correct file paths are returned with data set "Just look for di.xml files"
 ✔ Correct file paths are returned with data set "Just look for composer.json files"
 ✔ Correct file paths are returned with data set "Just look for module.xml files"
 ✔ Correct file paths are returned with data set "Just look for all files"

List Files From Stdin (MageOs\PhpDependencyList\ListFilesFromStdin)
 ✔ Empty stream
 ✔ Single file
 ✔ Two files
 ✔ Empty then non empty file
 ✔ Non empty then empty file
 ✔ Three empty files

Referenced Classes In Composer Json (MageOs\PhpDependencyList\ReferencedClassesInComposerJson)
 ✔ Throws exception on invalid json
 ✔ Correct modules are extracted with data set "no requirements"
 ✔ Correct modules are extracted with data set "single requirement"
 ✔ Correct modules are extracted with data set "two requirements"
 ✔ Correct modules are extracted with data set "php extension requirement"

Referenced Classes In Di XML (MageOs\PhpDependencyList\ReferencedClassesInDiXML)
 ✔ Throws exception on non xml code
 ✔ Parses preferences
 ✔ Parses object arguments
 ✔ Parses virtual types

Referenced Classes In Module Xml (MageOs\PhpDependencyList\ReferencedClassesInModuleXml)
 ✔ Throws exception on invalid json
 ✔ Correct modules are extracted with data set "no requirements"
 ✔ Correct modules are extracted with data set "single requirement"
 ✔ Correct modules are extracted with data set "two requirements"

Referenced Classesin PHP (MageOs\PhpDependencyList\ReferencedClassesinPHP)
 ✔ Throws exception on invalid php
 ✔ Throws exception on xml

Time: 00:00.162, Memory: 8.00 MB

OK (37 tests, 37 assertions)

And the executable itself correctly outputs the class name for a given file on PHP 8.3.

ryan@mage-os-rhoerr:/var/www/mage-os/tmp/test-install-1.0.2$ php8.3 ~/bin/php-classes.phar --json-output -f setup/src/Magento/Setup/Controller/ResponseTypeInterface.php
setup/src/Magento/Setup/Controller/ResponseTypeInterface.php

I opted not to update PhpUnit at this time to avoid refactoring or breaking the existing tests.

Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

will it be able to be used on older versions of PHP for older versions of magento or it's not needed?

@rhoerr
Copy link
Contributor Author

rhoerr commented Apr 11, 2025

will it be able to be used on older versions of PHP for older versions of magento or it's not needed?

I set it to use the platform's PHP version, and the package still supports back to PHP 7. So, if something needs to be built on PHP 7, that should still work by using PHP 7 during execution. This is mostly used for nightly builds and Mage-OS releases though, all of which will be PHP 8.2+ only going forward.

$parser        = (new ParserFactory())->createForHostVersion();

Copy link
Contributor

@Vinai Vinai left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you 👍

@mage-os-ci mage-os-ci merged commit 2310502 into mage-os:main Apr 11, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants