Fix phpstan errors in UpdatePathTestTrait

Get a load of that diffstat! 😍😍😍

xjm → credited cmlara → .

xjm → credited nicxvan → .

Gorgeous!

Adding credits for this Slack discussion also, since I took it into account when considering the issue scope:

nicxvan
Today at 6:53 PM
Wow! (edited)
6:54
Are there other traits like that? Or are the rest not in tests

acbramley
1 hour ago
there are almost definitely other traits without return types, I've fixed one other recently in the same issue I found the problem in. The issue is that phpstan reports the errors in the file that imports the trait, not the trait itself so adding a trait that has no return types into a new test means adding to the baseline :weary: (edited)

nicxvan
40 minutes ago
Well the original trait must have the error in baseline too, right?
7:13
I'll search later

cmlara
12 minutes ago
IIRC traits are scanned in the context of the files that use them, not independently. (A trait can not be independently executed unlike a class, and thus must be scanned in a full file.)
That said a phpstan run (not a baseline) does output that it is from a trait IIRC so one could identify them easily enough.

While I am quite sure there are probably other traits in core missing their void return typehint, the sheer scope of the baseline reduction here is enough for me to commit this regardless. Saving issue credits.

Committed to 11.x and 11.2.x. Hooray!

We'll want a 10.5.x version of this also, so setting PTBP. Thanks!

I should note that, technically, test traits are considered API, and changing the function signature to add a return typehint (even a void return typehint) is technically a BC break.

However, I stil think this is an acceptable change for the following reasons:

1. Writing upgrade path tests is probably very, very uncommon outside the core issue queue. (Heck, it's uncommon even in the core issue queue.)
2. For someone to actually be disrupted by this, I think they'd have to be overriding the trait methods rather than merely calling them, or maybe treating them as some false-ish non-null value. I'd be very surprised if anyone ever does the former, and I wouldn't even support the latter.
3. The reduction in technical debt is significant for a small, very theoretical disruption.
4. The change is being made only in minors during the beta phase, and it's actually an ideal sort of beta target issue.

All that said, we shouldn't use this as precedent to commit other return typehint additions to test traits without RM signoff. Test traits were excluded from the scope of 🌱 [meta] Add return typehinting to the test codebase Active on purpose. So if/when we file followups to add the appropriate void typehints to any test traits, we should make sure that the BC policy and potential disruption are given due consideration before spending 10 min at a time generating new PHPStan baselines and such. :D

📌 Determine if there are other TestTraits we can add return types to. Active is the followup (thanks @nicxvan!).

Oops.

acbramley → closed merge request !12350

These missing return types don't flag errors currently on 10.5.x so I don't know if it's even worth bothering. I guess if we eventually bump phpstan levels on 10.5 then it would be but that seems unlikely?

Yeah that's a much smaller diff!

I've reviewed it, I'll mark it if it passes and committers can decide to close this or merge it.

Ahh interesting. So it's not as compelling a 10.5.x backport, BUT given:

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

That means it's as non-disruptive as a baby bunny in its nest! So, might as well backport for, you know, parity.

d.o, y u uncredit commmitter? Saving. No wonder my clients are making sad faces at me.

All done!

xjm → closed merge request !12362

Automatically closed - issue fixed for 2 weeks with no activity.