-
Notifications
You must be signed in to change notification settings - Fork 146
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
Dev dependencies should include the compatible PHPunit version #34
Dev dependencies should include the compatible PHPunit version #34
Conversation
And this is exactly why these need to be in composer.json.
@@ -24,7 +24,9 @@ | |||
}, | |||
"require-dev": { | |||
"league/oauth2-facebook": "^1.1", | |||
"symfony/security-guard": "^2.8|^3.0" | |||
"symfony/security-guard": "^2.8|^3.0", | |||
"phpunit/phpunit": "^4.8", |
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 we should allow 5.x version of PHPUnit in this case too with ^4.8|^5.0
, right?
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.
It would also need finetuning of the php-code-coverage
since they're intertwined (phpunit 5 requires code coverage 3 or 4), but yeah it could work. In general though I myself favor just picking a single major to avoid having differences between developers. I would just postpone the 5.x upgrade until PHP 5.5 is dropped in this project itself.
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'm fine with using just one major, as this - because being a dev
dependency, this won't cause any problems with end-user projects, correct (i.e. an end-user could still install phpunit/phpunit
version 5 in their project, without this conflicting)?
And, is there a precedence for including php-code-coverage
as a dependency in other OS projects? I'm just not familiar with this - and mostly, I want to follow what's most expected/standard.
Thanks!
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 recommend sticking to the single major for the universally reproducible results. As dev requirements are installed per project this means it's fine if you have 3 projects on the same machine depending on PHPunit ^4.6
, 5.1.*
and ^5.2
- each will get their own copy.
As dev requirements themselves are never propagated to external projects you can in practice do whatever the hell you want with them, it's your project 😄 php-code-coverage
itself has been installed 25.5 million times so I'd think it enough precedent 😉
I'll pick this up along with #36, do expect a freakish PR coming up from that with all files modified hehe.
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.
My question is why do we really need phpunit/php-code-coverage
in this project? I agree with PHPUnit, we use it for tests, but we don't use php-code-coverage
here. As for me adding a dependency which we don't use in a project just in case is a bad idea which leads to overhead and even could confuse other devs.
I agree but it broke when I removed it hehe. I'll investigate why it did that. |
The code coverage dependency is no longer needed, and PHP requirement should match Symfony's.
Apparently that fixed itself along the way. |
@@ -12,7 +12,7 @@ | |||
} | |||
], | |||
"require": { | |||
"php": " >=5.5.0", | |||
"php": " >=5.5.9", |
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.
👍
Awesome, thanks! |
Added the correct
phpunit
andphp-code-coverage
dependencies so everyone can run the tests right out of the box after a fork.