[RFC] New PHP min version for Mink and drivers

[stof]

There is some discussions happening on various driver repos about dropping PH 5.3 support. This issue is about discussing a consolidated plan for the whole set of Mink repos, to be consistent.

Arguments to drop support:

• all PHP versions < 7.1 are unmaintained (and PHP 7.1 reaches that point at the end of the year)
• Travis supports only PHP 5.6+ on their xenial infrastructure, forcing to use trusty for 5.4 and 5.5 and precise for 5.4. And they announced that the precise infrastructure will be decommissioned in the future, forbidding us to run tests on 5.3 at that point (and so to officially support it)
• PHPUnit is dropping support for unmaintained PHP versions regularly, which forces us to support a wide range of PHPUnit versions to run our testsuite, which becomes more painful over time. For now, we need these versions:
  • 4.8 for PHP 5.3 to 5.5
  • 5.7 for PHP 5.6
  • 6.5 for PHP 7.0
  • 7.4 for PHP 7.1+
  • we are currently not yet compatible with the new PHPUnit 8, but it supports PHP 7.2+
• PHPUnit 8 is adding void return type in many places. Making our testsuite compatible with PHPUnit 8 will be a pain if we need to keep support for PHP < 7.1
• Symfony 5 will add return types on a some interfaces, which will force us to do the same in our testsuite implementation (for BrowserKitDriver), which will require dropping support for PHP 5.x (or do crazy stuff to keep 2 implementations). One of the methods affecting us will use the object type, which requires PHP 7.2
• our new driver based on facebook/webdriver will require at least PHP 5.6+ (due to the min requirement of the library and they plan to make it PHP 7.1+ in the next version). This is not a strict requirement to match the other parts of Mink (it would have this requirement even without this min version bump on Mink), but it is nice to take that into account in the thinking.

As we start dropping support for old versions, I suggest dropping more than just 5.3, to benefit from a cleanup (as we don't do it often). I suggest to at least drop 5.3 to 5.5, which will free us from old Travis infrastructure and from PHPUnit 4.8. Then, multiple suggestions are possible:

• 7.2+, to make it easy to add support for Symfony 5 (but this is mostly things happening in the testsuite, so we could deal with it otherwise)
• 7.1+, to make it easy to migrate to PHPUnit 8 (while keeping support for PHPUnit 7.4 due to our needs), which also aligns with the min requirement of Symfony LTS, and with the current maintained PHP versions
• 7.0+
• 5.6+

Another thing to take into account is that Mink is currently used as part of the Drupal ecosystem. Here is their support matrix (source):

• Drupal 8.6 has PHP 5.5.9 as its min version
• Drupal 8.7 supports PHP 5.5.9+ for existing projects, but requires PHP 7.0.8+ for new projects (enforced by their installer)
• Drupal 8.8 will support only PHP 7.0.8+

As they are currently pinned on a dev version of Mink due to needing unreleased changes, we should avoid dropping support for these versions before the release (so that they can at least switch these branches to use our next stable release). The fact that Drupal 8.8 will still support PHP 7.0 might be an argument to keep support in Mink if they don't want to allow a range constraint (allowing to use an uptodate Mink on maintained PHP versions and the older release only on PHP 7.0). But we are not strongly tied to the Drupal ecosystem so this argument alone is not a full blocker.

@aik099 what is your mind on the new min version ?

[aik099]

I agree with your version dropping idea. Considering every 5th issue created is about dropping PHP 5.x support with only argumentation, that they're EOL. That was no big motivation for me.

Now once major consumers of Mink are dropping PHP 5 support as well and we don't want to stay behind it's only logical to support PHP 5.6+ only.

I'm not sure how we can release minor version of Mink/drivers considering that some of merged PRs turned out to be features. But otherwise I like the idea to make a minor release with all the features that Drupal is currently using.

[stof]

@aik099 my idea is to release a new minor version of Mink (minor, not patch, precisely because we have features in them), and then drop support for old PHP only in the following minor version.

[stof]

@aik099 so your vote would go to use 5.6 as the new min version ?

[oleg-andreyev]

I would vote to use 7.0+ because Drupal already dropped 5.5 and 5.6 source, old releases of Drupal will be able to use old releases of Mink/drivers.

[aik099]

@aik099 so your vote would go to use 5.6 as the new min version ?

Yes, it's 5.6 to allow people who missed PHP 7 upgrade (for whatever) still use new Mink features if they somehow manage to upgrade server/codebase to PHP 5.6.

With sed approach discussed in minkphp/MinkBrowserKitDriver#138 supporting new PHPUnit might not be that hard as well if we'll patch vendor code directly on Travis CI.

[stof]

I don't want to use this sed approach. The testsuite should be usable locally too. And we should not patch vendors to revert their types. That would be worse than staying on old versions.

[aik099]

Not patching their code, but patching our code. Our test suite in vendor folder I mean, so that our types match whatever PHP classes we're extending.

But I agree, that in such approach test suite locally won't work.

Having lots of classes for every BC breaking change in any of libraries we're using (e.g. PHPUnit) is the way to go then unfortunately.

[greg-1-anderson]

The ask for Drupal is:

• Create an unmaintained 1.7 branch that has the current commits from the 1.7.x-dev branch alias.
• Make a 1.8.0 stable release, ideally from the same commit as the 1.7 branch.

It would be great if you could hold off on removing old php's until after the 1.8.0. You could make a 1.9.0 immediately after, and decommission whatever php's you thought best at that time. The code freeze for Drupal 8.8.0-alpha is 14 October; it's possible that 8.8.x might use a 1.9.0 release, but more likely this would be put off until 8.9.x.

Also, regarding supporting complex combinations of dependencies, e.g. multiple versions of Symfony, multiple versions of phpunit, etc., you might want to consider Composer Test Scenarios.

[stof]

It would be great if you could hold off on removing old php's until after the 1.8.0.

this is already the plan, see previous comments.

Also, regarding supporting complex combinations of dependencies, e.g. multiple versions of Symfony, multiple versions of phpunit, etc., you might want to consider Composer Test Scenarios.

Our issue is not regarding testing the different scenarios. It is the complexity of making the same testsuite compatible with all these PHPUnit versions.
Our goal is not to support many PHPUnit version, but to be able to run tests on all our supported PHP versions. Having to support multiple PHPUnit versions is a consequence of that (as modern PHPUnit versions cannot be used for old PHP versions, and old PHPUnit versions may not be compatible with modern PHP versions as they are unmaintained).

[ryanaslett]

Our goal is not to support many PHPUnit version, but to be able to run tests on all our supported PHP versions. Having to support multiple PHPUnit versions is a consequence of that (as modern PHPUnit versions cannot be used for old PHP versions, and old PHPUnit versions may not be compatible with modern PHP versions as they are unmaintained

PHPUnit is essentially forcing libraries to reduce their breadth of php support as a result of their own very tightly constrained support. Drupal had to do several 'crazy' things to make it work: https://git.drupalcode.org/project/drupal/commit/c9898ab

As an aside, Drupal 9.0.x branch is open, and is starting at php7.2+ support, but may become 7.3+

[aik099]

@stof, what if we will:

1. have several classes with same name, e.g. Base_PHPUnit_Framework_TestCase - one class for each PHPUnit version, where BC break occurred
2. each class will have a helper method for every PHPUnit method, that has changed signature or otherwise introduced a BC break
3. in our test suite we'll use these helper methods instead of original PHPUnit methods, that have BC break; other usage places remains unchanged
4. if something more radical has changed (e.g. how exception testing works) then we'll create a polyfill and use it instead
5. the custom autoloader will load corresponding class based on a PHP version, where test suite is being executed

[greg-1-anderson]

Regarding #4, Symfony recently released a bunch of polyfills for phpunit that might be helpful to you.

I will reiterate what I said above as well: it would be great if you could make a stable tag for the 1.7 sha that folks have been using as stable for years (and also in the mink-selenium2-driver), and also create a 1.7 branch. If rename your branch alias from 1.7.x-dev to 1.8.x-dev without doing this, you're going to break all of the Composer-managed Drupal 8 sites that exist. There's no need to wait for 1.8 to be perfect before making a tag on the code that's already in use. It would give folks time to make their projects safe before 1.8.x came out.

[aik099]

Regarding #4, Symfony recently released a bunch of polyfills for phpunit that might be helpful to you.

Thanks @greg-1-anderson , I already see, that custom autoloader, that I've talked about in #787 (comment) was implemented in PHPUnit Bridge (see https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/PhpUnit/SymfonyTestsListener.php), but using class_alias function.

I've also noticed other interesting ideas in https://github.com/symfony/symfony/tree/master/src/Symfony/Bridge/PhpUnit/Legacy folder.

Altering PHPUnit code to remove :void might not be great idea (as @stof told about altering PHP code in vendor folder in general). But we could surely use some concepts from PHPUnit Bridge in here.

Locally I use PhpStorm to run the tests and it has it's own wrapper, which doesn't support (probably) usage of simple-phpunit. If all PHPUnit Bridge magic could somehow wrapped in custom bootstrap file, that can be specified in phpunit.xml, then this would be great.

I will reiterate what I said above as well: it would be great if you could make a stable tag for the 1.7 sha that folks have been using as stable for years (and also in the mink-selenium2-driver), and also create a 1.7 branch. If rename your branch alias from 1.7.x-dev to 1.8.x-dev without doing this, you're going to break all of the Composer-managed Drupal 8 sites that exist. There's no need to wait for 1.8 to be perfect before making a tag on the code that's already in use. It would give folks time to make their projects safe before 1.8.x came out.

Why having a branch named as version, as we did in https://github.com/minkphp/MinkSelenium2Driver isn't good enough? Packagist created correct alias and all developers using that alias were happy.

[greg-1-anderson]

The branch named after the version is sufficient to prevent all Composer-managed Drupal sites from breaking. Having a stable tag would also allow us to stop requiring minimum-stability: dev in our project templates.

[oleg-andreyev]

@aik099 @stof we need to return on discussing this RFC.

• We need to create 1.7.x branch because Drupal is using 1.7.x-dev (which is pointing to master) https://github.com/drupal/drupal/blob/8.8.x/composer.json#L19 and it won't change likely because: we haven't release any new version in 1.7.x and soon Drupal will release their 8.8
• After creating 1.7.x branch, change branch alias in master to 1.8.x
• I suggest bumping PHP min version at least to 5.6 because based on stats it's still widely used https://blog.packagist.com/php-versions-stats-2019-1-edition/
• After creating 1.8.x alias, update all drivers to support it.

[greg-1-anderson]

It would be really swell to have a stable tag for the head of the 1.7.x branch so that we're not stuck with minimum-stability: dev for Drupal.

[umulmrum]

Did you have opportunity to make progress on this issue? I'm really sorry for press-ganging but the missing support for Symfony 5 is a huge blocker for my project.

To add 2 cents to the topic: I'd advocate to keep things as simple as possible and just drop support for PHP < 7.2 (in a minor release, so fixes for 1.7.x can still be delivered without having to upgrade PHP). I'm not familiar with the code, but judging from this issue everything else seems overly complex, brittle, and time-consuming.

[pamil]

I've forked this repository into friends-of-behat/mink. There is v1.8.0 release published providing support for PHP 7.2, 7.3 and 7.4, as well as Symfony 3.4, 4.4 and 5.x (FriendsOfBehat#1). I don't plan to change anything else there.

You can temporarily replace this library with the fork like in this PR: Sylius/Sylius#11024.

[alexander-schranz]

Not sure what the state of this Issue is, currently to support symfony 5 and newer php versions I created a BC Layer for the older PHP versions see #794.