Determine if there are other TestTraits we can add return types to.

Added the number of times core uses the traits.

The juiciest targets in the list -- the REST stuff -- are actually also potentially the riskiest, because REST is actually tested heavily in in contrib in various extension modules, because Wim.

OTOH, CommentTestTrait is not that likely to get used; it was factored out of the legacy monstrosity that was descended from the original Comment module SimpleTests. (Which were also the very first automated tests of any kind written for core, so... yeah. (We had things to learn back then.) So that single method is probably fair game as a quick win.

All that said, before we start weighing this versus that, we should probably consider at a higher level whether we're actually concerned about the disruption of changing these signatures with a return typehint that matches the current behavior. It's disruptive if:

1. You use the trait, and
2. You override the method without it also already having a void return typehint on the signature.

If that happens, you get a nice clean fatal telling you that declaration of blah doesn't match blah and the line number.

So the questions are:

1. How many people are overriding these test trait methods ever?
2. Do we care if we give them fatals in their tests as part of reducing both the baseline and the scope of what has to be typehinted in fiddly ways in 📌 Introduce a @return-type-will-be-added annotation to indicate return type addition in next major; enforce with a PHPStan rule Active ?
3. If we sort of care, should we make the changes in a way that mitigates disruption, such as grouping them as beta targets (so no patch backport divergence to worry about nor bridging of multiple versions for contrib)?

Is this a dupe of 📌 [meta] Add return types to all test Traits Active ?

#4.3 (if we sort of care) - we can use @return instead and phpstan will stop complaining, at least while we have treatPhpDocTypesAsCertain. Then we can figure out when to actually make the change.

However, I'm pretty sure if you override a method from a trait then the trait method effectively does not exist, so you can do whatever you want with the signature and PHP won't care. See https://3v4l.org/kW8DR

However, I'm pretty sure if you override a method from a trait then the trait method effectively does not exist

TIL! Great point.

It does look like a dupe

I wonder who made the duplicate...

I think we move the files over here and close the other even though it's older, there is better info here.

Starting from the top, I opened 📌 Add return types to EntityDefinitionTestTrait Active . While there are only 11 usages of the trait, the trait has 30 functions in it, so it's a much bigger cleanup than CommentTestTrait with only 1 function.

Ranking the remaining from juiciest to least juicy:

CookieResourceTestTrait - 1746 deletions
AnonResourceTestTrait - 1224 deletions
BasicAuthResourceTestTrait - 1140 deletions
FieldUiTestTrait - 810 deletions
WorkspaceTestTrait - 312 deletions
CommentTestTrait - 270 deletions
ContentModerationTestTrait - 228 deletions
PathAliasTestTrait - 216 deletions
OEmbedTestTrait - 108 deletions

Everything else is less than 100 lines deleted. Any suggestions for how to split?

We need to address #4 and the RM review first. :)

But also:

However, I'm pretty sure if you override a method from a trait then the trait method effectively does not exist, so you can do whatever you want with the signature and PHP won't care. See https://3v4l.org/kW8DR

💡 Neat! But also wait, why did I get a different result? Was I a silly pumpkin using classes instead of traits when I meant to test traits? Clearly I was.

I tested it with void specifically and confirmed that PHP doesn't care. Double neat! Also weird!

Thus: RM review of disruption hereby completed 🪄 and #4 is addressed. Leaving the tag on for the scope discussion. Proceeding thereto:

The main factor for the reviewability of these isn't the number of deletions (now that we know that deleting and regenerating the baseline is way faster and presumably would be the same amount of time regardless of scope), nor the usages (because doing weird false-y things with a void return method is unsupported nonsense), but the number of actual methods under review. We simply need to open the methods and verify that they don't return anything under any circumstance. It sounds like we don't even need to look for anyone overriding those methods to see if overridden method call anything (which would normally be the next thing I would say), so we can focus only on the actual methods themselves. We also don't need to read the baseline itself since it's an auto-generated asset that's reviewed with CI and CLI tooling.

Since we need to review the whole methods to verify that they are, indeed, intended to be void, the line length of the methods is the factor we want to look at (so far fewer changes per MR than when we were bulk-adding the typehints to actual test methods where returning anything woudl be illogical. We should target logically grouped contexts that give us roughly 100-200 LOC of methods that are updated. All of it is one pattern, so there is no need to separate it up by different types of changes; therefore, we can group it by subsystem or logical groups of subsystems, since the code style is more likely to be internally consistent.

If I remember that comment trait, it's probably that long by itself. The REST and JSONAPI methods together probably form a group of about the right size, although I didn't actually check. It doesn't need to be exact, but scan the traits in given subsystems or logical groups of subsystems and use your judgement therefrom. :) We'll try the first few issues like that, and then we can decide if we want to adjust.

OK, now actually removing the tag. 🪄 🌈

@mstrelan, your 3v4l of trait overrides led to core committers posting extra punctuation, e.g.:

So we can add return type hints to every trait in core whenever? (!?!?!)

I closed 📌 [meta] Add return types to all test Traits Active as a duplicate since this issue has more detail now. Attaching the assets from that issue here.