Change 503146 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/core@master] Add test to check action- messages exist

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

Change 503146 merged by jenkins-bot:
[mediawiki/core@master] Add test to check action- messages exist

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

reedy@deploy1001:/srv/mediawiki-staging/php-1.33.0-wmf.25/extensions$ grep -R -c \"right\- */i18n/en.json | grep -v \:0
AbuseFilter/i18n/en.json:13
AntiSpoof/i18n/en.json:1
ArticleCreationWorkflow/i18n/en.json:1
CentralAuth/i18n/en.json:8
CentralNotice/i18n/en.json:1
CheckUser/i18n/en.json:2
CodeReview/i18n/en.json:10
Collection/i18n/en.json:2
ConfirmEdit/i18n/en.json:1
Flow/i18n/en.json:6
Gadgets/i18n/en.json:2
GlobalBlocking/i18n/en.json:3
GWToolset/i18n/en.json:1
Interwiki/i18n/en.json:1
JsonConfig/i18n/en.json:1
LiquidThreads/i18n/en.json:3
MassMessage/i18n/en.json:1
Newsletter/i18n/en.json:4
Nuke/i18n/en.json:1
OATHAuth/i18n/en.json:3
OAuth/i18n/en.json:7
OpenStackManager/i18n/en.json:6
PageTriage/i18n/en.json:1
Petition/i18n/en.json:1
ProofreadPage/i18n/en.json:2
Renameuser/i18n/en.json:1
SecurePoll/i18n/en.json:1
SpamBlacklist/i18n/en.json:1
TimedMediaHandler/i18n/en.json:2
TitleBlacklist/i18n/en.json:3
TorBlock/i18n/en.json:1
UploadWizard/i18n/en.json:2
UrlShortener/i18n/en.json:1
UserMerge/i18n/en.json:1
VipsScaler/i18n/en.json:1
WikibaseMediaInfo/i18n/en.json:1
WikimediaIncubator/i18n/en.json:1
ZeroBanner/i18n/en.json:1

vs

reedy@deploy1001:/srv/mediawiki-staging/php-1.33.0-wmf.25/extensions$ grep -R -c \"action\- */i18n/en.json | grep -v \:0
AbuseFilter/i18n/en.json:10
CentralAuth/i18n/en.json:4
CentralNotice/i18n/en.json:1
CheckUser/i18n/en.json:2
CodeReview/i18n/en.json:10
Flow/i18n/en.json:1
Gadgets/i18n/en.json:2
GlobalBlocking/i18n/en.json:3
GWToolset/i18n/en.json:1
Interwiki/i18n/en.json:1
JsonConfig/i18n/en.json:1
LiquidThreads/i18n/en.json:1
MassMessage/i18n/en.json:1
Newsletter/i18n/en.json:4
Nuke/i18n/en.json:1
OATHAuth/i18n/en.json:2
OAuth/i18n/en.json:5
OpenStackManager/i18n/en.json:3
PageTriage/i18n/en.json:1
Petition/i18n/en.json:1
Renameuser/i18n/en.json:1
SecurePoll/i18n/en.json:1
SpamBlacklist/i18n/en.json:1
TimedMediaHandler/i18n/en.json:2
TitleBlacklist/i18n/en.json:1
UploadWizard/i18n/en.json:1
UrlShortener/i18n/en.json:1
UserMerge/i18n/en.json:1

Looking at wmf is not enough, there are at least 114 missing action messages in 77 extensions.

All these 77 extensions (out of 800 extensions) are broken now ...

AdManager                action-admanager
AdminLinks               action-adminlinks
AdvancedMeta             action-advancedmeta-edit
AnonPrivacy              action-anonprivacy
ApprovedRevs             action-viewlinktolatest
ApprovedRevs             action-viewapprover
ApprovedRevs             action-approverevisions
ArticleFeedbackv5        action-aft-oversighter
ArticleFeedbackv5        action-aft-member
ArticleFeedbackv5        action-aft-administrator
ArticleFeedbackv5        action-aft-monitor
ArticleFeedbackv5        action-aft-reader
ArticleFeedbackv5        action-aft-editor
ArticleToCategory2       action-ArticleToCategory2
ArticleToCategory2       action-ArticleToCategory2AddCat
AuthorProtect            action-author
AutoProxyBlock           action-notagproxychanges
AutoProxyBlock           action-proxyunbannable
BibManager               action-bibmanagercreate
BibManager               action-bibmanagerdelete
BibManager               action-bibmanageredit
BlueSpiceChecklist       action-checklistmodify
BlueSpiceExtendedStatistics action-statistic-viewspecialpage
BlueSpiceFoundation      action-wikiadmin
BlueSpicePageAssignments action-pageassignments
BlueSpicePageAssignments action-pageassignable
BlueSpiceUsageTracker    action-usagetracker-update
BlueSpiceUserManager     action-usermanager-editpassword
Cargo                    action-runcargoqueries
Challenge                action-challengeadmin
ChangeUserPasswords      action-changeuserpasswords
CloseWikis               action-editclosedwikis
CloseWikis               action-closewikis
Comments                 action-commentadmin
Comments                 action-commentlinks
Comments                 action-comment-delete-own
Comments                 action-comment
ConfirmAccount           action-lookupcredentials
ConfirmAccount           action-requestips
DataTransfer             action-datatransferimport
DeleteBatch              action-deletebatch-spoof
DeleteOwn                action-deleteown
DeleteUserPages          action-delete-usersubpages
DeleteUserPages          action-delete-rootuserpages
Diagnosis                action-diagnosis-access
Duplicator               action-duplicate
Farmer                   action-createwiki
Farmer                   action-farmeradmin
FilterListUsers          action-viewallusers
FlaggedRevs              action-movestable
FlaggedRevs              action-autoreview
FlaggedRevs              action-stablesettings
FlaggedRevs              action-autoreviewrestore
FlaggedRevs              action-unreviewedpages
FlaggedRevs              action-validate
ForcePreview             action-forcepreviewexempt
GlobalUserrights         action-userrights-global
GoToShell                action-gotoshell
GoogleLogin              action-managegooglelogindomains
HAWelcome                action-welcomeexempt
LastUserLogin            action-lastlogin
LdapGroups               action-manageldapgroups
LinkFilter               action-linkadmin
LinkedWiki               action-data-edit
Maintenance              action-maintenance
MarkAsHelpful            action-markashelpful-view
MarkAsHelpful            action-markashelpful-admin
MediaWikiAuth            action-mwa-createlocalaccount
MintyDocs                action-mintydocs-administer
MintyDocs                action-mintydocs-edit
NewSignupPage            action-bypasstoscheck
NoBogusUserpages         action-createbogususerpage
NukeDPL                  action-nukedpl
OfflineImportLexicon     action-offlineimportlexicon
PageBlock                action-page-block
Patroller                action-patroller
PictureGame              action-picturegameadmin
Poll                     action-poll-create
Poll                     action-poll-vote
Poll                     action-poll-admin
Poll                     action-poll-score
PrivateDomains           action-privatedomains
PureWikiDeletion         action-purewikideletion
Push                     action-filepush
QuizGame                 action-quizadmin
QuizTabulate             action-viewquiztabulate
ReassignEdits            action-reassignedits
RegexBlock               action-regexblock-exempt
SecureAuth               action-sa-access
SemanticACL              action-sacl-exempt
ShowRealUsernames        action-showrealname
UserBoard                action-userboard-delete
SpellingDictionary       action-spelladmin
StaffEdits               action-staffedit
StaffPowers              action-unblockable
StalkerLog               action-stalkerlog-view-log
UIFeedback               action-read_uifeedback
UIFeedback               action-write_uifeedback
UserStatus               action-delete-status-updates
VoteNY                   action-voteny
WatchSubpages            action-watchsubpages
WebChat                  action-webchat
Widgets                  action-editwidgets
WikiForum                action-wikiforum-moderator
WikiForum                action-wikiforum-admin
WikiLexicalData          action-addlanguage
WikimediaMessages        action-templateeditor
WikimediaMessages        action-viewdeletedfile
WikimediaMessages        action-editeditorprotected
WikimediaMessages        action-editextendedsemiprotected
WikimediaMessages        action-extendedconfirmed
WikimediaMessages        action-superprotect
WikimediaMessages        action-banner-protect
googleAnalytics          action-noanalytics

Structure tests should be enable after fixing the extensions and not produce something like "Unbreak now"

Structure tests should be enable after fixing the extensions and not produce something like "Unbreak now"

This landed just after the REL1_33 cut, which gives developers six months to fix before people will sanely be using their code. Hardly "unbreak now".

In T220779#5108274, @Umherirrender wrote:

Structure tests should be enable after fixing the extensions and not produce something like "Unbreak now"

A couple of those were waiting on jenkins, and are now merged

It's mostly a copy paste job, and takes a few minutes to fix. So if any of the extensions are actually actively maintained, it won't take them that long to fix it.. Never mind these messages should've really existed in the first place (and probably just means no one has changed default config and exposed the missing messages)

In T220779#5108357, @Jdforrester-WMF wrote:

Structure tests should be enable after fixing the extensions and not produce something like "Unbreak now"

This landed just after the REL1_33 cut, which gives developers six months to fix before people will sanely be using their code. Hardly "unbreak now".

Yes, that keeps the branches alive and does not need backports.
But when libraryupgrader or other version updates or deprecation removal are done on extensions, than broken jobs always annoying and need investigation, if the patch set itself is failing or the extensions CI is in a broken state

The message is only needed when a permission error page is shown, like for special pages. But the 'edituserjs' right never shows that page, it shows a disabled edit form when the permission is missing. But having the messages seems better than missing the message in some situation.

I have uploaded some patch sets - https://gerrit.wikimedia.org/r/#/q/branch:master+topic:action-message

I recommend double-checking before merging these extension changes, as these tests don't make any sense in quite a few cases - for anything happening behind the scenes (jobs, bypasses, special checks as part of other actions for different handling), there's no way to trigger these messages in the first place. So having extraneous messages will really only make the source files more complicated and waste the translators' time.

This came up in MediaWikiAuth because there we have a permission that all it does is return early and not keep checking other things if they have it; there is no special action associated with it to begin with. While we may add an actual action later in this particular case, for quite a few other extensions that's definitely not going to happen: for instance HAWelcome runs a job behind the scenes and just skips it with its right...

So, uh, we'll probably need to revert a bunch of these changes, and make the test itself a lot smarter, since we need there to even be an associated action to begin with for an action message to make sense...

Adding a few dozens "extra" messages is not a problem in itself, assuming the text makes sense and the documentation is clear so that they translators are not driven mad. Ideally, a useful reminder to developers like this would not require shifting some work to the translators.

In T220779#5109117, @Isarra wrote:

This came up in MediaWikiAuth because there we have a permission that all it does is return early and not keep checking other things if they have it; there is no special action associated with it to begin with. While we may add an actual action later in this particular case, for quite a few other extensions that's definitely not going to happen: for instance HAWelcome runs a job behind the scenes and just skips it with its right...

That isn't a userright then...

https://www.mediawiki.org/wiki/Manual:User_rights

User rights are specific access and ability permissions that can be assigned to customizable user groups.

How many of these are actually exceptions? How many are likely to actually be attempted to translated (in most cases, it's a copy paste of the right, maybe with a casing change; at least in english)? There are (at least with my out of date clones) ~355 right- messages in WMF hosted extensions that are defined. In the same clone, ~256 are defined. Numerous of the missing ones are indeed missing, they do have an action, but in most cases people leave them as the default config, so it's (highly) unlikely that the message will be needed, but that doesn't mean it won't ever be

The issue is the test doesn't make sense - it should only be checking for the action message if there is an associated action users can access. @ashley could probably say with more certainty, as he's the one who was specifically looking into this, but at a guess this could apply even to just ones that add special pages or page actions, and that would do it.

User rights are specific access and ability permissions that can be assigned to customizable user groups.

Bypassing normal checks/restrictions/actions is an ability. Bypassing account creation limits or other rate limits is another such ability that similarly does not have any user-facing places where an access message would make any sense, because it too simply silently bypasses a check behind the scenes.

Actions are a fuzzy concept in MediaWiki (see e.g. T212345: Authorization checks have $action parameter, but accept a user right). In one sense, an action is something that has an Action subclass and can be triggered via the action= URL parameter, and that's a small subset of rights. (Well, not technically a subset, there are actions like history which do not have an associated user right.) But the action- messages are used in permission error messages, and in the future we'll probably want to move towards always having error messages (T180888: All permission checks should be able to return a custom error message) as it is hard for the extension author to predict when they will be necessary, so having these messages seems like a step in the right direction. There will be exceptions, but it is always better to err on the harmless side.

Outside core user rights can and in many cases do exist without the corresponding action-* message, for the action-* message is primarily (AFAIK) used by the code in SpecialPage#checkPermissions which extensions can use when checking for sufficient access privileges in the execute method of a restricted SpecialPage; but special page extensions are hardly the only kind of extensions which exist.

Like I wrote in a few of the patchsets (for StaffPowers, StaffEdits, VoteNY at least) that I had a chance to review earlier on related to extensions I maintain, it seems highly unlikely if not outright impossible for anything to cause a broken i18n string to be displayed to the end-user, because these extensions are not SpecialPages and thus they do error handling differently; and that is perfectly fine and acceptable. Maybe for core it makes sense to have an actually enforcing test but extensions should be able to decide this on a case-by-case basis.

Adding extraneous i18n messages that are actually never used by anything serves to increase translator workload at TWN, and given that TWN intentionally refuses to support extensions with inadequate or missing message documentation (a.k.a qqq), it's beyond me why supporting literally unused messages would be any more OK. Why have an unused message or messages in the first place?!

Furthermore I want to stress that even though I disagree with what was done here and how quickly these patches got merged, I'm very grateful to @Umherirrender for providing these patches - kudos! If only everyone would provide patches in this fashion as they deprecate or remove methods from core...(however, I do believe that extension and skin authors should be able to adequately CR such patches, else we'd have a formal auto-approval system for patches which only fix deprecated things, like how we currently auto+2 the TWN bot's patches; but we don't, as there is no need to turn code review into a rubber stamping factory).

Another problem is if we pre-create these messages when they're not needed, if we do wind up needing them later, this will almost certainly be paired with a change to how we describe what the permission does, because we will in fact be changing what the permission does at this point, or at very least a change to how we describe the action, which will definitely be something new. Thus all associated strings will need to be updated and retranslated, which means the preemptive additions and translations will have been pointless.

There a four extensions left which missing the messages and have a broken build since the addtion of the structure test.

How to process? Do you suggest a revert and who decide that? Adding a new config to be excluded from the structure test or it is possible to merge the patch sets to unbreak the build of the extension?

or it is possible to merge the patch sets to unbreak the build of the extension?

That seems like the obvious thing to do.

In T220779#5260027, @Tgr wrote:

or it is possible to merge the patch sets to unbreak the build of the extension?

That seems like the obvious thing to do.

Done