Page MenuHomePhabricator

[subtask] Permissions based errors should not reuse messags intended for skin labels
Closed, ResolvedPublicBUG REPORT

Description

Various messages are shared between skins and form errors. These concerns should be separated so that skins can be maintained in such a way that they do not need to worry about side effects.

OutputPage::formatPermissionsErrorMessage shares messages that are also used in skins. Offending line is:

$action_desc = $this->msg( "action-$action" )->plain();

This should be updated to use a dedicated message for this context so that skin interface messages can be changed for.

Special:Movepage also uses action-move. This should be updated to use a dedicated message for this context.

To avoid translations, I suggest we copy across existing messages where needed.

Event Timeline

No, it’s the other way round: skins should not reuse messages intended for permission-based errors. Apart from some refactorings, the line you cited hasn’t changed d71a1a4b48d, the commit that also added these messages to the codebase 13 years ago. I don’t know why you think they were intended for skin labels, but they weren’t.

I've corrected the description but my recommendation still stands.

The change in c510db2665c in Dec 31 2010 to introduce skin-specific versions of these messages on skins ($skname- prefix) , means the messages are primarily seen by most users in skins and should be considered skin labels at this point.

It's far less risky at this point to update these two lines rather than rearchitect the skins.

Jdlrobson renamed this task from Permissions based errors should not reuse messags intended for skin labels to [subtask] Permissions based errors should not reuse messags intended for skin labels.Mar 4 2022, 4:22 PM
Jdlrobson claimed this task.
Jdlrobson triaged this task as Medium priority.

This is obviously an error in the original patch, and it should be reverted. These messages have been used that way for 10+ years and it's not acceptable to just change them. You should pick different keys for the new messages.

Fixing the problem does not require you to "rearchitect the skins" either, it's a trivial change in SkinTemplate::getSkinNavOverrideableLabel(), and then the translations that got bungled up in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/761985 need to be restored.

@matmarex I am not introducing new messages. The key vector-action-move has also been that way for 10+ years. Skin overrides must now be the default key prefixed with the skin name, which is an intentional change to make this more intuitive and maintainable, so if I don't change these two instances, I have to rename vector-action-move, monobook-action-move, action-move etc.. which seems more disruptive to me.

If you really think that's the way to go, I can do that, but length of time spent in the code doesn't seem like the best way to judge this.

(I plan to import the messages either way)

Skin overrides must now be the default key prefixed with the skin name

They only "must" be because you decided so. Just decide otherwise. We could perhaps decide that vector-action-move is the skin override, and skin-action-move is the default. The possibilities are endless.

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

[mediawiki/core@master] Use permissionserrorstext- prefixed messages for permission errors

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

I've corrected the description but my recommendation still stands.

The change in c510db2665c in Dec 31 2010 to introduce skin-specific versions of these messages on skins ($skname- prefix) , means the messages are primarily seen by most users in skins and should be considered skin labels at this point.

It's far less risky at this point to update these two lines rather than rearchitect the skins.

I think you're oversimplifying this. Many extensions use action- messages for their own rights; there are integration tests ensuring their presence. All these will need to be renamed too.

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

[mediawiki/core@master] Introduces skin- prefixed message key for nav items

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

So my thinking was to add a new prefixed and preferred message and fallback to action-$1 when that didn't exist. That helped me achieve my short-term goal of separating the skin messages from the form actions and provided a long-term path for giving more context to where these messages get used. I've asked Bartosz for clarity on the ticket as he said its not feasible for a reason I don't understand.

I think @matmarex suggestion of supporting a message prefixed with skin- also makes sense. Ideally, I would like to do both, I am just a little concerned with the amount of work in the latter and high likelihood of annoying Monobook users. I've done that in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/768750 but only for the 3 problematic messages that remain to limit the work remaining.

Change 768750 merged by jenkins-bot:

[mediawiki/core@master] Introduces skin- prefixed message key for nav items

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

The changes to action messages are causing problems in translations on Translatewiki.net, with BROKEN messages imported by Fuzzybot (affects 380 languages) since this morning 8 March around 08:20 UTC and 11:00 UTC). All these new imports are non-sense !

If so, this is the culprit: https://github.com/wikimedia/mediawiki/commit/40d041e0642dec08bfc00e07390e2d598e1baf03

Many messages were broken (signaled in TWN's Support page, and in TWN's Telegram channel.
Niklas Laxström and Abijeet Patrot have been alerted.

Above, the statement made in the task description: "To avoid translations, I suggest we copy across existing messages where needed." was not a suggestion

It was effectively applied, and it broke unrelated messages with blind/incorrect copies (even some messages partly splitted, with one part moved to another unrelated message, breaking BOTH messages at the same time, and many messages reverted/deleted incorrectly, and many others marked incorrectly as "FUZZY").

Can you explain how they are broken and nonsense?

See: Thread:Support/Several problematic messages on TranslateWiki.net support.
Or look at the discussion in Telegram channel for TWN.

Can you also explain which messages you're talking about? I found a thread at https://translatewiki.net/wiki/Thread:Support/Several_problematic_messages, but there you're talking about some messages from the Android app and Wiki Ed Dashboard, and I don't see how that's relevant to this bug.

It is the analysis made by Jon Harald Soby (on Telegram) :

The changes to action messages are what's causing problems, right? If so, this is the culprit: https://github.com/wikimedia/mediawiki/commit/40d041e0642dec08bfc00e07390e2d598e1baf03

GitHub (https://github.com/wikimedia/mediawiki/commit/40d041e0642dec08bfc00e07390e2d598e1baf03)
Introduces skin- prefixed message key for nav items · wikimedia/mediawiki@40d041e
This takes precedent over the other messages e.g. action-protect

Since only 3 messages clash with the protection forms, only 3 are
ported for now.

The action-protect, action-move and action-undel...


Jon Harald Søby, [08/03/2022 14:06]
a lot of weird stuff there


Jon Harald Søby, [08/03/2022 14:06]
like, why is Jdlrobson changing "action-move", "action-delet" etc. in so many different languages?

They are relevant, the support in Support just gives a few examples, but all the new imports made by Fuzzybots in TWN at around 08:00 UTC and 11:00 UTC are caused by this task. They edited too many messages from the Wikimedia meta-group on TWN, because they come from the same edited source file in Gerrit, shared by multiple MediaWiki extensions (not just the two extensions as you think).

This is a project management problem: separate extensions (e.g. for skins) should have their own source files, even if this means adding new messages IDs (with similar contents), without breakling other extensions.

And in fact the way these messages were forcibly edited (mixing parts of messages across different lines, probably because of a bad edit in an external text editor with some bad regexps, ignoring line breaks)

I was mostly looking at the messages mentioned by Ooswesthoesbes in the Support thread on Translatewiki, which are related to the changes from this task (it's difficult to parse what happened there, but I think the end result now should be OK).

The problems Verdy_p mentioned in the TWN Support thread are indeed not related to this task, but to other bits of software.

Change 768178 abandoned by Jdlrobson:

[mediawiki/core@master] Use permissionserrorstext- prefixed messages for permission errors

Reason:

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

Jdlrobson added a subscriber: Jdrewniak.

There should be no need for any further changes. So this is what happened:

  • In https://gerrit.wikimedia.org/r/c/mediawiki/core/+/761985 I batch changed all the values of action-move, action-delete, action-undelete, action-protect to vector-action-move, vector-action-delete, vector-action-undelete and the unprotect message respectively as I renamed those messages.
  • I was alerted to this bug T303012 (discovering those messages were still being used in a different context)
  • In https://gerrit.wikimedia.org/r/c/mediawiki/core/+/768750/ I restore the values of action-move, action-delete, action-undelete, action-protect to their previous values (as they were in 98fa55566a9df1d6dd74654979f890d75c602220 before the first change) and add 4 new messages to replace them in skins.

I think we should be done now so I'll QA this later today.

Yes, I agree with @Jdlrobson here.

To rephrase in short: the translations were actually overwritten incorrectly last week by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/761985, and yesterday that was undone and the problem was fixed by https://gerrit.wikimedia.org/r/c/mediawiki/core/+/768750.

@MF-Warburg either today or tomorrow. It depends on which wiki are you seeing the incorrect MonoBook messages on which is not clear from your original bug report.

Italian is a group 2 wiki so I can test this now.

Working on Italian: https://it.wikipedia.org/wiki/Pagina_principale?action=protect&uselang=en

Screen Shot 2022-03-09 at 3.59.52 PM.png (888×2 px, 291 KB)

For comparison English Wikipedia
Screen Shot 2022-03-09 at 4.00.34 PM.png (1×2 px, 378 KB)

Monobook German labels also looking correct on Italian:
https://it.wikipedia.org/wiki/Pagina_principale?useskin=monobook&uselang=de

Screen Shot 2022-03-09 at 3.55.41 PM.png (462×2 px, 282 KB)

Will be on English/German tomorrow after which I'll clean up some messages and resolve this out.

This should be fixed everywhere now. Please let me know if anything looks incorrect by letting me know the URL and the skin being used.