Since PHPUnit 10, process isolation must be specified on a single class level or as a global CLI/config switch.
This would cause issues because Drupal needs to run Unit tests NOT in isolation for performance reasons.
For that purpose, workarounds were introduced to programmatically set process isolation in the test constructor.
However, in PHPUnit 12 that is no longer possible as the test constructor can no longer be overridden, and trying to intervene in the setUp() method is already too late for process isolation.
The #[RunTestsInSeparateProcesses] attribute works but needs to be set on each concrete test class, it can not be specified on abstract base classes.
In this issue, introduce a check that throws a deprecation if the #[RunTestsInSeparateProcesses] attribute is not specified on the concrete Kernel test class.
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes, plain diff MR !13196
Comments
Comment #3
mondrakeComment #4
mondrakeNote that the deprecation for removal in Drupal 11.4 is on purpose.
The point here is that in order to bump PHPUnit to 12, this issue is a must (unless we get another solution from upstream, which on PHPUnit 11 that we're currently using it less and less likely).
So if we were to remove only in D12, this would mean that D12 would ship with PHPUnit 12 i.e. a version whose first release would be 16 to 22 months behind D12 release date. D12 will release when PHPUnit will be on version 13, with version 14 coming in Feb 2027.
I'm not saying we necessarily have to aim being current with PHPUnit release, just we need to be aware. Tagging for release manager review.
Comment #5
catchI'm not sure this is necessarily the case, we could theoretically continue running phpunit 12 on Drupal 11, but update to phpunit 13 in Drupal 12. To do that we'd need to communicate phpunit 13 deprecations where possible, but it seems feasible. I'll ping the other RMs for more opinions.
Either way we definitely need a change record here asap since it will become a requirement soon.
Comment #6
mondrakeAdded draft CR.
Comment #7
mondrakeComment #8
mondrakeWhat I also meant in #4 (it's not clear) is that if we want to allow PHPUnit 12 in Drupal 11, this issue is a must AND we need to throw an exception (not a deprecation) if the attribute is not added - that is why I suggest to add the deprecation here for D11.3, and change it to an exception in D11.4 (i.e. in a minor, not a major).
Comment #9
catch@mondrake yes I got that bit, but I think that might depend on whether we can jump to phpunit 13 in Drupal 12.0, without having phpunit 12 support in Drupal 11. Don't know what the answer to that is, but if it might be less disruptive than requiring all contrib and custom tests to add the attribute for a minor release.
Comment #10
mondrakeOK, time will tell :)
Comment #11
mondrakeComment #12
smustgrave commentedCame here from a related issue. But will say would prefer not getting an exception in contrib modules for a minor release.
Comment #13
mondrake#12 I understand the concern, but then we need to rule out the possibility to support PHPUnit 12 in Drupal 11 now before committing this, and adjust the deprecation accordingly.
To me, the worst possible option is ending up to silently running kernel and functional tests with no process isolation.
Comment #14
smustgrave commentedSo this may be a dumb question
But is there a major need to have php unit 12 in 11? Yea it would be nice to be up to date but is it a security reason?
If there is a reason does it outweigh the community disruption?
Comment #15
mondrakeComment #16
mondrakeComment #17
catchOnly the actual phpunit 12 update will throw the exception, so for me I think it's worth preparing core for that update as much as possible, and then we can make a final decision when we get there.
The main question is if we're able to jump to phpunit 13 in Drupal 12 without too much disruption. At the moment this will be the most disruptive change, but it's compatible with phpunit 11.
We could maybe reword the change record to be a bit more open about when an exception will start getting thrown, but I think it's good if people start adding the attributes asap.
Comment #18
mondrake#17 actually, PHPUnit 12 will not throw anything... simply, it will not run the test in isolation without the attribute having been set.
That's why I am proposing to convert, when time comes, the deprecation triggered here in the
setUpmethod into an exception - that would be Drupal's way of checking that the concrete test complies with the requirement. Alternatively, we could do that check in a static analysis rule, but that would not be guaranteed to be part of contrib checks: by adding it as an exception in the abstract base class there's no way around it.So, I think we agree to start throwing the deprecation now. Question is when we start throwing the deprecation instead. Do I understand we can leave that open for now?
I.e. a deprecation message like
'Kernel tests must specify #[RunTestsInSeparateProcesses], not doing so is deprecated in drupal:11.3.0 and is removed when PHPUnit 12 is supported. See https://www.drupal.org/node/3548485'would be it?Comment #19
catchSomething like that sounds good to me.
Comment #20
mondrakeOK then, lets do that.
Comment #21
mondrakeComment #22
smustgrave commentedSo that seems very vague trigger error. When phpunit12 lands? Is that D12 or are we going to randomly throw exceptions in 11?
Comment #23
mondrake#22 well, that's exactly what was discussed above
Comment #24
smustgrave commentedEh will leave to you guys that know better but that seems worst. Practically saying you could start throwing exceptions tomorrow
Comment #25
catchJust discussed this with @alexpott, @bircher and others at DrupalCon, and we think that we should only update to phpunit 12 in Drupal 12 - just because forcing every contrib module to update their tests in a minor release would be a massive break.
This may come back to bite us if there's another big incompatible change in a later phpunit version we need to update in Drupal 12, but it feels like this is that incompatible version.
Given that I think we could explicitly say in the deprecation message that we'll thrown an exception in Drupal 12. The Drupal 12 branch (or hopefully not an actual 12.x branch but
main) will open soon-ish, so we can do the actual phpunit12 update in there asap once it is.Comment #26
mondrakeI think #25 accounts for a release manager review. Fully agree, readjusting the deprecation message right now.
Comment #27
smustgrave commentedAwesome! LGTM
Comment #28
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #29
mondrakeBot????? It looks like the bot is posting based on old info
Comment #31
catchCommitted/pushed to 11.x, thanks!
Comment #35
mondrakeThis is fixed, right?
Comment #38
alexpottFYI #3556315: Current approach to ensuring RunTestsInSeparateProcesses is added does not work