Add void return typehints to protected test helper methods

Have included private methods here too.

Was a larger MR but appears all changes have been done to the protected functions.

Was expected a 1 to 1 for additions to subtractions but see some phpstan things were fixed in the process. Not super clear how adding void to doTestGrouping() fixed those but the checks are all green so won't think too much into it.

LGTM

Was expected a 1 to 1 for additions to subtractions but see some phpstan things were fixed in the process. Not super clear how adding void to doTestGrouping() fixed those but the checks are all green so won't think too much into it.

Most of the phpstan issues were due to rector adding a return to the parent method but not the child method, likely due to the order they were processed. So phpstan would complain that they are not covariant, which adding the type fixed.

The other issues were when a caller attempted to use the return value of something that returns void, for example, $this->assertNull($this->somethingVoid()).

Annoyingly as this didn't make 11.0.0-rc1 we can't just commit this as-is. Discussed with @xjm and although we only provide best-effort BC in tests there are some base classes and common traits touched here that I think we should avoid changing except in a major release, ie they will have to wait for 12.x now.

Some files that I think we should revert here are:

core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
core/modules/rest/tests/src/Functional/ResourceTestBase.php
core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php
core/tests/Drupal/FunctionalTests/Installer/InstallerTestBase.php
core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
core/tests/Drupal/KernelTests/AssertContentTrait.php
core/tests/Drupal/KernelTests/KernelTestBase.php
core/tests/Drupal/Tests/BrowserTestBase.php
core/tests/Drupal/Tests/UiHelperTrait.php

These are just the ones that I know are widely used as base classes/traits, there may be more, although there's also a tradeoff in considering which methods are really likely to be overridden in subclasses.

I just mentioned to @longwave that IMO this actually isn't tied to RC at all -- we would want to backport as much of this as possible to 10.3 to cut down on merge conflicts. I apologize for not giving better/clearer advice to start! Hopefully it's just a matter of tweaking a filename matching pattern or two to exclude things called *Base.php or *Trait.php.

Oh also: We can discuss what to do with the leftover methods once the easy/non-disruptive ones are committed. E.g., breaking ResourceTestBase or MigrateTestBase causes pain for a small handful of contributors who are already very active enough to address it quickly, but breaking BTB methods themselves is probably not something we should do in 11.1. So that's the only example where the RC does make a difference, I think. But that's a few lines out of thousands here that we can clean up from the parent(s) once the bulk of the work is committed.

Oh FWIW regarding:

Was expected a 1 to 1 for additions to subtractions but see some phpstan things were fixed in the process. Not super clear how adding void to doTestGrouping() fixed those but the checks are all green so won't think too much into it.

If the only thing that's not adding : void is deleting lines from our PHPStan baseline, that's better than good. If there are other changes to actual code, though, that would be something to keep an eye on and document.

Re-ran the rector and decided to just git add core/*Test.php as that was easier than filtering out all the other paths, like traits, base classes, test modules, and random helpers in core/tests. That should address #6 and #7. We can discuss #8 in the parent.

The addition to subtraction is equal so confident in the use of :void.

I spot checked a few dozen and change appears to be as advertised.

Didn't run a grep but this seems like a good chunk and any stragglers I think could be handled later if needed.

The 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.

Rebased on HEAD. Since ✨ Enforce return types in all new methods and functions RTBC we also need to regenerate the phpstan baseline, so this is a nice reduction of 266KB.

Rebase seems good.

I noticed some changes to private and public methods in the MR, but they are all helper methods in individual tests - they were probably added in 2009 before we used protected and never updated, have seen several of those in the past. Rather than splitting them out let's just change the issue title. There's no useful concept of a public test method and adding the return type is no different for them anyway.

However this needs another rebase for the phpstan baseline.

rebased, rebaselined, re-rtbc

 442 files changed, 845 insertions(+), 5909 deletions(-)

Committed/pushed to 11.x, thanks!

catch → closed merge request !8510

Automatically closed - issue fixed for 2 weeks with no activity.