Skip to content
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

[PhpUnitBridge] install PHPUnit 7 on PHP 7.1 and fix requir. for PHPUnit 6 #29439

Merged
merged 1 commit into from Dec 10, 2018

Conversation

Projects
None yet
7 participants
@gregurco
Copy link
Contributor

commented Dec 3, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Added support of PHPUnit 7.4 if PHP version is 7.1+ (release link: https://packagist.org/packages/phpunit/phpunit#7.4.5).
Also I found that PHPUnit 6.5 required PHP 7.0, not 7.2 (proof: https://packagist.org/packages/phpunit/phpunit#6.5.13)

@derrabus
Copy link
Contributor

left a comment

The PR changes the default PHPUnit versions. The new defaults look reasonable to me.

The changes you've made to the comments however are a bit misleading.

Show resolved Hide resolved src/Symfony/Bridge/PhpUnit/bin/simple-phpunit Outdated
Show resolved Hide resolved src/Symfony/Bridge/PhpUnit/bin/simple-phpunit Outdated
Show resolved Hide resolved src/Symfony/Bridge/PhpUnit/bin/simple-phpunit Outdated
@gregurco

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

@derrabus thanks for review 👍 I changed comments.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

That should be submitted on master: we shouldn't change the version on a patch release.

@nicolas-grekas nicolas-grekas added this to the next milestone Dec 3, 2018

@gregurco gregurco changed the base branch from 3.4 to master Dec 3, 2018

@gregurco gregurco force-pushed the gregurco:3.4 branch 2 times, most recently from 84fafe8 to 79d771b Dec 3, 2018

@gregurco

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

@nicolas-grekas thank you for remark. Changed.

@stof

stof approved these changes Dec 4, 2018

@gregurco gregurco force-pushed the gregurco:3.4 branch 4 times, most recently from fb107eb to e5c7c6b Dec 4, 2018

@ostrolucky
Copy link
Contributor

left a comment

Also I found that PHPUnit 6.5 required PHP 7.0, not 7.2

Comment you deleted does not say that you need to use PHP 7.2 for PHPUnit 6. It explains that PHP 7.2 can't be used with PHPUnit 5. You are free to use PHPUnit 5 until version 7.2.

Comments you reversed no longer explain why is given phpunit version installed for given php version. And suddenly installing phpunit 6 for php 7.0-7.1 and phpunit 7 for php>=7.2 by default will probably break the builds for lot of people.

This PR significantly changes current strategy from installing lowest working PHPUnit version for given PHP version to installing latest PHPUnit version.

@gregurco gregurco changed the title [PhpUnitBridge] install PHPUnit 7 on PHP 7.1 and fix requir. for PHPUnit 6 [WIP][PhpUnitBridge] install PHPUnit 7 on PHP 7.1 and fix requir. for PHPUnit 6 Dec 4, 2018

@gregurco gregurco force-pushed the gregurco:3.4 branch 3 times, most recently from 14f1b75 to a63cb9e Dec 5, 2018

@gregurco

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

@ostrolucky

This PR significantly changes current strategy from installing lowest working PHPUnit version for given PHP version to installing latest PHPUnit version.

User is free to setup SYMFONY_PHPUNIT_VERSION env variable and to receive the version he/she wants. So, by default Symfony will setup the last available version for used PHP, what is good and also user has control on it.

It explains that PHP 7.2 can't be used with PHPUnit 5.

The comment was wrong. PHPUnit 5 can be used with PHP 7.2 and also 7.3... so, that's why I changes the comment.

@gregurco gregurco changed the title [WIP][PhpUnitBridge] install PHPUnit 7 on PHP 7.1 and fix requir. for PHPUnit 6 [PhpUnitBridge] install PHPUnit 7 on PHP 7.1 and fix requir. for PHPUnit 6 Dec 7, 2018

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

User is free to setup SYMFONY_PHPUNIT_VERSION env variable and to receive the version he/she wants. So, by default Symfony will setup the last available version for used PHP, what is good and also user has control on it.

I know, doesn't change the fact that it will by default update major phpunit version without asking.

The comment was wrong. PHPUnit 5 can be used with PHP 7.2 and also 7.3... so, that's why I changes the comment.

In that case condition should have been removed to install PHPUnit 5, which would follow current strategy and there is no need for PHPUnit 6 and 7 at all. At the time there were issues with it #27370 but it appears it has been resolved since then.

For the record, I am for using newer versions, but it needs to be highlighted that this PR is not about fixing the requirement, but about installing latest PHPUnit version possible for given PHP version, which changes the strategy that was used until now that was using oldest major working for given PHP version.

@gregurco gregurco force-pushed the gregurco:3.4 branch from a63cb9e to 7c18b56 Dec 8, 2018

@gregurco

This comment has been minimized.

Copy link
Contributor Author

commented Dec 8, 2018

@ostrolucky

... it needs to be highlighted that this PR is not about fixing the requirement, but about installing latest PHPUnit version possible for given PHP version ...

do you have any suggestions how to highlight that? Generally it's the single scope of this PR.

@ostrolucky

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2018

No need, I did highlight it with my comment. It needs comment from symfony members now

status: needs review

@gregurco gregurco force-pushed the gregurco:3.4 branch from 7c18b56 to 30609bf Dec 8, 2018

@fabpot

fabpot approved these changes Dec 10, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Thank you @gregurco.

@fabpot fabpot merged commit 30609bf into symfony:master Dec 10, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 10, 2018

feature #29439 [PhpUnitBridge] install PHPUnit 7 on PHP 7.1 and fix r…
…equir. for PHPUnit 6 (gregurco)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[PhpUnitBridge] install PHPUnit 7 on PHP 7.1 and fix requir. for PHPUnit 6

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Added support of PHPUnit 7.4 if PHP version is 7.1+ (release link: https://packagist.org/packages/phpunit/phpunit#7.4.5).
Also I found that PHPUnit 6.5 required PHP 7.0, not 7.2 (proof: https://packagist.org/packages/phpunit/phpunit#6.5.13)

Commits
-------

30609bf [PhpUnitBridge] install PHPUnit 7 on PHP 7.1 and fix require for PHPUnit 6

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.