Change 757400 had a related patch set uploaded (by Ammarpad; author: Ammarpad):

[mediawiki/skins/Timeless@master] Do not duplicate categories in primary action tabs space

https://gerrit.wikimedia.org/r/757400

Change 757400 merged by jenkins-bot:

[mediawiki/skins/Timeless@master] Do not duplicate categories in primary action tabs space

https://gerrit.wikimedia.org/r/757400

It may be technically a core breaking change, but I think in this case it's mostly Timeless's fault for sticking the contents of unknown portlets into the area it also uses for page actions. And, I mean, it literally has this comment next to the code that does this: :D

		// This is really dumb, and you're an idiot for doing it this way.
		// Obviously if you're not the idiot who did this, I don't mean you.
		foreach ( $pileOfEditTools as $navKey => $navBlock ) {

Other skins I checked just ignore the new categories portlet.

Someone probably should have tested that change with WMF-deployed skins though.

In T300100#7652691, @matmarex wrote:

Someone probably should have tested that change with WMF-deployed skins though.

Perhaps, but Timeless isn't a supported skin. This is exactly what we mean by 'not supported'. I'd suggest that this shouldn't be considered a train blocker.

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/754586 which was an unannounced breaking change,

That's because https://gerrit.wikimedia.org/r/c/mediawiki/core/+/754586 was not a breaking change.

Skins should not be blindly rendering $this->data['content_navigation'] or its counterpart Skin::buildContentNavigationUrls. Doing so before this change would have resulted in multiple user menus, mulitiple Echo icons and all sorts of other problems. Note the Skin::buildContentNavigationUrls method is not @stable and should likely be made private in a future version. We have skin helper methods that skins should be using instead.

In this particular case as @matmarex has pointed out, Timeless was knowingly hacking around this behaviour. A unit test should likely be added when using such hacks to avoid this sort of thing happening.

I'd suggest that this shouldn't be considered a train blocker.

It doesn't meet the criteria of https://wikitech.wikimedia.org/wiki/Deployments/Holding_the_train#Issues_that_hold_the_train so I agree. THat doesn't mean it can't be backported ASAP though especially given a patch exists.

Change 757470 had a related patch set uploaded (by Bartosz Dziewoński; author: Ammarpad):

[mediawiki/skins/Timeless@wmf/1.38.0-wmf.19] Do not duplicate categories in primary action tabs space

https://gerrit.wikimedia.org/r/757470

Change 757470 merged by jenkins-bot:

[mediawiki/skins/Timeless@wmf/1.38.0-wmf.19] Do not duplicate categories in primary action tabs space

https://gerrit.wikimedia.org/r/757470

In T300100#7653457, @Jdlrobson wrote:

Note the Skin::buildContentNavigationUrls method is not @stable […]

The Skin class is by its very nature designed to be subclassed and marked "stable to extend" in the class doc. That annotation carries meaning. The Stable interface policy states that stable to call includes:

• public methods, unless otherwise stated.
• protected methods of classes designed to be subclassed (marked "stable to extend").

I'm not leaving room for interpretation here because I co-wrote and approved the policy in question.

In the decade before the 2019 policy, it was also generally understood by those with maintainer responsibilities, that it stands to reason that developers would not have made this method protected if it wasn't meant to be called by individual Skins as otherwise it would be indistinguishable from a private method, or it would have been marked as @internal if it was only exposed for a a special use case within a core subclass. As such, most of the policy was the result of gathering and summarising the status quo among core maintainers for ease of reference and on-boarding new engineers, not as a new practice.

Note that Timeless was developed and deployed with peer review, as is our standard practice, and would not have been deployed had it been using internal core methods (we do ocasionally accept tech debt in the form of internal method calls, but I'm not aware of Timeless being of an urgency where we would have accepted that).

If I understand correctly, you intent to start or have already begun, replacing this method of the skin API with a better interface that existing consumers of this stable API will be migrated towards. That sounds good to me.

Mentioned in SAL (#wikimedia-operations) [2022-01-26T19:19:13Z] <lucaswerkmeister-wmde@deploy1002> Synchronized php-1.38.0-wmf.19/skins/Timeless/includes/TimelessTemplate.php: Backport: [[gerrit:757470|Do not duplicate categories in primary action tabs space (T300100)]] (duration: 00m 51s)

I'm not leaving room for interpretation here because I co-wrote and approved the policy in question.

Right Skin::buildContentNavigationUrls method is protected and not tagged as @stable to override.

that it stands to reason that developers would not have made this method protected if it wasn't meant to be called by individual Skins as otherwise it would be indistinguishable from a private method, or it would have been marked as @internal

The method was made protected in 2011 before @internal became a thing. I think this needs to be marked as @internal. I'll open a task.