Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromgregurco:3.4
Dec 10, 2018

Conversation

@gregurco
Copy link
Contributor

@gregurcogregurco commentedDec 3, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
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)

Copy link
Member

@derrabusderrabus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

@gregurco
Copy link
ContributorAuthor

@derrabus thanks for review 👍 I changed comments.

@nicolas-grekas
Copy link
Member

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

OskarStark and gregurco reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to thenext milestoneDec 3, 2018
@gregurcogregurco changed the base branch from3.4 tomasterDecember 3, 2018 22:39
@gregurcogregurcoforce-pushed the3.4 branch 2 times, most recently from84fafe8 to79d771bCompareDecember 3, 2018 22:41
@gregurco
Copy link
ContributorAuthor

@nicolas-grekas thank you for remark. Changed.

Copy link
Contributor

@ostroluckyostrolucky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

@gregurcogregurco 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 6Dec 4, 2018
@gregurcogregurcoforce-pushed the3.4 branch 3 times, most recently from14f1b75 toa63cb9eCompareDecember 7, 2018 07:26
@gregurco
Copy link
ContributorAuthor

gregurco commentedDec 7, 2018•
edited
Loading

@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 setupSYMFONY_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.

@gregurcogregurco 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 6Dec 7, 2018
@ostrolucky
Copy link
Contributor

ostrolucky commentedDec 7, 2018•
edited
Loading

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, Iam 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
Copy link
ContributorAuthor

@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
Copy link
Contributor

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

status: needs review

gregurco reacted with thumbs up emoji

@fabpot
Copy link
Member

Thank you@gregurco.

gregurco reacted with thumbs up emoji

@fabpotfabpot merged commit30609bf intosymfony:masterDec 10, 2018
fabpot added a commit that referenced this pull requestDec 10, 2018
…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-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@derrabusderrabusderrabus requested changes

@fabpotfabpotfabpot approved these changes

@stofstofstof approved these changes

+1 more reviewer

@ostroluckyostroluckyostrolucky requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

7 participants

@gregurco@nicolas-grekas@ostrolucky@fabpot@stof@derrabus@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp