-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Skip test if symfony/property-info
doesn't support phpstan/phpdoc-parser:2.0
yet
#58997
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
Conversation
66826d3
to
5c4f7cd
Compare
phpstan/phpdoc-parser>=2
symfony/property-info
doesn't support phpstan/phpdoc-parser:2.0
yet
cdaad34
to
33caa1c
Compare
$versionRange = InstalledVersions::getVersionRanges('symfony/property-info'); | ||
|
||
if ( | ||
'5.4.x-dev' !== $versionRange && version_compare($versionRange, '5.4.47', '<') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version_compare() doesn't handle ".x-dev" versions well, thus the hard check
Hi! As far as I understand, this means that Serializer is broken for users when phpdoc-parser 2.0 is installed? Shouldn't we conflict with it in that case or add it as a dev dependency with a version range limit to 1.x? |
That was indeed what my first commit contained! Then we had a discussion with @xabbuh about it. In fact, the problem with doing this is that it blocks compatibility with version 2.0 in 5.4 (and other maintained branches), where support does work on the latest patch releases. Indeed, the problem is that support is not available on unmaintained versions (6.3 for example). We thought it would be better to just skip the test rather than penalize maintained versions (Serializer installs PropertyInfo 6.3 in high deps). We agreed that if the dev installs versions that are no longer maintained, it's his/her responsibility. I hope I've made myself clear, I struggled a bit summarizing the whole conversation 😄 |
|
||
if ( | ||
'5.4.x-dev' !== $versionRange && version_compare($versionRange, '5.4.47', '<') | ||
|| version_compare($versionRange, '5.4.48', '>=') && '6.4.x-dev' !== $versionRange && version_compare($versionRange, '6.4.15', '<')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I get the constraint on 5.4.48, shouldn't this be on >= 6?
|| version_compare($versionRange, '5.4.48', '>=') && '6.4.x-dev' !== $versionRange && version_compare($versionRange, '6.4.15', '<')) { | |
|| version_compare($versionRange, '6', '>=') && '6.4.x-dev' !== $versionRange && version_compare($versionRange, '6.4.15', '<') | |
) { |
…pstan/phpdoc-parser:2.0` yet
33caa1c
to
136fc16
Compare
Actually, the fix might be to remove that conflict in 6.4: |
Nevermind 😄 |
Replaced by #59008 of course. |
The support for
phpstan/phpdoc-parser
2.0 was introduced in PropertyInfo lately (cc @xabbuh). Unfortunately, Serializer cannot benefit from this in 5.4 because the highest version of PropertyInfo installable is 6.3 (see the failing job). Before running this particular test, we need to ensure PropertyInfo is installed with a version that supportsphpstan/phpdoc-parser
2.0.