Problem/Motivation
#3497431: Deprecate TestDiscovery test file scanning, use PHPUnit API instead added some new deprecations to the baseline. These were deprecated in https://github.com/sebastianbergmann/phpunit/issues/6052.
Proposed resolution
Replace Assert::isType calls with PHPUnit 11+ specific type assertions.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3530453
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3530453-remove-deprecated-use
changes, plain diff MR !12389
Comments
Comment #3
mstrelan commentedComment #4
mondrakeI’ve closed #3497080: Assert::isType() is deprecated that is now a duplicate.
The only question I have here is whether we should wait for PHPUnit 10 support to be removed, or add PHPUnit 10 polyfills for the new methods. PHPUnit 10 lacks them and we might still have someone testing locally core with it ATM.
Comment #5
mstrelan commentedOh I missed that one. A polyfill makes sense, but I wonder if it's worth it for 8 tests. Do we do still do test runs of 11.x with PHPUnit 10 or is it only 11 now? Do we need to keep BC for people running it in other environments?
Comment #6
mondrakeCore in GitLabCI is now consistently on PHPUnit 11 (has been so for just a couple of days though, very fresh), but we can’t say for contrib or people running core tests locally or via other CI tools.
Comment #7
mondrakeComment #8
smustgrave commentedSeems to be just 4 more instances inside ParamConverterManagerTest
Comment #9
mstrelan commentedUpdated ParamConverterManagerTest and rebased. Not sure who is best to answer #4.
Comment #10
mondrakeNo more isType calls reported in the baseline. RTBC
Let’s see what core committers think of #4.
Now core is set on testing with PHPUnit 11 by default, so not sure it’s worth the effort.
Comment #11
mondrakeComment #12
xjmRe: #4: It's been suggested that it was incorrect of us to drop support for PHPUnit 10 in 11.2, and that we should re-broaden core's constraint to be
^10 || ^11so as to be less disruptive to folks' CI. We also discussed changing one of the daily or weekly core environments back to testing on PHPUnit 10 for that (because if we don't run the test suite on PHPUnit 10 occasionally then the lower minimum will be a lie). That said, even if we do both those things, I'd still lean toward "no polyfill" given the low number of usages.Comment #13
xjmCorrecting my comment from #12: The core constraint is still
^10 || ^11; it was dropping support forsebastian/diff5 that accidentally made PHPUnit 11 the de facto core requirement. But same conclusion. 🙂Comment #14
xjmTechnically the PHPUnit 10 support question is a release manager review thing, so getting a second opinion.
Comment #15
xjmDiscussed with @catch. We are okay with no polyfill. However, that would mean officially dropping support for running the core test suite locally on PHPUnit 10 (currently impossible anyway due to the
sebastian/diffconstraint, but ideally that will be fixed before the Aug. patch release window). So, we will need a CR and a release note.Comment #17
xjmComment #18
mondrakeComment #19
mondrakeComment #20
mondrakeI think the CR of #3537104: Remove support for PHPUnit 10 is enough. Not sure about a release note specifically for this issue, but leaving the tag.
Comment #21
mondrakeComment #22
smustgrave commentedYea not sure either about the release note but does appear the instances have been replaced. Think it would be good to get into 11.3
Comment #23
longwaveYeah we don't need a separate release note for this now we explicitly dropped support for PHPUnit 10. Good to keep chipping away at the baseline though!
Committed and pushed 35283e39ca0 to 11.x and 033a864f1c3 to 11.3.x. Thanks!