Child issue of #3529510: [meta] Add return types to test traits. Adding void return types to 3 functions removes 1746 lines from the baseline.
As per discussion on the parent issue we need to determine if this trait is used in a base class. If it is, then adding return types may result in a BC break for child classes that override methods from the trait. There are 97 usages of use CookieResourceTestTrait in core, and they are all in files that end in Test.php and none are in files that end in TestBase.php. This is a good start.
We also need to look at whether returning void from the method is indeed the intended outcome:
initAuthentication are in the form of $this->initAuthentication();, there are no usages that assign the result to a variable or use the result in some way.assertResponseWhenMissingAuthentication
assertAuthenticationEdgeCases. The interesting thing to highlight here is the last line sets a value in the $request_options array that is neither passed by reference nor returned. This seems to be a mistake.
Furthermore, these 3 methods are documented in \Drupal\Tests\rest\Functional\ResourceTestBase and don't indicate that they should have a return value. Adding a return type declaration to the test base would be more likely to introduce a BC break.
The other place to look is contrib. This search seems to find usages while filtering out duplication of core and contrib modules - https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22use+C...
Note there is at least one abstract class there - https://git.drupalcode.org/project/drupal_admin_ui/-/blob/master/admin_u... but the child class in the same namespace doesn't override any of these methods, so again we're likely safe.
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 !12383
Comments
Comment #3
mstrelan commentedComment #4
smustgrave commentedMaybe the baseline needs to be generated again?
If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #5
mstrelan commentedI updated my deps and regenerated the baseline but it is now adding a number of unrelated deprecations to the baseline that are out of scope for this issue. Nevertheless, I think the issue can still be reviewed for the actual changes in the trait and the points made in the issue summary.
EDIT: Never mind, those deprecations had been added recently in #3497431: Deprecate TestDiscovery test file scanning, use PHPUnit API instead. Have rebased to include those.
Comment #6
smustgrave commentedThanks! Reviewed the 3 functions in question and confirmed they don't return anything so no issue with :void
Comment #7
xjmAs per parent meta.
Comment #8
xjmI searched contrib for re-declarations of methods with the same names. All are declared without typehints:
So, next question: how many subclasses of LayoutRestTestBase are there? There appears to be... ...one test?
Let's look then at the two projects affected by the two assertion methods.
Layout Builder Symmetric Translations has no stable release, is already failing its own pipelines, and has 3K users. (It is also maintained by people who will forgive me if I potentially break something, because I've broken much worse and so have they.) 😀
REST Menu Items does have a stable release, but has fewer than 2k users and is also already failing its own pipelines.
In the case of both modules, the assertions are overridden to be no-ops.
Given the limited scope of these disruptions, and given that they are documented as internal API already, I think the change is merited.
Futhermore, when these tests break, they'll break with a nice clean fatal. So hopefully it should be easy for contrib to fix. (I think.)
So. What I have taken from this exercise is: Overriding test trait methods in subclasses in a way that would fatal does happen in limited fashion, so an issue that bulk-hints
voidmight cumulatively break many things. That means that if we do a bulk patch to hint a majority of the stuff at once, it should probably be announced in advance, perhaps as a scheduled beta target.The response to this issue landing, if any, will also give us more information. (Even by its absence.)
Actual review of the issue itself anon.
Comment #10
xjmThree methods. One code review. No return values.
Fifteen minutes later.... Committed to 11.x (with icepacks for my laptop of course) and.... sooner or later will land in 11.2.x but I kinda want to post this comment before midnight. Thanks!
Comment #13
xjmPhew!