PHPUnit 9+ (for PHP 8 support) #10019

brynwhyman · 2021-07-11T22:53:39Z

Overview

Creating this spike to investigate the approach for adopting PHPUnit 9. This is currently the main blocker for full PHP 8 support.

Update: Then do all the work :-)

Tasks

See if it's possible to upgrade SapphireTest to use PHPUnit 9
Our favoured approach is to have CMS tests to also be upgraded (in addition to project code compatibility), so document the reasoning for this in the spike
Figure out the best way to move through this upgrade, given that it's assumed to relate to both CMS and project code
- Do we do a 'big bang' approach?
- Iteratively upgrade, prioritising project code use before CMS code?
Understand the implication of this change for people who are currently using SapphireTest for their own projects
- Assuming we'll need a comms plan to advise that adding PHPUnit 9 support, we'll need to explicitly move away from supporting out of date PHP versions (assuming PHPUnit isn't compatible with these versions)
Engage with project developers to understand their current requirements for project testing
Investigate what support could be given to projects upgrading to this new version
Is docs enough? Or could we provide an upgrade helper if it's expected to be a challenge?
Outline the options for the various ways of releasing the change - should it stay in framework, be in a new module, or something else?

Timebox: 10 days (including consultation)

Notes

See the related issues:

PRs

Remove dms polyfill

Rerun after removing polyfills above

Rerun after polyfill removed

Archive controller policy

emteknetnz · 2021-07-26T02:33:25Z

I've made very good progress getting dual support for both phpunit5 and phpunit9 - both are currently passing in travis (php 7.1 in phpunit 5, php 7.4 and 8.0 are running phpunit 9 https://travis-ci.com/github/silverstripe/silverstripe-framework/builds/233625826 - this is using a custom .travis file and also some forks of assets and the like that have compatibility commits

The core of this solution is to have 2 separate versions of SapphireTest (and other classes such as FunctionalTest) within the same SaphireTest.php file on the Silverstripe\Dev namespace, and dynamically load the correct one via class_exists() checks

I believe that we MUST use this solution as there's a requirement to support both phpunit 5 (not compatible with php8) AND phpunit 9 (not compatible with php7.1 or php7.2). This means that we cannot have something like a new class called SapphireTestNine, because all core unit tests extends SapphireTest. The phpunit 9 API change will be a breaking change for projects, even if they're running 7.4. This will prevent then from seamlessly running composer update of their project code which is the expectation for doing a minor upgrade on a project. Projects that were setup with silverstripe/installer will have [phpunit|sminnee]/phpunit ^5.7 in their composer require-dev, so wouldn't even be able to install without modification. When a site upgrades from php7 to php8 however, it is a breaking change so there is an expectation that some code might needs to be change, so this would be a reasonable time to require that unit tests be updated.

phpunit5 and phpunit9 have different API, the main things being:

functions such as setUp() have gone from no return type to having a : void return type
quite a few functions were deprecated and removed, along with the @expectedException annotation

There's no way around the fact that we need to update all the core unit tests in some capacity. The can be mostly achieved with a global regex find and replace, though plenty of unit tests needed some additional work.

Where it made sense, I've added some forwards-compatible methods to the phpunit 5 version of SapphireTest, and some backwards compatible methods to phpunit 9 SapphireTest

I've got a bunch of loose notes that will enventually form something on an upgrade guide in the description on #10028 (comment)

Investigate what support could be given to projects upgrading to this new version
Is docs enough? Or could we provide an upgrade helper if it's expected to be a challenge?

I think docs should be enough. The average project will be vastly simpler than core. it's likely to be just updating the return types on their setUp() functions to add : void. This can easily be done with VSCode. I'd be against creating some sort of 'upgrade' CLI utility. I think most project would only do this as part of an upgrade to php8, and it's fair to expect developers to have to upgrade some of their project code themselves during this to get things php8 compatible.

Outline the options for the various ways of releasing the change - should it stay in framework, be in a new module

This needs be a core update, not a module. This is because of the need to support dual versions on phpunit.
We'll need to release a new major of silverstripe/recipe-testing that includes phpunit9 instead of the sminnee fork for phpunit 5.
Merge the framework PR that includes the update to SapphireTest, then start upgrading other core modules as required, then upgrade satelitte modules. Remove the --ignore-platform-reqs flag from travis-shared so that phpunit 9 installs

Figure out the best way to move through this upgrade, given that it's assumed to relate to both CMS and project code .. Do we do a 'big bang' approach? .. upgrade, prioritising project code use before CMS code?

I think we should go for big bang after the 4.9 release. This doesn't mean merge every PR at once, we'd merge framework first, then do all the other modules one at a time. I'd much prefer to do this all back-to-back rather than incrementally (I'm happy to do all the work myself). This is likely to cause a fair amount of red builds in travis for a while until we fix all the loose ends. Since the bulk of the technical challenge has been solved in the framework PR, so it should be technically easy to do the rest of the core modules which are expected to be simpler than framework

emteknetnz · 2021-07-26T02:49:46Z

SapphireTest is a wrapper around phpunit that does a couple of things:

Initialises TestSession which, as the name implies, creates a session specially for the test
Handles silverstripe things like temporary database and loading database fixtures.
Has a handful of Silverstripe util methods such as public static findEmail() which sees if emails sent in unti tests were sent to the TestMailer

SapphireTest extends the phpunit class TestCase, so the vast bulk of assertions and the like is just phpunit

I'm pointing this out specifically for this:

Engage with project developers to understand their current requirements for project testing`

This seems like it implies we should be looking to add extra functionality to SapphireTest as part of this spike. I'd be very against doing this at this stage as what we actually want to do is just ensure that existing unit tests work on phpunit 9 with as little friction as possible. Feature development should be done separately, though I don't even think we should look to 'enhance' SapphireTest as it's just a wrapper. PHPunit 9 is the enhancement.

emteknetnz · 2021-07-26T05:03:21Z

Most modules have the following for require-dev:

"sminnee/phpunit": "^5.7.29",`

or

"sminnee/phpunit": "^5.7.29",
"sminnee/phpunit-mock-objects": "^3.4.9",

This is going to cause composer conflicts with phpunit/phpunit": "^9", since some modules may still have the sminnee version while others have the phpunit version. We can solve this via an intermediary testing module.

Recommend we take the existing silverstripe/recipe-testing which is

"require": {
    "silverstripe/recipe-plugin": "^1",
    "silverstripe/recipe-core": "^1@dev || ^4@dev",
    "sminnee/phpunit": "^5.7.29",
    "sminnee/phpunit-mock-objects": "^3.4.9",
    "silverstripe/behat-extension": "^4.4",
    "silverstripe/serve": "^2.2",
    "squizlabs/php_codesniffer": "^3"
}

And make the following changes:

mark it as archived
split it into "php-testing" and a "behat-testing" modules
release two major versions of php-testing, one for phpunit 5 version and another for phpunit 9
Remove recipe-plugin and recipe-core as requirements
Require framework instead
Add "replace": { "<module>": "*" } which conveniently removes a module from installing, which prevents conflicts

"silverstripe/php-testing" ^1

"require": {
    "php": "^7.1",
    "silverstripe/framework": "^4",
    "sminnee/phpunit": "^5.7.29",
    "sminnee/phpunit-mock-objects": "^3.4.9",
    "squizlabs/php_codesniffer": "^3"
}
"replace: {
    "phpunit/phpunit": "*"
}

"silverstripe/php-testing" ^2

"require": {
    "php": "^7.3 || ^8",
    "silverstripe/framework": "^4.10",
    "phpunit/phpunit": "^9",
    "squizlabs/php_codesniffer": "^3"
}
"replace": {
    "sminnee/phpunit": "*",
    "sminnee/phpunit-mock-objects": "*",
}

"silverstripe/behat-testing" ^2

"require": {
    "silverstripe/framework": "^4",
    "silverstripe/behat-extension": "^4.4",
    "silverstripe/serve": "^2.2",
}

Modules:

Add the following to all modules require-dev so they work on any version of php 7.1 to 8+

"silverstripe/php-testing": "^1 || ^2"

Projects:

Projects are going to vary depending on how long ago there were setup. If using installer

https://github.com/silverstripe/silverstripe-installer/blob/4.3/composer.json

"require-dev": {
    "phpunit/phpunit": "^5.7",
}

https://github.com/silverstripe/silverstripe-installer/blob/4.5/composer.json

"require-dev": {
    "phpunit/phpunit": "^5.7",
    "sminnee/phpunit-mock-objects": "^3.4.5"
},

https://github.com/silverstripe/silverstripe-installer/blob/4.8/composer.json

"require-dev": {
    "sminnee/phpunit": "^5.7",
    "sminnee/phpunit-mock-objects": "^3.4.5"
},

These will all continue to work with the dual versions of SapphireTest while they're on php 7. When they upgrade to php8, we'll need to make it clear for them to change to "phpunit/phpunit": "^9" and remove the phpunit-mock-objects fork (it's now part of phpunit/phpunit core).

Note: Unlike require, nested require-dev's don't install, meaning projects won't automatically start installing silverstripe/behat-testing and the like

We probably shouldn't recommend they use "silverstripe/php-testing" since that's more intended for internal module use, not project use, and we want the flexibility to change this at will later.

maxime-rainville · 2021-09-21T00:37:35Z

I had high level overview. Valiant effort in trying to keep both PHPUnit 5.7 and PHPUnit 9 support.

However, I suspect we might have reach an impasse where we force people to upgrade to PHPUnit 9 and break some APIs along the way.

Maybe there's a way we can split the SilverStripe\Dev namespace into its own module. This would allow us to release a PHPUnit 5.7 compatible version under a 1 branch and a PHPUnit9 version under a 2 branch. Framework could allow you to install either the 1 or 2 branch.

If that's not possible, I think we need to seriously look at breaking SEMVER here and drop support for PHP 7.1 and 7.2.

emteknetnz · 2021-09-21T01:18:35Z

I suspect we might have reach an impasse where we force people to upgrade to PHPUnit 9 and break some APIs along the way

Correct, the impasse is upgrading to php8, which developers do understand is a potentially breaking change to their both their code and some requirements

Maybe there's a way we can split the SilverStripe\Dev namespace into its own module. This would allow us to release a PHPUnit 5.7 compatible version under a 1 branch and a PHPUnit9 version under a 2 branch. Framework could allow you to install either the 1 or 2 branch.

I'm not sure this would achieve what we want it to? Presumably we'd have a requirement in framework of "silverstripe/dev": "^1 || ^2" - for most people this would default to ^2 which would be php7.4 compatible and use phpunit9, thus breaking their unit tests. Since sminnee/phpunit is merely a dev requirement, I don't think it would be used in any way to restrict the version of silverstripe/dev installed. This means that a site with phpunit 5 tests that simply ran composer update with no modification to their composer.json file just broke their tests, so this is a breaking change during a minor upgrade.

If that's not possible, I think we need to seriously look at breaking SEMVER here and drop support for PHP 7.1 and 7.2.

It doesn't make a difference? Let's say we did this and we release phpunit 9 version of SapphireTest in framework 4.10. Well if my site is on php7.4 when I run composer update my tests break.

Again, the correct time to break the tests is during a sites upgrade to php8, and not merely composer update

maxime-rainville · 2021-09-21T02:11:56Z

PHP 7.4 is about to enter limited support at the end of this year. It will be EOL by the end of 2022. In this context, I find it very painful to have weird bifurcated logic in Framework to give people an extra year to consider their PHP8 upgrade. I would rather we rip the band aid off and be done with it.

If we want to give people more time to make the transition, then maybe we guarantee support for the 4.9.0 branch until PHP 7.4 is EOL. There's precedent for doing this as well with our previous major releases of GraphQL and the asset permalink work.

This would actually fit nicely with our GraphQL v4 release. 4.10 could be the first release where we transition to GraphQL v4 and PHPUnit 9. The 4.9 release line would be the last release to support both GraphQL 3, PHPUnit 5.7 and PHP 7.1-7.3.

Which ever way we do this, I suspect the 4.9 to 4.10 upgrade won't be straightforward and people will minimally have to fiddle with their composer file.

silverstripe/dev

If we don't want people to accidentally upgrade to silverstripe/dev, we can just put a conflict statement in the v2 branch to disallow the installation of sminee/phpunit.

There might be some value in splitting off dev namespace into it's own module any way.

emteknetnz · 2021-09-21T02:43:24Z

There's precedent for doing this as well with our previous major releases of GraphQL

We'll, we haven't actually released this yet so we don't know what the fallout from this will be Probably not that bad because most end users won't notice the change in graphql, only those who actually have a custom graphql endpoint which is probably a fairly small number. Majority of sites will just be using graphql for asset-admin and it won't need to do any code changes. For a small number of sites though this will be a breaking change, though there is a relatively easy fix of just specify ^3 for graphql.

If anything the precedent for graphql is what I'm proposing we do here, which is dual version support, as messy as it is.

asset permalink work

Was that to do with the hash being introduced in the url on the public asset store? Yeah that was not a good experience. That was a change got reversed in 4.4 cos it broke peoples sites if I remember correctly?

4.9.0 branch until PHP 7.4 is EOL

So force people to upgrade to php8 if they want framework 4.10? That seems like we're just adding in a hurdle to upgrade and adding significant maintenance headache to ourselves

What happens if there's a security patch we release that requires an API addition? We break semver on the 4.9 branch?

This seems particularly nasty when we're up to say version 4.13 and we need to backport something to that old 4.9 version we still need to support. This is very nasty IMO. We're effectively making 4.9 the end of the road for php7.

we can just put a conflict statement in the v2 branch to disallow the installation of sminee/phpunit

Would need to validate this, though I think because it's a dev requirement it'll behave differently. Dev requirements of required modules aren't considered when doing a composer install

Even if dev requirements are considered specifically for conflict, it's not fail proof because people can still do composer install --no-dev, write a lock file, and now they're on ^2 instead of ^1. You could argue it's user error at this point though.

I've provided a solution here which will result in minimal breakages on peoples sites. I don't really think that a solution that either breaks peoples sites or just prevents them from upgrading is going to get much community acceptance.

maxime-rainville · 2021-09-21T03:21:07Z

If anything the precedent for graphql is what I'm proposing we do here, which is dual version support, as messy as it is.

SS4.0 shipped with GraphQL v1. We're not supporting GraphQL v1 and v2 anymore. Part of the rational there was that a library we relied on for our graphQL support wouldn't work with the latest version of PHP. Which is exactly the situation we're in now.

Was that to do with the hash being introduced in the url on the public asset store? Yeah that was not a good experience. That was a change got reversed in 4.4 cos it broke peoples sites if I remember correctly?

We didn't do that lightly. But there was a fundamental design flaw in assets that couldn't be remedied easily without some upgrade pains.

So force people to upgrade to php8 if they want framework 4.10?

I'd say 4.10 can support PHP7.4 and up ... with soft support for PHP 7.3 if you don't care about unit test.

PHP7.3 has been EOL for some time now, so it shouldn't be such a shocking surprise we're dropping support for it.

What happens if there's a security patch we release that requires an API addition? We break semver on the 4.9 branch?

Adding an API to in a patch release is not great, but sometimes it's necessary to fix a security issue and we've done it in the past.

I've provided a solution here which will result in minimal breakages on peoples sites. I don't really think that a solution that either breaks peoples sites or just prevents them from upgrading is going to get much community acceptance.

The solution you got there involves a lot of bifurcated logic which we'll be stuck maintaining for a while. We've already been maintaining some weird fork of PHPUnit for some time now ... which we should never have done. I think we are at a stage where we need to have a clean break.

We're not hanging out devs to dry if we give them support an extra year to make the transition.

We try our best to support third party libraries and PHP versions for as long as possible, but sometimes there's factors out of our control that makes that impossible.

At some point, we just need to be honest with the community and explain that it's not practical for us to indefinitely maintain old APIs we are inheriting from third party dependencies.

emteknetnz · 2021-09-21T04:05:56Z

Adding an API to in a patch release is not great, but sometimes it's necessary to fix a security issue and we've done it in the past.

Yeah but you realise that means we essentially have 2 active minor versions, instead of 1? 4.9 would end up sticking around as "supported" for ages along with the actual current minor. We would be increasing our maintainence burden by doing this. This I do not agree with at all.

The solution you got there involves a lot of bifurcated logic which we'll be stuck maintaining for a while

This is not really a problem in practical terms though? SapphireTest doesn't exactly get a lot of work done it - https://github.com/silverstripe/silverstripe-framework/commits/4/src/Dev/SapphireTest.php

I'd estimate the ongoing "maintainence" work here to be near zero. Unlike GraphQL, SapphireTest is not in active development. After all SapphireTest is just a Silverstripe wrapper around PHPUnit to get things like fixtures working

Bulk of the work here is updating all the unit tests to work with phpunit9, which I've already made large inroads into

The downside to this approach seems to be "more work for CMS squad", however I estimate this extra work to be very low, so it's not really a downside?

Alternative approach you're suggesting here has multiple downsides, including more ongoing maintenance, more than just having dual support.

emteknetnz · 2021-09-21T22:19:10Z

I've summarised a few approaches below. I'm obviously biased to the solution I presented in the POC PR's linked above, though I really do think it's the only viable solution as the other approaches are going to cause users an bad upgrade experience where either things break or they can't upgrade.

Background

SapphireTest is a wrapper around a forked version of phpunit5 and mainly does things such as get test session and fixtures to work. It also utility functions such as seeing if emails were sent by the test mailer.
FuntionalTest is an extension of SapphireTest simulates GET and POST request to application being tested
phpunit5 is not compatible with php8
This lack of compatibility is the major blocker to us saying that silverstripe works with php8
We need SapphireTest to work with phpunit9
phpunit9 is similar to phpunit5 though there are some funciton signature differences, meaning that projects and module will be required to update their unit tests, even if just to add a :void return type to some functions
phpunit9 is not compatible with php7.1 or php7.2, though it works with php7.3 and php7.4

Options

A. Two different versions SapphireTest/FunctionalTest in the framework codebase supported phpunit5 and phpunit9 respectively. Use the correct version depending on the version of phpunit installed**

Pros:

Non-semver breaking change, means that composer update will not break your site during an upgrade
Sites on php7 can upgrade to phpunit9 at their leisure
Ditto for community modules
The breaking change of going to phpunit9 is forced at the same time as the breaking change going to php8 where developers will expect to have to make some code and dependency modifications.
We can retain support for older versions of php such as 7.1 until we decide to drop it

Cons:

Duplicate SapphireTest/FunctionalTest files

Notes:

I've already done several PR's for this POC to upgrade the unit tests in modules to both phpunit5 and phpunit9 and those are worked as expected
SapphireTest and FunctionalTest are very mature, do their job and seem very unlikely to have new functionality added to them any time soon. I've estimated the ongoing maintainence burdern of running duplicate classes as minimal
This is my recommended because it seems like the only way to maintain a minor upgrade experience for projects

B. Make php7.3 the minimum required for 4.10 - Make a semver breaking change and only include the phpunit9 SapphireTest/FunctionalTest in the codebase.

Pros:

Single SapphireTest and FunctionalTest files

Cons:

Breaking change, bad upgrade experience. Running composer update means that unit tests will no longer work on projects. Developers will need to make code changes.
This is not how composer update should work for minor upgrades. What's should happen is you upgrade and things still work the same as before, with bugs fixed and additional features added
Community module maintainers will find their unit tests suddenly breaking if they work out of the vendor folder of a regular silverstripe install. They'll also be forced to use have php7.3 as a supported minimum

C. Make php8 the minimum required for 4.10 - Make a semver breaking change and only include the phpunit9 SapphireTest/FunctionalTest in the codebase.

Pros:

Single SapphireTest and FunctionalTest files

Cons:

4.9 becomes the end of the line for sites on php7. php8 upgrade project required to get to 4.10
Obvious blocker for adoption for later version, this will annoy people
We'll still need to backport security fixes to 4.9, for all the sites still on php7, even when we're on version like 4.13 - so we'll end up with 2 current minors and increase the maintainence burden.
Community module maintainers are likely to get very frustrated where their unit tests only work on either php7 or php8

maxime-rainville · 2021-09-21T22:46:16Z

Option A

I'm pretty sure having two different and contradictory APIs in one release is not SEMVER compliant either.
We're not going to maintain two sets of unit test forever. We're just delaying the pain for projects.

Option B

PHP 7.3 is already EOL, I would set the minimum to PHP 7.4.

I would combine this with an extra year of support for Silverstripe CMS 4.9

This would cover critical bug fixes and security fixes
I would try to combine this with a switch to GraphQL v4 in 4.10

In effect, this gives you a year to:

migrate your project to a supported version of PHP
adapt your tests to work with PHP Unit 9
get your users off IE11
get your project off GraphQL v3
(get off Swiftmailer maybe)

I think this is better because it gives clear guidance and timeframes for projects to move off deprecated dependencies.

Option C

I don't think we should do that.

emteknetnz · 2021-09-21T22:53:18Z

(Option A)

We're not going to maintain two sets of unit test forever. We're just delaying the pain for projects.

Ah, that's not what we're doing 😆 we have duplicate SapphireTest + FunctionalTest classes only, not unit tests.

The PRs listed above upgrades the supported module unit tests themselves to work with phpunit9 and still work with phpunit5

(Option B)

This really means that we're making 4.10 a quasi-major release

At this point I'd just call it 5.0

maxime-rainville · 2021-09-26T21:48:07Z

When I did my initial review, I missed the part where we add a bunch of polyfills to restore API that PHPUnit has deprecated. I would like some clarity about how we want to approach this.

Copying deprecated APIs from PHPUnit into our own codebase and adding them to our own API would be a terrible mistake. We already have a gigantic API surface to maintain. The last thing we should be doing is taking over APIs that the PHPUnit maintainer doesn't want to look after.

Alternatively, we could backport some of the newer APIs back to the sminne/phpunit and update all our unit test to move away from using deprecated APIs. Wasting time to add things would be painful but it would be less bad than adding to our API surface.

On a related note, the PHPUnit support timeline seems to indicate that they release a new major in February of most years. We should expect a PHPUnit10 in Feb 2022. Looking at their master branch looks like it will drop support for PHP7 altogether.

We probably need to decide now what we want to do PHPUnit10. This means potentially supporting 3 different versions of PHPUnit simultaneously. Or we skip PHPUnit10 altogether and we have this entire discussion again in a year or two when PHPUnit 9 support gets deprecated.

emteknetnz · 2021-09-26T22:33:08Z

Looking at my previous notes on #10028 - it's basically to prevent having to rewrite large chunks of core unit tests which need to work in BOTH phpunit5 and phpunit9. Yes we could update them to work with straight phpunit9 functions, but we'd need to update a lot of unit tests (possibly not that bad, maybe find and replace will be good enough. And then we'd also need to add the forward compatibility polyfills to phpunit5)

Searching in kitchen-sink for

assertContains nets 2021 results
assertNotContains nets 401 results
assertInternalType nets 718 results

The backwards compatibility polyfills also make it easier for existing projects to keep their unit tests as is for when they need to migrate to phpunit9. You could argue this is just being overly nice since they will have to do some work e.g. migrate off the docblocks for expected exceptions

Wasting time to add things would be painful but it would be less bad than adding to our API surface.

It's probably worth re-exploring some 'find and replace' to migrate everything to phpunit9 to see if it's a semi-quick win, though it's certainly more work and I don't see backwards compatibility polyfills as really that bad maintainence wise (and obviously they're way better migration wise). I wouldn't automatically assume doing a bunch of extra work "would be less bad", particularly when there's an opportunity cost associated with doing work.

assertArraySubset is definitely a special case. It has been reimplemented with a backwards compatibility community module that can be included via a trait. It's a complex assertion and I want to keep it as we'll just end up wasting time duplicating this function.

Notes copied from previous comment:

assertInternalType() and assertNotInternalType() were removed - upgrade guide - sebastianbergmann/phpunit#3368 - though I've added backwards compatible versions - we've added a backwards compatible version of assertInternalType() to SapphireTest to support existing core unit tests on phpunit5, but not for assertNotInternalType() which wasn't used in any core tests

assertArraySubset() has been removed. It can be polyfiled for backwards compatibility on individual SapphireTest classes with a trait in a module:

composer require --dev dms/phpunit-arraysubset-asserts

use DMS\PHPUnitExtensions\ArraySubset\ArraySubsetAsserts;

class MyTest extends SapphireTest
{
    use ArraySubsetAsserts;

phpunit10
It's too soon to plan from phpunit10 since it's still in development. For all I know it'll work out of the box with what we have with phpunit9. Or maybe it won't work at all? Given that the world did not end when we didn't support phpunit 6, 7 and 8 it's probably safe to assume that skipping 10 wouldn't be too bad. Regardless we can worry about this much later.

maxime-rainville · 2021-09-26T23:30:35Z

The backwards compatibility polyfills also make it easier for existing projects to keep their unit tests as is for when they need to migrate to phpunit9. You could argue this is just being overly nice since they will have to do some work e.g. migrate off the docblocks for expected exceptions

If we do that, we're functionally taking over the deprecated APIs from PHPUnit and committing to supporting them. There's probably some other deprecated PHPUnit APIs that we're not using that some people out there might be using.

I would rather take the extra time needed to convert all our test coverage to the PHPUnit9 equivalent and then fully drop PHPUnit5 in Dec 2022.

It's too soon to plan from phpunit10 since it's still in development. For all I know it'll work out of the box with what we have with phpunit9. Or maybe it won't work at all? Given that the world did not end when we didn't support phpunit 6, 7 and 8 it's probably safe to assume that skipping 10 wouldn't be too bad. Regardless we can worry about this much later.

The "world did not end when we didn't support phpunit 6, 7 and 8" because we kept jumping through hoops to keep our PHPUnit fork working. I would rather avoid being in this position going forward.

"Much later" is probably going to be Feb 2022 so it's not that far away. My guess is that PHPUnit 9 will go in limited support when PHPUnit 10 is release.

I'm fine with taking the risk of "maybe not supporting PHPUnit 10" if we don't think that's essential. But if we think PHPUnit 10 is going to be essential, then the decision we make here could have lot of downstream effects and we should try to figure out what those will be.

emteknetnz · 2021-09-27T22:59:09Z

Max pointed out

https://github.com/Yoast/PHPUnit-Polyfills

Idea would be to add all the traits to the phpunit5 version of sapphiretest and remove my forwards compat polyfills.

We could probably also add wrapper functions in phpunit5 sapphire test with deprecation notices and just call parent::func(). that would help during project/module migration to identify which functions needs to be changed

maxime-rainville · 2021-11-15T20:24:05Z

We'll spin up the controller policy bit in a separate PR.

davejtoews · 2021-11-26T21:30:04Z

Some chance of it being problematic in the interim before we tag 4.10 as these modules CI will be installing 4.x-dev instead of a stable version, though probably a non-issue

Chiming in here to challenge "probably a non-issue". I was doing some maintenance and bumped a couple of modules up a minor version and suddenly found myself running bleeding edge, non-stable versions of SilverStripe core modules.

I caught the problem in two modules, but from the looks of this thread it's bigger than that.

Version 3.3.0 requires Silverstripe Framework version ^4.10 which doesn't exist symbiote/silverstripe-gridfieldextensions#328
Version 2.1.0 requires Silverstripe Framework version ^4.10 which doesn't exist #328 silverstripe-lumberjack#106

emteknetnz · 2021-11-28T20:28:33Z

Hey @davejtoews - could you check your composer.json file - we recommend the following:

"prefer-stable": true,
"minimum-stability: "dev"

silverstripe/framework 4.10.0 hasn't yet been released, so the new "silverstripe/framework": "^4.10" constraint should behave as follows:
"prefer-stable": true - should install ~~4.9.1~~ 4.x-dev
"prefer-stable": false - should install 4.x-dev

Let me know if this isn't behaving as expected

davejtoews · 2021-11-29T16:30:05Z

@emteknetnz I have exactly those settings for prefer-stable and minimum-stability. As far as composer is concerned the directive for ^4.10 is incompatible with 4.9.1. Running composer update silverstripe/framework:4.9.1 when I have a module installed that requires ^4.10 results in an error and composer being unable to resolve the dependency requirements. It only resolves ^4.10 to 4.x-dev

emteknetnz · 2021-12-02T03:20:32Z

@davejtoews Sorry about this happening. We've written an explanation of what went wrong and why here.

We recommend setting silverstripe/framework to ~4.9.0 until 4.10.0 has been released, or setting minimum-stability to stable

emteknetnz self-assigned this Jul 20, 2021

emteknetnz removed their assignment Jul 26, 2021

brynwhyman changed the title ~~Spike: Possible upgrade scenarios for PHPUnit 9+~~ Spike: Possible upgrade scenarios for PHPUnit 9+ (for PHP 8 support) Aug 30, 2021

brynwhyman assigned maxime-rainville Sep 19, 2021

brynwhyman assigned emteknetnz and unassigned maxime-rainville Sep 22, 2021

emteknetnz assigned maxime-rainville and unassigned emteknetnz Sep 22, 2021

maxime-rainville mentioned this issue Sep 28, 2021

RFC Adoption of an official PHP release support policy #10102

Closed

1 task

maxime-rainville removed their assignment Nov 3, 2021

emteknetnz assigned maxime-rainville and unassigned emteknetnz Nov 7, 2021

This was referenced Nov 10, 2021

API phpunit 9 support silverstripe/recipe-solr-search#2

Merged

API phpunit 9 support silverstripe/silverstripe-security-extensions#37

Merged

maxime-rainville closed this as completed Nov 15, 2021

maxime-rainville mentioned this issue Nov 28, 2021

Does it support PHP 8.0.12? silverstripe/silverstripe-installer#311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPUnit 9+ (for PHP 8 support) #10019

PHPUnit 9+ (for PHP 8 support) #10019

brynwhyman commented Jul 11, 2021 •

edited by maxime-rainville

Loading

emteknetnz commented Jul 26, 2021 •

edited

Loading

emteknetnz commented Jul 26, 2021

emteknetnz commented Jul 26, 2021 •

edited

Loading

maxime-rainville commented Sep 21, 2021

emteknetnz commented Sep 21, 2021 •

edited

Loading

maxime-rainville commented Sep 21, 2021

emteknetnz commented Sep 21, 2021 •

edited

Loading

maxime-rainville commented Sep 21, 2021 •

edited

Loading

emteknetnz commented Sep 21, 2021 •

edited

Loading

emteknetnz commented Sep 21, 2021 •

edited

Loading

maxime-rainville commented Sep 21, 2021 •

edited

Loading

emteknetnz commented Sep 21, 2021 •

edited

Loading

maxime-rainville commented Sep 26, 2021

emteknetnz commented Sep 26, 2021 •

edited

Loading

maxime-rainville commented Sep 26, 2021

emteknetnz commented Sep 27, 2021

maxime-rainville commented Nov 15, 2021

davejtoews commented Nov 26, 2021

emteknetnz commented Nov 28, 2021 •

edited

Loading

davejtoews commented Nov 29, 2021

emteknetnz commented Dec 2, 2021

PHPUnit 9+ (for PHP 8 support) #10019

PHPUnit 9+ (for PHP 8 support) #10019

Comments

brynwhyman commented Jul 11, 2021 • edited by maxime-rainville Loading

Overview

Tasks

Notes

PRs

emteknetnz commented Jul 26, 2021 • edited Loading

emteknetnz commented Jul 26, 2021

emteknetnz commented Jul 26, 2021 • edited Loading

Modules:

Projects:

maxime-rainville commented Sep 21, 2021

emteknetnz commented Sep 21, 2021 • edited Loading

maxime-rainville commented Sep 21, 2021

silverstripe/dev

emteknetnz commented Sep 21, 2021 • edited Loading

maxime-rainville commented Sep 21, 2021 • edited Loading

emteknetnz commented Sep 21, 2021 • edited Loading

emteknetnz commented Sep 21, 2021 • edited Loading

Background

Options

A. Two different versions SapphireTest/FunctionalTest in the framework codebase supported phpunit5 and phpunit9 respectively. Use the correct version depending on the version of phpunit installed**

B. Make php7.3 the minimum required for 4.10 - Make a semver breaking change and only include the phpunit9 SapphireTest/FunctionalTest in the codebase.

C. Make php8 the minimum required for 4.10 - Make a semver breaking change and only include the phpunit9 SapphireTest/FunctionalTest in the codebase.

maxime-rainville commented Sep 21, 2021 • edited Loading

Option A

Option B

Option C

emteknetnz commented Sep 21, 2021 • edited Loading

maxime-rainville commented Sep 26, 2021

emteknetnz commented Sep 26, 2021 • edited Loading

maxime-rainville commented Sep 26, 2021

emteknetnz commented Sep 27, 2021

maxime-rainville commented Nov 15, 2021

davejtoews commented Nov 26, 2021

emteknetnz commented Nov 28, 2021 • edited Loading

davejtoews commented Nov 29, 2021

emteknetnz commented Dec 2, 2021

brynwhyman commented Jul 11, 2021 •

edited by maxime-rainville

Loading

emteknetnz commented Jul 26, 2021 •

edited

Loading

emteknetnz commented Jul 26, 2021 •

edited

Loading

emteknetnz commented Sep 21, 2021 •

edited

Loading

emteknetnz commented Sep 21, 2021 •

edited

Loading

maxime-rainville commented Sep 21, 2021 •

edited

Loading

emteknetnz commented Sep 21, 2021 •

edited

Loading

emteknetnz commented Sep 21, 2021 •

edited

Loading

maxime-rainville commented Sep 21, 2021 •

edited

Loading

emteknetnz commented Sep 21, 2021 •

edited

Loading

emteknetnz commented Sep 26, 2021 •

edited

Loading

emteknetnz commented Nov 28, 2021 •

edited

Loading