Ensure correct signature for setUp(), tearDown(), setUpBeforeClass(), and assertPostConditions() methods

Hi, these are all BASE test classes and were left out on purpose in #3107732: Add return typehints to setUp/tearDown methods in concrete test classes. AFAICS we will be converting these classes too in Drupal 10

Hello @dan2k3k4,

Thanks a lot for the MR. I had been able to apply the patch from the MR on Drupal 9.1.0. Rector working again! Thanks!

Thanks for the patch. Moving back to needs work based on:

1) Was able to apply the patch cleanly to 9.2.

https://git.drupalcode.org/project/drupal/-/merge_requests/28.diff

2) After applying patch, all setUp and tearDown methods include void.

3) There were some setUpBeforeClass and assertPostConditions methods changed as well.

-  public static function setUpBeforeClass() {
+  public static function setUpBeforeClass(): void {

-  protected function assertPostConditions() {
+  protected function assertPostConditions(): void {

4) Based on 3, there are two methods that still need adjusting:

./core/tests/Drupal/Tests/BrowserTestBase.php:  public static function setUpBeforeClass() {
./core/tests/Drupal/Tests/UnitTestCase.php:  public static function setUpBeforeClass() {

5) The title and issue summary also need adjusting to match the new scope. I took a stab at a new title but it might be too specific.

The current state of this issue is little bit confusing.
This issue belongs to Drupal core version 10.0.x.dev.
MR28 Request to merge issue:3182103-browsertestbasesetup-must-be into 9.0.x

In "drupal/core:9.2.3" the \Drupal\Tests\UnitTestCase::setUpBeforeClass still throws an error (Attached patch fixes this problem).
Patch from MR28 is not applicable to 9.2.x.

When we'll do this, we also need to remove the BC layer.

A few methods were missing the typehint.

The changes in #11 to core/tests/Drupal/TestSite/TestSiteInstallTestScript.php and core/tests/Drupal/TestSite/TestSiteMultilingualInstallTestScript.php are wrong.

These classes do NOT extend either BrowserTestBase or KernelTestBase.

Instead, these classes implement \Drupal\TestSite\TestSetupInterface, which defines a (lowercase) public function setup(); method with no capital 'U' and no void return type hint.

I would also argue that as long as we're fixing the signature of setUp(), let's do it right and finish the job by making all instances protected instead of public. The test base classes BrowserTestBase and KernelTestBase both define this function as protected function setUp(): void;, and classes which extend these bases should not be redefining the visibility. I did not make this change in the patch - waiting to hear if anyone else considers this in-scope. (There are 98 uses of public function setUp() and 4 uses of public function tearDown() that should be corrected to be protected.)

BTW, it's really discouraging to fix this stuff time and time again only to have it pop back up and be pushed off to future versions of core. Here are just a few of the issues that "fixed" this: #2322889: Various setUp() and tearDown() methods are not protected, #2642236: Various setUp() and tearDown() methods are not protected (the sequel), #2858646: Methods called with incorrect case.

The signature of these methods was frequently wrong in core Drupal 7 and in core Drupal 8, and now the signatures are wrong again in Drupal 9. Contrib has been copying these core mistakes for more than 10 years, serving as a bad example for people who contribute core patches as well. Contrib can't use the correct "protected" visibility until core gets its act together and fixes core, and it's a real pain to maintain a mixture of public/protected in contrib simply to keep the tests from failing.

It would be really great if we fixed this in Drupal 9 instead of putting it off until Drupal 10. Breaking changes have been made for other reasons in D9, but unlike most of those other breaking changes this one (changing signatures on test functions) only affects tests. And in fact this isn't even a breaking change IMO because changing public to protected in core doesn't break contrib tests that use a public visibility. Likewise, adding :void in contrib is a minimal change that shouldn't inconvenience anyone for more than a few minutes.

as we're fixing the signature of setUp(), let's do it right and finish the job by making all instances protected instead of public

+1. PHP allows to change a protected method to public in extending classes, https://3v4l.org/JFZgY (not viceversa, https://3v4l.org/qqTJS). So it is up to us to ensure consistent visibility signature. We could also implement a check in Drupal\Tests\Listeners\DrupalListener::startTest() similarly to what's being done already for the $modules property.

EDIT - FWIW, I think the use of public visibility is a leftover of Simpletest times.

Fun fact: setUp() was changed from function setUp() (public by default) to protected function setUp() in Drupal 7 by:

commit c2f26d3368d38ce4d73aa0089d09d2d78ccf9285
Author: Dries Buytaert <dries@buytaert.net>
Date:   Thu Dec 10 15:39:43 2009 +0000

    - Patch #653940 by sun: tests weren't reporting all errors.

This was later even backported to the D6 contributed Simpletest module for consistency with core D7 ... so this has been broken for a really long time.

Here's patch #12 with the additional change of public -> protected for all setUp() and tearDown() methods that override \PHPUnit\Framework\TestCase, \Drupal\Tests\UnitTestCase, \Drupal\KernelTests\KernelTestBase, or \Drupal\Tests\BrowserTestBase.

Removed the 400+ lines of code diff from the issue summary. That's not helpful to understanding the issue and it was out-of-date.

I've read through #3107732: Add return typehints to setUp/tearDown methods in concrete test classes and the sense I get is that the big concern was BC compatibility with D8, which is why the base classes were excluded from that patch.

When D8 officially becomes unsupported a few weeks from now (2 Nov 21), this will no longer be a concern. PHP 7.3 will then be the minimum supported version of PHP so return type hinting will be allowed for all supported versions of PHP, unlike with D8.

Likewise, we use PHPUnit 9 in all supported versions of Drupal 9 (D9.1 is the minimum-supported version at this time). The void return type hint was added in PHPUnit 8.

We have a BC layer that was added in Overridden test methods require void return type hints, so contrib has already been moving to :void over the past 18 months - this deprecation has been reported by all our standard tools since the BC layer went in.

I really don't see any need to put this off until D10.

The tests in #15 failed because of the change to core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Fixtures.php.

Again, this class does NOT extend UnitTest, BrowserTestBase, or KernelTestBase. It is a utility class that defines its own tearDown() method - the signature on this method should not be changed (and indeed cannot be changed without breaking the tests ...). Here's a new patch.

Updated issue summary.

Likewise, we use PHPUnit 9 in all supported versions of Drupal 9

This is not quite true, for historical reasons we support both PHPUnit 8 and 9, and PHPUnit 8 is still used if you are on PHP 7.3 even in Drupal 9.3.

We have a BC layer that was added in Overridden test methods require void return type hints, so contrib has already been moving to :void over the past 18 months - this deprecation has been reported by all our standard tools since the BC layer went in.

I really don't see any need to put this off until D10.

We promised to contrib that we would not break this on them until D10, and we keep our BC promises elsewhere. The issue is that contrib may extend any of these test base classes, and if they haven't yet added typehints then their tests will break if we add typehints first.

            @trigger_error("Declaring ::$method without a void return typehint in " . get_class($test) . " is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724", E_USER_DEPRECATED);

To me this is currently working as designed and is a task that can be committed when 10.0.x opens.

#22, I agree.

NW because I think it would make sense to do #14 too

... it is up to us to ensure consistent visibility signature. We could also implement a check in Drupal\Tests\Listeners\DrupalListener::startTest() similarly to what's being done already for the $modules property.

Likewise, we use PHPUnit 9 in all supported versions of Drupal 9

This is not quite true,

@longwave What is the purpose of pointing out my inexact language in one small sentence of one comment? It's completely irrelevant to the argument and only serves to derail the issue. You also took this statement out of context. The full statement was:

Likewise, we use PHPUnit 9 in all supported versions of Drupal 9 (D9.1 is the minimum-supported version at this time). The void return type hint was added in PHPUnit 8.

Read that in full. I said the void return type hint was added in PHPUnit 8. It doesn't matter that we use PHPUnit 8 in some configurations and use PHPUnit 9 in some configurations - they both use the void return type hint.

Note that I was precise when I re-wrote the issue summary and quoted the change record as saying:

Drupal 9.1.x has updated to PHPUnit 9. Installs on PHP 7.3 will continue to use PHPUnit 8.4 for compatibility reasons.

I also cited the PHPUnit change record for PHPUnit 8.

And while it would have been better if I had written:

Likewise, we use PHPUnit 8+ in all supported versions of Drupal 9

THAT DOES NOT CHANGE THE REASONING in any of my posts, because the void return type was added in PHPUnit 8, which we use for ALL supported versions of Drupal 9.

So can we get back to the work of trying to fix core?

There are two parts of this issue:

1. Method visibility
2. Return type hints

Regarding point 1: There's absolutely no reason that anyone has articulated that would prevent us from fixing the method visibilities right now, to agree with the published Drupal API that has been in effect for more than 10 years. And we already fixed this for the $modules property. Not a single reason to delay this any longer.

Regarding point 2: The "promise" that was made in the change record Overridden test methods require void return type hints was that the backwards compatibility shim would persist until Drupal 10. If you want to keep this "promise" then simply add back the shim that was removed from the patch by @mondrake in #10. No reason we can't keep the shim, if that is your concern. I have added that back into the patch.

Note we have already added void return type hints for UnitTestCase, KernelTestBase, and BrowserTestBase - these have been in Drupal core since April 2020. There is no good reason to delay enforcing these in core - enforcing it now will make the transition for contrib easier (and the change need in contrib is already pretty trivial). Right now core is making it harder for contrib because core forces contrib to have all sorts of special-case situations, due to core not following its own rules. Remember the entity manager? Core changed that pre 8.0 yet core continued to use it until 8.8, causing all sorts of cascading problems that still arise even today. The way to avoid those situations is for core to live by its own rules - when core changes the signature of a method, then we must insist that ALL of core changes so that the legacy examples won't persist and be copied into contrib and back into core etc. It's the double standard that causes problems for contrib, not necessarily the change itself.

Remember, we're talking about test cases and only test cases here. Even in the worst case scenario this won't break any code, and might break a test case only temporarily. Worst case. If you've kept up maintenance of your contrib module there will be no effect at all. The changes needed in contrib in any case will be extremely small - certainly less that the current disruption of having to maintain both public and protected for the same methods, certainly less than having to use :void for the methods in some places and not others. By delaying this you're forcing contrib maintainers to deal with the disruption twice, spread out over two years, rather than just fixing it and getting on with our lives.

I'm speaking from the point of view of a maintainer of contrib projects when I say that you're not doing me a favor by delaying this, you're just making it harder to maintain my projects.

@mondrake: I won't go switching the version again, but I would appreciate if you would reconsider. At a minimum, let's reduce the scope of this issue and at least fix the method visibility in D9 - don't put that off yet again. That's been broken and causing problems in contrib since D7.

Here's a new patch rerolled against current D9 HEAD, including the check in DrupalListener::startTest() like you suggested and with the BC shim for :void restored, as mentioned above.

Note we have already added void return type hints for UnitTestCase, KernelTestBase, and BrowserTestBase - these have been in Drupal core since April 2020.

There are no void return types in any of our test base classes yet (and in fact your patch adds these return types to these three base classes). PHPUnit 8/9 does declare void return types - but we patch this out at runtime, precisely so downstream users don't have to add the return types just yet. See Drupal\TestTools\PhpUnitCompatibility\PhpUnit8\ClassWriter::alterTestCase(). This allows us to trigger the deprecation in DrupalListener::startTest() without a fatal error before we even get to that point.

If you use contrib search at http://grep.xnddx.ru/search?text=function+setUp&filename= a random sample shows there are many modules that are D9 compatible but that have not yet added void return types to their setUp method. If we commit #25 now then these tests will no longer run: https://3v4l.org/uHi7Q

Right now core is making it harder for contrib because core forces contrib to have all sorts of special-case situations

Can you give an example of a special case that is needed here? All contrib and other downstream tests can add void return types to all methods right now, as the deprecation message states, so they can seamlessly upgrade from Drupal 9 to 10 later on. If there is a case where this is not possible I'd like to help solve it.

I do agree that the method visibilities can and should be fixed; I suggest that it should be done separately from adding the return types.

Rerolled #25 for 10.0.x.

+++ b/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit8/ClassWriter.php
@@ -83,11 +83,6 @@ private static function alterTestCase(ClassLoader $autoloader): void {
-    // While Drupal still allows methods in test base classes that inherit from
-    // TestCase with no void return typehints specified, we also alter TestCase
-    // to remove the typehints.
-    // @see https://www.drupal.org/project/drupal/issues/3182103
-    $alteredCode = preg_replace('/^    ((?:protected|public)(?: static)? function \w+\(\)): void/m', '    $1', $alteredCode);
     include static::flushAlteredCodeToFile('TestCase.php', $alteredCode);

I think we should also do this from #11 here to prevent accidental regression.

+++ b/core/tests/Drupal/Tests/Listeners/DrupalListener.php
@@ -106,6 +106,11 @@ public function startTest(Test $test): void {
+    // Check for incorrect visibility of setUp() and tearDown() methods.
+    if ($class->hasMethod('setUp') && !$class->getMethod('setUp')->isProtected() ||
+        $class->hasMethod('tearDown') && !$class->getMethod('tearDown')->isProtected()) {
+      @trigger_error('The ' . get_class($test) . '::setUp() and ::tearDown() methods must be declared protected. See https://www.drupal.org/node/2322889', E_USER_DEPRECATED);
+    }
   }

Is that all? Do we also need to check
- 'setUpBeforeClass',
- 'assertPreConditions',
- 'assertPostConditions',
- 'tearDownAfterClass',
- 'onNotSuccessfulTest',
? Also, isn't this something PHPCS or PHPStan would be better at?

Addressed #29. Also removed another chunk of code that checked for the void return methods, they all need to be void return now or the classes won't load in the first place.

> isn't this something PHPCS or PHPStan would be better at?

I realise we have the check for the $modules property immediately above this, but adding more checks is a slippery slope. - this definitely feels like a job for static analysis instead of runtime checks, so removed this for now. Also this doesn't technically cause any issues if a subclass has the wrong visibility, and it seems pretty unlikely someone will instantiate a different test class and try to call one of these methods from outside.

Try again, missed a line.

Looks good, RTBC. See also Slack conversation https://drupal.slack.com/archives/C1BMUQ9U6/p1639474615373900.

@mondrake asked in slack if this was eligible for alpha1 (which is strictly dependency updates with Symfony 5.4, no Drupal deprecation removals yet). Since this is removing a phpunit bc/fc compatibility layer and allowing us to 'properly' update to phpunit 9 I think it's in scope to commit pre-alpha. Although it's not as high a priority as hard blockers it's also been ready to go for quite some time.

Committed cfd8517 and pushed to 10.0.x. Thanks!

For those rare fellows in need of kinda “backporting” this patch to 9.3.x branch, here's a slightly modified one: as UpdateUploaderTestBase was introduced on the 10.0.x branch, therefore the following part blocks the patch from applying:

diff --git a/core/modules/update/tests/src/Functional/UpdateUploaderTestBase.php b/core/modules/update/tests/src/Functional/UpdateUploaderTestBase.php
index 972495444d..d803bb3f3d 100644
--- a/core/modules/update/tests/src/Functional/UpdateUploaderTestBase.php
+++ b/core/modules/update/tests/src/Functional/UpdateUploaderTestBase.php
@@ -12,7 +12,7 @@ class UpdateUploaderTestBase extends UpdateTestBase {
   /**
    * {@inheritdoc}
    */
-  protected function setUp() {
+  protected function setUp(): void {
     parent::setUp();
 
     // Change the root path which Update Manager uses to install and update

For people who wants to be able to apply patch on 9.4.0.

I took patch from comment 35, applied on 9.3.x, then cheery-pick on 9.4.x.

Rebased patch from comment 37 to be able to apply on core 9.4.8.

Patch for 9.5.x (2022-12-25 f385639b85d7b5eb719cbb79bc394534a67e3796)

Here's a patch made for the 9.5.3 release (From 2023-01-31 - 11:33pm 0e78230972c599be926cbb7baa5a77a2afa362a2).