[meta] Add return types to test 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 IIRC the very first automated tests of any kind written for core well before even the release of Drupal 7, 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 #3486376: Extend Symfony DebugClassLoader to report missing cross-module @return types?
    

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)?

For context, the documented API policy is:

Test traits and abstract base classes should generally use deprecation where possible rather than breaking backwards compatibility, but they are still considered an internal API and may change if necessary.

However, experience has borne out that a clean break to test APIs is actually often better for contrib than a deprecation workflow (and then there's the whole thing about typehinting not even having any normal deprecation workflow on top of that).

Is this a dupe of #3487027: [meta] Add return types to all test Traits?

#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

Starting from the top, I opened #3529714: Add return types to EntityDefinitionTestTrait. 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 (and moreover weren't supposed to, which static analysis can't tell us). It sounds like we don't even need to look for anyone overriding those methods to see if the overridden methods 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 to put more traits into each MR because it's less work to review than I'm thinking.

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 #3487027: [meta] Add return types to all test Traits 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 #3529714: Add return types to EntityDefinitionTestTrait 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.

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

@nicxvan, I disagree because this then becomes a way of encoding broken/undocumented behavior -- static analysis enforcing existing mistakes -- but I will think on your feedback. :) Maybe it's better for static analysis to enforce a handful of existing mistakes from a technical debt perspective.

Part of my responsibility is to push back on these bulk changes when people only review the lines in the context of the diff or scan if they have implication in a wider context, which the larger typehinting problem at least does.

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

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. Need to think about the extent to which we care about that since it's probably uncommon.

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 #3530276: Add return types to CookieResourceTestTrait 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 think maybe just have a release manager's love of discovering major bugs 🔎🐞 so passing up an opportunity to do that in what is essentially a static analysis audit takes conscious effort. 🙃 (Edit: It's also definitely a thing in bulk standards update issues in general that people often don't properly review context lines/the method content/the callstack/etc. so we get very used to pushing back on that.)

Removing over-exuberant claims from title :)

Opened #3541927: Add return types to ContentModerationTestTrait which is ready to go. Maybe it could have been bigger in scope, but it's ready now.

Added the three biggest child issues that I could find, removing 426 entries from the baseline. These all seem uncontroversial and I don't think anyone will be overriding them.

Added #3579915: Add return types to AnonResourceTestTrait to resolve another 196 violations

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

Grouping of Remaining Traits

• 1. Migration & Data-related traits
  • StubTestTrait (9)
  • FileMigrationTestTrait (6)
  • NodeMigrateTypeTestTrait (5)
  • MigrateMenuLinkTestTrait (4)
  • ValidateMigrationStateTestTrait (2)
  • FieldDiscoveryTestTrait (2)
• 2. Functional & module-specific traits
  • TaxonomyTestTrait (23)
  • CKEditor5TestTrait (12)
  • EntityDefinitionTestTrait (11)
  • TaxonomyTranslationTestTrait (3)
  • BasicAuthTestTrait (2)
  • ResourceResponseTestTrait (1)
• 3. Core / generic / infra traits (bulk)
  • InternalTypedDataTestTrait (2)
  • HandlerTestTrait (2)
  • SortableTestTrait (2)
  • GroupIncludesTestTrait (2)
  • ConfigTestTrait (2)
  • TestTrait (1)
  • SessionTestTrait (1)
  • SchemaIntrospectionTestTrait (1)
  • PerformanceTestTrait (1)