[D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods

So the deprecation here will help child implementations. But what about callers? There are callers in contrib that are doing $this->assertTrue($this->assertCustomThing($foo, $bar));

When we correct return values in this issue, those contrib tests will start failing abruptly, with no deprecation. Any ideas for how we could provide deprecation for the callers as well?

I suppose the question is do we want to make them all @internal, or do we just make them private?

Thought about this one for a while and decided I am +1 to full type declarations here. There are no real BC concerns - tests are considered internal and it is fairly unlikely someone is using most of these let alone extending them downstream. Adding type declarations to both arguments and return values sets a good example for the rest of core, and also might help to catch bugs earlier than we might otherwise spot. Therefore the upsides are much greater than the downsides and I think we should go ahead with this.

As regards @internal I guess it can't hurt, we are warning downstream users, and we don't provide BC for tests. If we really didn't want users to extend tests we would declare them final, and we don't do that - so they are free to at their own risk, and that includes using any custom assertions.

When we correct return values in this issue, those contrib tests will start failing abruptly, with no deprecation. Any ideas for how we could provide deprecation for the callers as well?

For random test helpers I think it's fine to break things, those should be considered internal.

For traits or base classes I think we should do some kind of minimal bc/fc effort, but doesn't seem like there are many options:

1. Only change the traits in Drupal 10 (gives time for a CR and rector rules to inform people).

or:

2. Deprecate the entire method and add a new one with no return value - the problem with this though is that it causes churn for callers that aren't relying on the return value, so arguably worse than not doing so.

However we can probably grep contrib for callers to see if we'll actually break anything first (again only for base classes/traits). Also if a method turns out to only have one or two usages in contrib, just pro-actively opening issues against those modules might be enough (i.e. make the grep return empty by the time we get to 9.3.0).

A few minor comments. Ran out of time while reviewing, will continue another time.

As far as I can see there is nothing left to do here, this is cleaning up some Simpletest cruft and bringing our PHP up to modern standards at the same time.

This no longer applies after the last three disruptive patches went in.

Tried a 3-way merge, but to no avail

both modified:   core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
	both modified:   core/modules/views/tests/src/Functional/Handler/HandlerTest.php

Left a question on the MR

I'll ask another committer their thoughts

Discussed this with @catch, @xjm and @alexpott and we agreed that because this isn't really a deprecation and more of a code-style concern that we'd remove the deprecation from the patch and just go with the rest of the patch.

When we get phpstan we can look to add a rule to core to ensure all assert* methods return void.

Can we get the changes in the listener removed - thanks!

This needs a reroll to apply cleanly, tried a 3-way merge too and no joy

Custom commands failed on the bot

Committed/pushed to 9.4.x and cherry-picked to 9.3.x, thanks!

Even though we're not enforcing it, published the change record anyway.

From #60:

tests are considered internal

Was/is this documented anywhere? And does this means that e.g. Drupal\Tests\BrowserTestBase or Drupal\KernelTests\KernelTestBase are internal 🤔?

@huzooka see https://www.drupal.org/about/core/policies/core-change-policies/drupal-8... - BTB and KTB are not internal, but mostly everything else is.