JsonMapper icon indicating copy to clipboard operation
JsonMapper copied to clipboard

Added support for nikic/php-parser:^5.0

Open guillaumesmo opened this issue 9 months ago • 3 comments

Q A
Branch? develop
Bug fix? no
New feature? yes
Deprecations? no
Tickets none
License MIT
Changelog changed
Doc PR none

Upgrade of dependency nikic/php-parser

Only notable change I could find is the removed ParserFactory::create method, for which I used the recommended alternative with a fallback for older versions

https://github.com/nikic/PHP-Parser/blob/master/UPGRADE-5.0.md#changes-to-the-parser-factory

guillaumesmo avatar May 04 '24 12:05 guillaumesmo

Coverage Status

coverage: 99.718%. remained the same when pulling acd19ff8484d01c6b4fd3b3b7e1a28b3d86cd44e on guillaumesmo:patch-2 into 83ea4d8904701ec10baf652cea2723e91b24dd02 on JsonMapper:develop.

coveralls avatar May 04 '24 15:05 coveralls

Thank you for this one as well @guillaumesmo however I believe this requires some small tweaking before it can be merged. The obvious one is a conflict on the CHANGELOG.md, could you go ahead and rebase this on the current develop? Secondly whilst I was checking the GitHub actions for this PR I noticed none of the builds from the matrix are installing nikic/php-parser version 5.x due to vimeo/psalm still pinned on ^4.16

The last finding makes me a bit curious on what their PR would solve or is it more a generic update of a dependency?

DannyvdSluijs avatar May 04 '24 18:05 DannyvdSluijs

Hi @DannyvdSluijs

Indeed, the GH actions always pull the v4 of the package because vimeo/psalm is not compatible with v5 yet.

However, this dependency is listed in the require-dev dependencies, which means a project which requires JsonMapper but not Psalm will include v5, since dev dependencies of dependencies are not installed.

I understand that this might be an issue here as the v5 support is not really tested by GH actions. To be worked around, it would need removing vimeo/psalm and redoing a composer update before running the tests.

guillaumesmo avatar May 05 '24 18:05 guillaumesmo

So I took some time to think about what it is this PR is trying to resolve and I've now found the underlying bug. Given a project already has a requirement on nikic/php-parser:^5.0 and doing a composer require json-mapper/jsonmapper it will install version 0.0.1. Trying to run composer require json-mapper/json-mapper:^2 will result in an failed installation:

Composer could not detect the root package (test/test) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
./composer.json has been updated
Composer could not detect the root package (test/test) version, defaulting to '1.0.0'. See https://getcomposer.org/root-version
Running composer update json-mapper/json-mapper
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - json-mapper/json-mapper[2.0.0, ..., 2.7.0] require nikic/php-parser ^4.10 -> found nikic/php-parser[v4.10.0, ..., v4.19.1] but it conflicts with your root composer.json require (^5.0).
    - json-mapper/json-mapper[2.8.0, ..., 2.22.1] require nikic/php-parser ^4.13 -> found nikic/php-parser[v4.13.0, ..., v4.19.1] but it conflicts with your root composer.json require (^5.0).
    - Root composer.json requires json-mapper/json-mapper ^2 -> satisfiable by json-mapper/json-mapper[2.0.0, ..., 2.22.1].

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.

Installation failed, reverting ./composer.json and ./composer.lock to their original content.

Still can't think of a good way to make this part of the CI/CD but will accept the PR for now to allow others to use JsonMapper when nikic/php-parser:^5 is a dependency.

DannyvdSluijs avatar May 14 '24 09:05 DannyvdSluijs