- Issue created by @mstrelan
- πΊπΈUnited States smustgrave
Maybe the baseline needs to be generated again?
If you are another contributor eager to jump in, please allow the previous poster(s) at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
- π¦πΊAustralia mstrelan
I updated my deps and regenerated the baseline but it is now adding a number of unrelated deprecations to the baseline that are out of scope for this issue. Nevertheless, I think the issue can still be reviewed for the actual changes in the trait and the points made in the issue summary.
- πΊπΈUnited States smustgrave
Thanks! Reviewed the 3 functions in question and confirmed they don't return anything so no issue with :void
- πΊπΈUnited States xjm
I searched contrib for re-declarations of methods with the same names. All are declared without typehints:
- assertResponseWhenMissingAuthentication() and assertAuthenticationEdgeCases*(): Two uses, one in a base class.
- initAuthentication(): False positive only.
So, next question: how many subclasses of LayoutRestTestBase are there? There appears to be... ...one test?
Layout Builder Symmetric Translations β has no stable release, is already failing its own pipelines, and has 3K users. (It is also maintained by people who will forgive me if I potentially break something, because I've broken much worse and so have they.) π
REST Menu Items β does have a stable release, but has fewer than 2k users and is also already failing its own pipelines.
In the case of both modules, the assertions are overridden to be no-ops.
Given the limited scope of these disruptions, and given that they are documented as internal API already, I think the change is merited.
Futhermore, when these tests break, they'll break with a nice clean fatal. So hopefully it should be easy for contrib to fix. (I think.)
So. What I have taken from this exercise is: Overriding test trait methods in subclasses in a way that would fatal does happen in limited fashion, so an issue that bulk-hints
void
might cumulatively break many things. That means that if we do a bulk patch to hint a majority of the stuff at once, it should probably be announced in advance, perhaps as a scheduled beta target.Actual review of the issue itself anon.
- πΊπΈUnited States xjm
Three traits. One code review. No return values.
Twenty minutes later.... Committed to 11.x and sooner or later will land in 11.2.x (with icepacks for my laptop of course). Thanks!