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

[6.0] Limit PHPUnit to version 8.0 and above. #29285

Merged
merged 1 commit into from Jul 25, 2019

Conversation

crynobone
Copy link
Member

The changes has been done on develop branch for laravel/laravel. It is logical that we does this as well on laravel/framework.

The changes has been done on `develop` branch for `laravel/laravel`. It is logical that we does this as well on `laravel/framework`.
@antonkomarev
Copy link
Contributor

antonkomarev commented Jul 25, 2019

Then we need to increment php: ^7.1.3 version. Because PHPUnit 8 requires php: ^7.2.

Edit: Nevermind, I missed that Laravel 5.9 already has php: ^7.2 requirement.

@driesvints
Copy link
Member

Heya thanks. We made the change on the skeleton repo to encourage people to make use of the 8.0 release. But we'd like to keep using the ^7.5 constraint on the framework one for now to still test that PHPUnit version for the "lowest" build. PHPUnit 7 is still actively supported until february next year. We'll probably drop support for it in Laravel 7.

@driesvints driesvints closed this Jul 25, 2019
@crynobone
Copy link
Member Author

@driesvints I don't see the value of testing against lower than 7.5 going to 5.9 or 6.0. It just means that whatever we doing with Illuminate\Foundation\Testing should be working with phpunit/phpunit version 8.0.0 and above.

Based on travis-ci setup the core is now tested only against 7.5.0 and latest 8.x version.

@driesvints
Copy link
Member

@crynobone right. But that's what I meant. We're still supporting PHPUnit ^7.5. Or did you mean something else? Seems like there's some confusion here?

@driesvints driesvints changed the title [5.9/6.0] Limit PHPUnit to version 8.0 and above. [6.0] Limit PHPUnit to version 8.0 and above. Jul 25, 2019
@driesvints
Copy link
Member

@crynobone I'll re-open this to see what Taylor says.

@driesvints driesvints reopened this Jul 25, 2019
@crynobone
Copy link
Member Author

Or did you mean something else? Seems like there's some confusion here?

  • We support "phpunit/phpunit": "^7.5|^8.0" in Laravel 5.8 because it still needed to be able to tests against installation with PHP 7.1, and that the reason why we made laravel/laravel 5.8 to use "phpunit/phpunit": "^7.5".
  • For 6.0, we already bumping the minimum PHP version to 7.2 which make it possible to completely stop supporting 7.5. The laravel/laravel app skeleton has been updated to 8.0 for quite sometime laravel/laravel@0582a20#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780
  • Technically developer should be able to use phpunit/phpunit=^7.5 when upgrading to 6.0, but when we say we still supporting 7.5 that would mean if any bug related phpunit usage we also need to use code that are supported by both 7.5 and 8.0, if we upgrade the version now that would mean we just need to fix based on 8.0 and above.

@taylorotwell taylorotwell merged commit 4516d94 into laravel:master Jul 25, 2019
@driesvints
Copy link
Member

@crynobone PHPUnit 7.5 supports 7.2. The version constraint you removed here was in require-dev, not require. It was only meant to use PHPUnit 7 to run the laravel framework test suite when applying "lowest". Because you removed it here the framework won't be tested with PHPUnit 7 anymore. At no time a PHPUnit is enforced in any of the require lists on any of the component's or main composer.json file. PHPUnit is still actively supported until February 2020 and dropping support for it now is a little too early imo. But since Taylor merged it I guess that's fine by me as well.

@crynobone crynobone deleted the patch-1 branch September 14, 2019 06:44
@driesvints
Copy link
Member

Just fyi: because this was removed, the test suite didn't caught a breaking change with #30989 and PHPUnit 7 support is now broken for 6.x. I suggest we re-add this so the test suite actually runs PHPUnit 7 on the lowest matrix.

@crynobone
Copy link
Member Author

laravel/laravel v6.0.0 comes with phpunit 8.0 as minimum. https://packagist.org/packages/laravel/laravel#v6.0.0

laravel/framework v6.0.0 comes with phpunit 8.3 as minimum. https://packagist.org/packages/laravel/framework#v6.0.0

Is there a reason why we need to readd PHPUnit 7 support?

@driesvints
Copy link
Member

@crynobone Laravel 6 definitely kept on supporting PHPUnit 7. Like I said before: the versions you're linking to are "dev" requirements for the test suite and not "required" versions. We usually drop support for PHPUnit versions in February releases when PHPUnit has dropped support themselves.

@driesvints
Copy link
Member

driesvints commented Jan 13, 2020

Also see #31099

Sorry, I mean: #31101

@crynobone
Copy link
Member Author

Then this shouldn't have happened.

laravel/laravel@0582a20#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780

@driesvints
Copy link
Member

driesvints commented Jan 13, 2020

@crynobone yes it does. It's only on the Laravel skeleton to encourage people to start using PHPUnit 8 when they start with a fresh Laravel install. Versions in the composer file of the Laravel skeleton don't indicate minimum support of libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants