-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 missing dev requirement against "phpspec/prophecy" #5617
Conversation
Prophecy ships with phpunit. Adding this dependency exposes us to autoloading conflicts. What exactly pushed you to create this PR? |
I didn't noticed that, you're right: https://github.com/sebastianbergmann/phpunit/blob/61d34e8dd6eb3555900f0f2a2fa9e7e570730102/composer.json#L34 |
We aren't requiring "phpunit/phpunit" explicitly, but "symfony/phpunit-bridge" (which has their own logic for version resolving): SonataAdminBundle/composer.json Line 71 in bc279a8
This line seems to remove the dependency, so, as result, "phpspec/prophecy" is not being installed: $SYMFONY_PHPUNIT_REMOVE = $getEnvVar('SYMFONY_PHPUNIT_REMOVE', 'phpspec/prophecy'.($PHPUNIT_VERSION < 6.0 ? ' symfony/yaml': '')); Results
|
Maybe we should work an this to solve the tooling problem? |
@phansys we're installing the bridge to get deprecation-related goodies, but we're not using simple-phpunit, are we? We're using the phpunit phar, which should contain prophecy. |
I think you're referring to the CI process, but this change was made focusing on the end user, which checks out the project and wants to test it; like the case I've shared before (see Results). IMO, if there is no strong conflict with the usage of the self declared dependencies on the CI suite, I think we could make both cases compatible. |
I think we should document what ways of testing we support. Our contributing guide is huge but does not contain this information. Right now, the way I test is downloading the phar in my Lines 64 to 70 in ddeef2e
I hear what you say @phansys , but I think it would be better to solve this by using the tool @core23 is suggesting. If you want to read, here is a long thread about this subject: sonata-project/dev-kit#197 |
I agree that the proposed solution at sonata-project/dev-kit#372 could solve the issue, but AFAIK, "symfony/phpunit-bridge" accomplish exactly that task:
|
I reread the conversation on dev-kit and yes, that would be a good solution too. IMO you can carry sonata-project/dev-kit#265 if you think we should go that way. I closed it because I no longer observed that kind of autoloading issues, probably because there was some work in upstream libraries to make them easier to avoid. |
Could you please rebase your PR and fix merge conflicts? |
Subject
Add missing dev requirement against "phpspec/prophecy".
I am targeting this branch, because these changes respect BC.
Changelog