Change #1093960 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[operations/mediawiki-config@master] Enable Skin-Codex logging

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

Change #1093960 merged by jenkins-bot:

[operations/mediawiki-config@master] Enable Skin-Codex logging

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

Mentioned in SAL (#wikimedia-operations) [2024-11-21T21:20:44Z] <brennen@deploy2002> Started scap sync-world: Backport for [[gerrit:1093960|Enable Skin-Codex logging (T375287)]]

Mentioned in SAL (#wikimedia-operations) [2024-11-21T21:26:26Z] <brennen@deploy2002> brennen, jdlrobson: Backport for [[gerrit:1093960|Enable Skin-Codex logging (T375287)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-11-21T21:36:38Z] <brennen@deploy2002> Finished scap sync-world: Backport for [[gerrit:1093960|Enable Skin-Codex logging (T375287)]] (duration: 15m 53s)

According to logging about 70k pages every 15 mins are rely on this behaviour
https://logstash.wikimedia.org/goto/1bd455888d36e19ddb5476db1257d36b

I made a few tasks for the biggest offenders and will submit a few patches shortly.

Change #1094052 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/NearbyPages@master] Special:Nearby shouldnt rely on skin code to render message box

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

Change #1094058 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Article: Add message box styles where needed

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

Change #1094052 merged by jenkins-bot:

[mediawiki/extensions/NearbyPages@master] Special:Nearby shouldn't rely on skin code to render message box

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

Change #1094058 merged by jenkins-bot:

[mediawiki/core@master] Article: Add message box styles where needed

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

Sorry, I want to make sure that I understand this task correctly: is the requirement that everyone who calls Html::errorBox etc. also explicitly loads Codex styles via RL? If so, this seems quite problematic, especially given the "when logs < 1000", for the following reasons:

• Nowhere in the documentation of Html::errorBox and friends is it stated that one needs to load Codex styles explicitly. Not only has it never been stated, but it isn't even mentioned now that this behaviour is slated for removal -- without deprecation, even. This is the kind of breaking change that should be announced on wikitech-l.
• The very fact that the *Box methods rely on Codex seems an implementation detail to me. In fact, they've only been using Codex for the last year or so. It doesn't seem ideal to tie a generic/abstract method (whose purpose is to "create an error box") to a specific implementation. But I guess it's not the end of the world if we do that.
• If the dependency really needs to be made explicit, can it at least be hidden behind helper methods in OutputPage (e.g., getErrorBox(), or even addErrorBox() that also adds it to the output) that load the necessary styles and then call the Html:: methods? This leaves the developer with a convenient way to create error boxes (a single method call), without having to worry about Codex.
• Isn't there going to be inevitable breakage when the feature is removed? Things like errorBox are used to display errors, which by definition are not part of the most common happy paths. From the task description, it's not clear what the timeframe applied to the "< 1000" would be (the dashboard linked is set to "last 15 minutes"), but I guess lots of usages wouldn't be detected, for all errors that don't tend to happen a lot.

If the hack in Skin.php reeeeeally needs to go away ASAP, the Html:: methods themselves could add the relevant styles to the global OutputPage object. It isn't pretty, but at least it would prevent breakage.

I hope the following context helps!

Sorry, I want to make sure that I understand this task correctly: is the requirement that everyone who calls Html::errorBox etc. also explicitly loads Codex styles via RL?

Yes you are understanding correctly. The skin is no longer responsible for rendering message boxes as not every page renders a message box.

can it at least be hidden behind helper methods in OutputPage

Yes. This function probably shouldn't exist in Html, similar to the existing Html::listDropdownOptionsOoui with the note TODO Find a better home for this function.. It does not create primitive HTML like the other methods.

I imagine on the long run this would be handled by the Codex PHP library that is in development and announced on wikitech-l yesterday (and a potential wrapper class). We don't want to automatically add the stylesheets, as in some cases this would add additional and unnecessary CSS. Most things using this method are already using Codex for other components so we wouldn't want to load additional CSS if it's been loaded already.

Not only has it never been stated, but it isn't even mentioned now that this behaviour is slated for removal -- without deprecation, even

How exactly would you wish for deprecation to be applied to a behaviour change? I'm not sure I'm understanding what is being asked for here. I am not sure what exactly would be deprecated here? Html::messageBox is not being deprecated if that wasn't clear, neither is any style module (At least not until a Codex PHP alternative has been provided)

Nowhere in the documentation of Html::errorBox and friends is it stated that one needs to load Codex styles explicitly.

patch welcome! In future, the hope is this will be self explanatory as any developer adding a new usage would see an unstyled error box on their pages if they didn't do this.

Isn't there going to be inevitable breakage when the feature is removed?

No, that's why we are logging PHP warnings now.

The code for adding the stylesheet is intended only to serve articles which make use of the markup so the code won't be removed until T363607 is done which makes that unnecessary. I don't suspect that to happen any time soon. When we do that, deprecation notices will be printed in the console per the process in https://www.mediawiki.org/wiki/Stable_interface_policy/Frontend.

However, an unintended side effect of this code path is that it's also adding styles to pages (e.g. special pages) which are using Codex components via Html::messageBox, thus should have added Codex styles which is a bug in the existing code. If we adjust the logic, we'd also increase the level of the existing log messages so that they are treated as Wikimedia-production-error

Note the <1000 number in the description relates to what I suspect the traffic will be in a 15 minute period when it has been limited to only apply to wikitext.

Looks like Flow (https://www.mediawiki.org/wiki/Topic:Up52ohtctpo4zhe1?uselang=qqx) also is at fault, but given plans to remove it, I've not filed a ticket.

In T375287#10359773, @Jdlrobson wrote:

I hope the following context helps!

Thank you!

can it at least be hidden behind helper methods in OutputPage

Yes. This function probably shouldn't exist in Html, similar to the existing Html::listDropdownOptionsOoui with the note TODO Find a better home for this function.. It does not create primitive HTML like the other methods.

I imagine on the long run this would be handled by the Codex PHP library that is in development and announced on wikitech-l yesterday (and a potential wrapper class). We don't want to automatically add the stylesheets, as in some cases this would add additional and unnecessary CSS. Most things using this method are already using Codex for other components so we wouldn't want to load additional CSS if it's been loaded already.

Good point. I wonder if this would be a good time to establish a convenient pattern for using CSS-only Codex components in MW, given the upcoming addition of codex-php to core. Something like shortcuts in OutputPage to deal with (part of) the boilerplate, like the WebRequest stuff and also loading the appropriate styles.

Not only has it never been stated, but it isn't even mentioned now that this behaviour is slated for removal -- without deprecation, even

How exactly would you wish for deprecation to be applied to a behaviour change? I'm not sure I'm understanding what is being asked for here. I am not sure what exactly would be deprecated here? Html::messageBox is not being deprecated if that wasn't clear, neither is any style module (At least not until a Codex PHP alternative has been provided)

Soft deprecation can be done as usual via documentation. We can state that callers are required to add the XYZ style modules before using those methods; and that not doing so is deprecated. Then, the same code that logs the warning could use wfDeprecatedMsg to trigger a deprecation notice, for the hard deprecation phase. But as you say, it can be tricky to apply deprecation to behaviour changes. That's why I mentioned wikitech-l: it's the standard process for removing deprecated code that can't easily be mark as hard-deprecated.

Nowhere in the documentation of Html::errorBox and friends is it stated that one needs to load Codex styles explicitly.

patch welcome! In future, the hope is this will be self explanatory as any developer adding a new usage would see an unstyled error box on their pages if they didn't do this.

I'm more concerned about the present than the future -- right now it's impossible to tell what's wrong with just calling the *Box methods. The only way to realize that you need to add Codex styles is if you stumble upon this task or one of its linked patches.

Even in the future though, we need to provide guidance. I don't think anyone, upon seeing an unstyled error box, would immediately be like "ah sure, I need to load the mediawiki.codex.messagebox.styles module that is not documented anywhere".

Isn't there going to be inevitable breakage when the feature is removed?

No, that's why we are logging PHP warnings now.

The code for adding the stylesheet is intended only to serve articles which make use of the markup so the code won't be removed until T363607 is done which makes that unnecessary. I don't suspect that to happen any time soon. When we do that, deprecation notices will be printed in the console per the process in https://www.mediawiki.org/wiki/Stable_interface_policy/Frontend.

OK, thanks for clarifying. My concerns were mostly due to the fact that error states are relatively uncommon, and that some could go unnoticed for a long time. But if this isn't happening anytime soon, then I guess it's fine.

Change #1094501 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] Html: Add hint about mediawiki.codex.messagebox.styles to Html::*Box

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

Change #1094501 merged by jenkins-bot:

[mediawiki/core@master] Html: Add hint about mediawiki.codex.messagebox.styles to Html::*Box

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

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

[mediawiki/core@master] Modernize status and error message handling

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

Change #1081214 merged by jenkins-bot:

[mediawiki/core@master] Modernize status and error message handling

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

Change #1116887 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Translate@master] Add stylesheet explicitly to search page

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

Change #1116887 merged by jenkins-bot:

[mediawiki/extensions/Translate@master] Add stylesheet explicitly to search page

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

Change #1119214 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Limit Codex logging to action and special pages

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

Change #1119214 merged by jenkins-bot:

[mediawiki/core@master] Limit Codex logging to action and special pages

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

Change #1098184 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Skins: Limit where Codex message box styles are auto loaded.

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

Change #1123707 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Add message box styles explicitly

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

At this point I'm confident all pages have been identified in this query:
https://logstash.wikimedia.org/goto/8778fe099fd4a421ae0d8bf21a9520ca

I've posted patches for the main offenders.

Change #1123707 merged by jenkins-bot:

[mediawiki/core@master] Add message box styles explicitly

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

@Jdlrobson This task were resolvable with https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1098184 in, correct?

In T375287#10621539, @Volker_E wrote:

@Jdlrobson This task were resolvable with https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1098184 in, correct?

Correct!

Change #1098184 merged by jenkins-bot:

[mediawiki/core@master] Skins: Limit where Codex message box styles are auto loaded.

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

I've created a user notice T388847: Please report unstyled Codex message boxes in case we missed any edge cases due to low traffic in our logging.