-
Notifications
You must be signed in to change notification settings - Fork 48
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
add support for phpunit 11 #157
Conversation
Unfortunately, just tweaking the version constraints won't be enough to make it work, because PhpUnit 11 removes features that are currently used by this library (like the |
- added sebastian/exporter dependency
29c3569
to
f797efd
Compare
f797efd
to
2b69b3d
Compare
@@ -4,13 +4,15 @@ | |||
|
|||
use PHPUnit\Framework\Constraint\Constraint; | |||
use PHPUnit\Framework\Constraint\IsEqual; | |||
use SebastianBergmann\Exporter\Exporter; |
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.
This will break when using the phpunit phar release, see #158.
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, i just noticed your merge request and was wondering whether it isn't a phar problem when a class is incorrectly vendored?
phpunit 11 does no longer include the exporter so not adding this dependency isn't really an option for this pr
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.
phpunit 11 does no longer include the exporter
Nope, it's definitely there in the composer.json for v11, and also in the v11 phar release (still vendored as PHPUnit\SebastianBergmann\Exporter\Exporter
).
was wondering whether it isn't a phar problem when a class is incorrectly vendored
I'm pretty sure that's the way it's supposed to be. The phpunit phar release basically vendors all it's dependencies under the PhpUnit
root namespace.
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 adding this dependency isn't really an option for this pr
You could add a compatibility factory, i.e. something like this:
use PhpUnit\SebastianBergmann\Exporter\Exporter as VendoredExporter;
use SebastianBergmann\Exporter\Exporter;
class PhpUnitCompatFactory {
public static function exporter(): Exporter|VendoredExporter {
return class_exists(VendoredExporter::class) ? new VendoredExporter() : new Exporter();
}
}
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.
Ah wait, I just noticed that you explicitely require sebastian/exporter
in this PR, so this will probably work fine as long as the Exporter
instances stay private.
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, i assume the phar vendors the exporter differently to prevent conflicts with a different version explicitly being installed in the repository so the actual fix is adding the sebastian/exporter
to the composer requirements
piepline fails due to phpunit config. |
composer.json
Outdated
@@ -15,16 +15,12 @@ | |||
"require": { | |||
"php": "^8.1", | |||
"matthiasnoback/symfony-config-test": "^5.0", | |||
"phpunit/phpunit": "^10.0 || ^11.0", |
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 guess it fix the lowest run, if you change it to ^10.5.11 || ^11.0
Do we have a rough idea of when this is going to be released? It’s preventing our application from being upgraded to PHPUnit 11. |
ping @matthiasnoback @Nyholm |
@wickedOne Sorry, I'm not personally involved in these libraries anymore. |
seems it's @stloyd now |
@matthiasnoback sorry, i wasn't aware of that :| |
@wickedOne No problem :) |
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.
Thank you for this PR.
And thank you for the reviews.
I think this looks correct. I'll merge this PR and hope that you will test it before we tag a new major version.
@Nyholm here's one of my repos running succesfully with phpunit 10.5.11 and phpunit 11.1.3 (both php 8.2 & 8.3) and the dev-master of this repo be aware that SymfonyTest/SymfonyConfigTest#79 still needs to be merged & tagged and added to the composer.json of this repo |
Is currently a released of this blocked by symfony/symfony#53812 ? |
No, not what I know |
allow installation of phpunit 11
removed conflict section as well as it won't install phpunit <9.6 anyway?
requires SymfonyTest/SymfonyConfigTest#79 to be merged & tagged