-
-
Notifications
You must be signed in to change notification settings - Fork 86
Fix compatibility to PHPUnit 9 #464
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
Merged
norberttech
merged 6 commits into
coduo:6.x
from
alexander-schranz:bugfix/php-matcher-phpunit-10-9-compatibility
Mar 27, 2023
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6a9012e
Fix compatibility to PHPUnit 9
alexander-schranz b6e70f0
Add phpunit also to require-dev part
alexander-schranz 591f74b
Move phpunit from tools to require-dev
alexander-schranz 54aabef
Fix prefer lowest of PHPUnit with PHP 8.1
alexander-schranz 0d96380
Update composer.locks with PHP 8.1.16
alexander-schranz 40e0692
Update Path to PHPUnit
alexander-schranz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Modifying tools/ seems more natural.
php-matcher/tools/composer.json
Line 5 in d2d7b1d
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.
Actually
"phpunit/phpunit": "~9 || ~10"
in tools should work even better but I'm starting to think that we might want to move PHPUnit into main composer.json require-dev section since phpmatcher is proving an integration with it. If PHPUnit would be added to composer.json instead of tools, test workflows would detect that since they should be expected at lowest/current/latest dependencies.Uh oh!
There was an error while loading. Please reload this page.
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.
The
require-dev
andtools
part is not keep in mind for project using the php-matcher and so will fail like in #463 and we recommend to this and not just set PHPUnit now to ^10 because else all will get failing version 6.0.12 on phpunit 9, so at current case I recommend to release a PHPUnit 9 compatible version, before requiring only PHPUnit 10.I did go with
conflict
this is the only way of handlingoptionalDependencies
just added it to avoid that incompatible version is installed in future. The only way would be move it as hard dependencies. Which is fine from my side, but I understand that maybe somebody don't want that PHPUnit is installed when using the package, so the only solution besideconflict
is set the supported version inrequire
section.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.
Lets move phpunit to require-dev in main composer.json - this will not make composer to install PHPUnit for everyone who is using phpmatcher, but it will let test suite to run against different PHPUnit versions.
https://phpunit.de/supported-versions.html
According to this page I think matcher 6.x should support
~8 || ~9 || ~10
versions of PHPUnit.Uh oh!
There was an error while loading. Please reload this page.
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.
We still require the
conflict
part as described above therequire-dev
does not disallow that false version of php-matcher is installed. Else we run into same problems when PHPUnit 11 is released.Lets say we remove the support for PHPUnit 8 and 9 in an upcoming release
6.1
release. With the conflict moved then to<10, >=11
users using PHPUnit 8 and 9 will only get the new version when they are also upgrading to PHPUnit 10 in there project. With the conflict they stay on the version supporting there related PHPUnit version. Without the conflict they will get incopatible coduo php matcher version which maybe doesn't support there PHPUnit version. This is becausecomposer
does only keeprequire
andconflict
in mind to find correct package version, therequire-dev
only for the package manager not the one using the package.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.
@norberttech can you help with generating the
composer.lock
files not sure how to handle this now.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.
yeah, that is indeed weird, could you maybe try to remove composer.lock and run
composer update
? If that won't work I will take a look at what's going on there.Uh oh!
There was an error while loading. Please reload this page.
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.
which php version I should use to update the
composer.lock
?I pushed now the locks with PHP 8.1.16
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.
8.1 should be fine as its the lowest supported version
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.
I think now all should be fine.