Skip to content

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@
"openlss/lib-array2xml": "^1.0",
"symfony/expression-language": "^2.3|^3.0|^4.0|^5.0|^6.0"
},
"conflict": {
"phpunit/phpunit": ">=11.0"
},
Copy link

@szepeviktor szepeviktor Mar 26, 2023

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.

"phpunit/phpunit": "^10.0",

"phpunit/phpunit": "^10.0 <11.0",

Copy link
Member

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.

Copy link
Contributor Author

@alexander-schranz alexander-schranz Mar 27, 2023

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.

The require-dev and tools 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 handling optionalDependencies 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 beside conflict is set the supported version in require section.

Copy link
Member

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.

Copy link
Contributor Author

@alexander-schranz alexander-schranz Mar 27, 2023

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 the require-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 because composer does only keep require and conflict in mind to find correct package version, the require-dev only for the package manager not the one using the package.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

@alexander-schranz alexander-schranz Mar 27, 2023

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

Copy link
Member

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

Copy link
Contributor Author

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.

"suggest": {
"openlss/lib-array2xml": "In order ot use Coduo\\PHPMatcher\\Matcher\\XmlMatcher",
"symfony/expression-language" : "In order to use Coduo\\PHPMatcher\\Matcher\\ExpressionMatcher"
Expand Down
8 changes: 4 additions & 4 deletions src/PHPUnit/PHPMatcherConstraint.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function toString() : string
return 'matches given pattern.';
}

protected function failureDescription(mixed $other) : string
protected function failureDescription($other) : string
{
$errorDescription = $this->matcher->error() ?: 'Value does not match given pattern';
$backtrace = $this->matcher->backtrace();
Expand All @@ -50,20 +50,20 @@ protected function failureDescription(mixed $other) : string
. "\nBacktrace:\n" . $this->matcher->backtrace();
}

protected function matches(mixed $other) : bool
protected function matches($other) : bool
{
return $this->matcher->match($other, $this->pattern);
}

/**
* {@inheritdoc}
*/
protected function fail(mixed $other, string $description, ComparisonFailure $comparisonFailure = null) : never
protected function fail($other, $description, ComparisonFailure $comparisonFailure = null) : never
{
parent::fail($other, $description, $comparisonFailure ?? $this->createComparisonFailure($other));
}

private function createComparisonFailure(mixed $other) : ?ComparisonFailure
private function createComparisonFailure($other) : ?ComparisonFailure
{
if (!\is_string($other) || !\is_string($this->pattern) || !\class_exists(Json::class)) {
return null;
Expand Down