JsonMapper
JsonMapper copied to clipboard
Added support for nikic/php-parser:^5.0
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
coverage: 99.718%. remained the same when pulling acd19ff8484d01c6b4fd3b3b7e1a28b3d86cd44e on guillaumesmo:patch-2 into 83ea4d8904701ec10baf652cea2723e91b24dd02 on JsonMapper:develop.
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?
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.
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.