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.

Oh, there was an implicit assumption in my comment in #11 that we are talking about void return typehints as per the original issue. We definitely should address things with void typehints first and then address other return types separately. Sorry for not being more clear!

The trait in 📌 Add return types to EntityDefinitionTestTrait Active was actually 500 lines long, but was manageable to scan because it was mostly simple logic with very similar operations to set things in state or perform data operations. So I think we can err on the side of a higher line count per issue (around 400 LOC) so long as there isn't a lot of complicated logic, especially if it's in an internally consistent subsystem.

It's also fairly foolproof, phpstan will complain if we get the return types wrong for a method. We just have to check any additions to baseline since we're regenerating.

@nicxvan, see what I said above about the intended behavior vs. documented/current behavior. :) I agree the risk is lower with static analysis but we should still actually read the code.

Re #12: on further thought, what if the trait is added to a parent class and contrib overrides it in a subclass. I think the subclass would still have to match the signature of (or be covariant with) the trait in the parent class. That could be an issue for example if you override drupalGet in a test and the signature changes in UiHelperTrait. Would need to confirm though.

My point was more that we should not limit this to 400 lines of methods for the following reasons.

1. The actual change is just one line, the type hints
2. Each method is independent and binary, check the method in isolation, if it returns void we are good
3. Gitlab tools make reviewing in multiple sessions easy, you can check individual files once you've reviews and they will remain closed unless something new changes.
4. We have static analysis that will catch any methods we add incorrect return types to
5. Large baseline changes are disruptive, it's far better to do once then 5 or 10 in a row that will all conflict with each other and create conflicts for most other issues touching baseline.

This issue is a prime candidate for a one time bulk update and would be easy to verify and review.

@nicxvan, I disagree because this then becomes a way of encoding broken/undocumented behavior, but I will think on your feedback. :)

In my book this isn't about fixing broken or undocumented behavior, it's about reducing baseline which is the cause of tons of conflicts. So if we found something wrong it would be out of scope to fix anyway.

We can create follow ups to evaluate the changed traits if we want to, but the truth is if they are broken now is not been noticed and adding a return type won't change that. These are test traits so we have more flexibility than most of core right?

We did these bulk updates for hook implementation return types with no problem*

* we did break those ones up by return type / hook groups but there were many, many implementations.

The only exception was hook help and that is because it is so variable that we need to manually change them, because of that we haven't even finished that one.

I think this case in particular is also mitigated because they are test traits.

In my book this isn't about fixing broken or undocumented behavior, it's about reducing baseline which is the cause of tons of conflicts. So if we found something wrong it would be out of scope to fix anyway.

Yes, but we would know not to put the typehint on it and to file a followup, which is my point. Vs. once it has the typehint, no one will ever look at it again and will assume that the missing return is design behavior.

These aren't theoretical things dancing on the head of the pin, BTW; the last round of adding @return to docs turned up actual broken code missing its return statements.

@nicxvan Hook implementations needing a return signature as such has only existed a few months, and that's one of those places (like the test methods) that returning something is illogical and unsupported. :) This is different because reused helper methods inside tests could theoretically be doing anything. We're talking here about code that has been moved from one place to another over a course of up to 15 years. :D

I should mention that I am still considering the "bulk update all the things" approach, BTW, and whether the benefits would outweigh the risks, at least within the scope of currently-according-to-static-analysis-void-return test traits. Even if we do that here, we might not want to use it as a precedent for other traits, though.

I plan to get feedback from the other RMs and FMs on that, but first, I think we need to look into #18. Can we confirm whether there's a fatal in that scenario? That will have impact on how we proceed.

I think we need to look into #18. Can we confirm whether there's a fatal in that scenario? That will have impact on how we proceed.

Yep it's a fatal - https://3v4l.org/8RLDn

Similarly if a class using a trait decides to implement their own interface that, for some reason, includes the trait methods, a fatal will be thrown too - https://3v4l.org/f43o4

Well that's unfortunate.

Helpful though. What we should immediately do next is sort these into what's used in test base classes (which are API) versus not. The ones that are not we can potentially take @nicxvan's approach with and change and backport liberally; the ones that are might require examining the specific methods.

We would still have a risk of cases where contrib defines its own base classes using the trait and then implements them

Unfortunately it's probably guaranteed that EntityDefinitionTestTrait was used in base classes somewhere, including almost certainly in the Entity API's own, which are API that actually gets used in contrib (unlike the upgrade path test base class). It's also a case theoretically you might want to override the methods if you e.g. were testing some sort of alternative storage perhaps. But I really don't want to revert that. 😅 Will look at it more closely again later. If someone wanted to check the usages of it in base classes prior to the commit and post that to the fixed issue for reference, that would be helpful.

I was keen to test some of the theories here and here's what I found:

1. Adding a void return type to a function that has a return statement does NOT throw an error with core's phpstan configuration
2. Adding a non-void (tested with int) return type to a function that has no return statements does throw an error with core's phpstan configurationm e.g Method Drupal\Tests\content_moderation\Kernel\ContentModerationStateTest::applyEntityUpdates() should return int but return statement is missing.

I am also interested in how we are meant to evaluate this statement:

We simply need to open the methods and verify that they don't return anything under any circumstance (and moreover weren't supposed to, which static analysis can't tell us)

Do we have any examples of things that don't return anything but were "supposed" to? This seems like it's going to add a huge overhead to reviewing this stuff.

Opened 📌 Add return types to CookieResourceTestTrait Active to practice evaluating the BC concerns

Do we have any examples of things that don't return anything but were "supposed" to? This seems like it's going to add a huge overhead to reviewing this stuff.

Sadly it's a number of years ago now and lost amidst the thousands of issues I comment on, but it's a number of major bugs that came out of it. It does add overhead, for sure. I'm also just concerned about the thing where an actual typehint is a much stronger implication that something is a design behavior.

The point about having to regenerate the baseline over and over for every child issue is a compelling reason that the effort might not actually be worth it, though, even if it ends up burying a major bug or two. So over the weekend I think I've come around to the view that (at least for the initial scope of void-return-typehint-test-traits-especially-not-used-in-core-base-classes, and then evaluate after based on how that goes) maybe the all-in-one-go approach is better. (But I will bring it to other committers for their input before we proceed.)

I just have a release manager's love of discovering major bugs 🔎🐞 so passing up an opportunity to do that takes conscious effort. 🙃

Removing over-exuberant claims from title :)