Page MenuHomePhabricator

Resolve revisiondelete/editchangetags debt around fake action/specialpage handling
Closed, ResolvedPublic

Description

Background

The form on action=history ("View history" on any wiki page) has three possible destinations:

  1. compareselectedversions (Compare selected revisions)

    This uses the 'oldid' and 'diff' radio buttons and generally has no 'action' param. The response is handled by ViewAction, and generally looks like a page view with a diff on top.
  1. revisiondelete (Change visibility ...)

    This uses the 'ids[x]' checkboxes and is handled by action=revisiondelete. Although the form actually goes to action=historysubmit&revisiondelete=1, with a server-side handler to rewrite it as action=revisiondelete, and there is also JavaScript to do so client-side directly when possible to keep the URLs prettier and forward-compatible.

    This hack was created in 2009 to support IE6 (SVN r57415, T22966), because IE6 did not always submit the value of clicked button itself during a form submission.
  1. editchangetags (Edit tags ..)

    Like revisiondelete, added in 2015 with I7d3ef927b (5c4681012e).

    As part of adding EditTags, the fake historysubmit action was further evolved to also include a fake SpecialPage class SpecialPageAction.

Problem

There are three areas of technical debt surrounding this:

  1. Performance and novel complexity around ActionFactory and SpecialPage.

    The ActionFactory has additional complexity to deal with the fake historysubmit action to support the RevisionDelete and EditTags features. On top of that, SpecialPageAction currently violates T314008 due to attempting to retroactively change the computed title/action in the global RequestContext. This very likely doesn't work well, if at all. Removing it seems to have no effect, but it may've worked in the past when fewer things were dependency-injected in MediaWiki. In any event, today both of these features still fully appear to the end user as a page action=.. experience, not as a special page. This should allow for removal with minimal disruption.

    Violation of T314008 incurs significant overhead due to re-computing the WikiPage and Action which involves missing process caches and having to re-run database queries and expensive hooks. Plus, in practice it doesn't actually work as many code paths have already decided for themselves what the context title and action are. Plus, when it does work, it actually does more harm than good since we actually don't want to render any UI with the virtual context of a special page action=view, the user needs to see the HTML as if it was on action=revisiondelete.

    These two are the only ones using SpecialPageAction and all needed code for them to work.
  1. JavaScript and double code maintenance.

    In order to present cleaner URLs, the server-side rewrite in ActionFactory is usually not excercised in favour of client-side code in mediawiki.action.history.js creating and maintaining a virtual copy of the form to change what happens during form submission.
  1. Understandability and consistency with other actions.

    These two actions are currently very non-trivial to understand how they work due to four levels of indirection in how they appear. The browser submits to action=historysubmit, which is rewritten as action=revisiondelete, which is rewritten as SpecialPageAction, which is then rewritten as SpecialRevisionDelete. Knowing what anything means in SpecialRevisionDelete and how to route there is entirely novel and quite difficult to understand and get right. It's also unclear to what extend the intermediary forms can be used publicly and if they can whether they're meant to work and be supported.
  1. Bugs.

    As an example, when you use RevisionDelete today, the page can include broken or unexpected links. For example, go as logged-in sysop to "View history", select a revision, and use "Change visiblity". The resulting page is at /w/index.php?title=Main_Page&action=revisiondelete&type=revision&ids[]=1 but may feature links like title=Special:RevisionDelete&action=purge.

Proposal

The end result is similar to what existed between 2011 (SVN r89874) and 2015 (change 203061, I7d3ef927). Although at the time it was only a light wrapper, which was later not expanded but instead removed in favour of ActionFactory gaining the ability to dispatch SpecialPageAction.

Bug: T314008
Bug: T249265
Bug: T22966

Event Timeline

Change 858436 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] HistoryAction: Remove use of action=historysubmit hack

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

Stacktrace for this error to help with searching phab for this bug

Error
normalized_message
[{reqId}] {exception_url}   PHP Notice: Unexpected clearActionName after getActionName already called
exception.trace
from /srv/mediawiki/php-1.40.0-wmf.12/includes/context/RequestContext.php(311)
#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.40.0-wmf.12/includes/context/RequestContext.php(311): trigger_error(string)
#2 /srv/mediawiki/php-1.40.0-wmf.12/includes/context/RequestContext.php(176): RequestContext->clearActionName()
#3 /srv/mediawiki/php-1.40.0-wmf.12/includes/actions/SpecialPageAction.php(93): RequestContext->setTitle(Title)
#4 /srv/mediawiki/php-1.40.0-wmf.12/includes/MediaWiki.php(540): SpecialPageAction->show()
#5 /srv/mediawiki/php-1.40.0-wmf.12/includes/MediaWiki.php(317): MediaWiki->performAction(Article, Title)
#6 /srv/mediawiki/php-1.40.0-wmf.12/includes/MediaWiki.php(902): MediaWiki->performRequest()
#7 /srv/mediawiki/php-1.40.0-wmf.12/includes/MediaWiki.php(560): MediaWiki->main()
#8 /srv/mediawiki/php-1.40.0-wmf.12/index.php(50): MediaWiki->run()
#9 /srv/mediawiki/php-1.40.0-wmf.12/index.php(46): wfIndexMain()
#10 /srv/mediawiki/w/index.php(3): require(string)
#11 {main}
Krinkle triaged this task as Medium priority.

Change 858436 merged by jenkins-bot:

[mediawiki/core@master] HistoryAction: Remove use of action=historysubmit hack

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

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

[mediawiki/core@master] HistoryPager: Prettify URL params of form submissions without JS

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

Change 858586 merged by jenkins-bot:

[mediawiki/core@master] HistoryPager: Prettify URL params of form submissions without JS

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

Change 867302 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] [WIP] Convert action=revisiondelete into proper RevisionDeleteAction

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

Change 867302 abandoned by Krinkle:

[mediawiki/core@master] [WIP] Convert action=revisiondelete into proper RevisionDeleteAction

Reason:

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

Krinkle removed a project: Patch-For-Review.

Hello everyone! I'm on train duty this week and I'm doing some pre-train log triage (which means running logspam-watch on mwlog1002.eqiad.wmnet and seeing what shows up throughout the day). There is a persistent low volume stream of these Unexpected clearActionName after getActionName already called errors (T323338#8443691) . I say low volume but it is high enough to be second or third on the list of errors in a 60 minute window.

This is a request for priority on this ticket. If the error is expected to be persistent for some time into the future, I can filter it out in logspam-watch. Please advise.

Side note: This error is masked by the mediawiki-new-errors opensearch dashboard, attributed to T323153.

Nikerabbit subscribed.
[2023-06-07T01:05:25.668086+00:00] error.WARNING: [c841d4761cbbb1d3572544ec] /w/i.php?action=historysubmit&type=logging&title=Unused&editchangetags=1&ids%5B8110658%5D=1   PHP Notice: Unexpected clearActionName after getActionName already called {"exception":"[object] (ErrorException(code: 0): PHP Notice: Unexpected clearActionName after getActionName already called at /srv/mediawiki/tags/2023-06-05_15:56:59/includes/context/RequestContext.php:307)
[stacktrace]
#0 [internal function]: MWExceptionHandler::handleError()
#1 /srv/mediawiki/tags/2023-06-05_15:56:59/includes/context/RequestContext.php(307): trigger_error()
#2 /srv/mediawiki/tags/2023-06-05_15:56:59/includes/context/RequestContext.php(174): RequestContext->clearActionName()
#3 /srv/mediawiki/tags/2023-06-05_15:56:59/includes/actions/SpecialPageAction.php(93): RequestContext->setTitle()
#4 /srv/mediawiki/tags/2023-06-05_15:56:59/includes/MediaWiki.php(559): SpecialPageAction->show()
#5 /srv/mediawiki/tags/2023-06-05_15:56:59/includes/MediaWiki.php(334): MediaWiki->performAction()
#6 /srv/mediawiki/tags/2023-06-05_15:56:59/includes/MediaWiki.php(925): MediaWiki->performRequest()
#7 /srv/mediawiki/tags/2023-06-05_15:56:59/includes/MediaWiki.php(579): MediaWiki->main()
#8 /srv/mediawiki/tags/2023-06-05_15:56:59/index.php(50): MediaWiki->run()
#9 /srv/mediawiki/tags/2023-06-05_15:56:59/index.php(46): wfIndexMain()
#10 {main}
","exception_url":"/w/i.php?action=historysubmit&type=logging&title=Unused&editchangetags=1&ids%5B8110658%5D=1","reqId":"c841d4761cbbb1d3572544ec","caught_by":"mwe_handler"} []

Change 935126 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] Use special pages instead of actions for revision delete and edit tags

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

A bigger mess than I thought. "Merely" porting this from invisible special page class with fake action router to a real Action subclass is not as trivial as I thought.

The good news is that actual implementation code ports very well into a FormlessAction, much like DeleteAction and other Action subclasses. Very few changes needed to make it work and this commit makes it work.

The bad news is that unlike form submissions from HistoryPager, links from elsewhere do actually rely on the Special-page today which will have to be ported. In particular, the Log-deletion use case makes this non-trivial. E.g. continuing to use target/title=Unused for log delete, it's not obvious to me that this will work on wikis where Main space is special like on Wikidata or where maybe you don't have the right to delete a page in the Main space but do have the right to hide log events, then using a URL like title=Unused could be a problem. If there's a way to make that work for now, then we could do that as first step. Otherwise, an alternative first step might be to first split out a "Special:LogDelete" of sorts at the UI level at least, or to re-consider the direction of the task, and actually go all-in on it being a special page and figure out a way to satisfy that from the HistoryPager form side.

They indeed can be switched to use special pages directly without any issue, so let's go for this direction.

I like this approach a lot. Also, I did some archaeology and it looks like a special page was the originally intended interface for revision deletion – but we ended up adding the action and some extra hacks because it was impossible to make the history form submit data correctly to different special pages on IE 6 (r55741, r49408). I think we can safely say now that we don't care (especially for a fairly rarely used administrative feature – maybe if this was about editing pages, we would give it more thought).

matmarex updated the task description. (Show Details)

On modern browsers, everything behaves as before with the patch: I was able to view diffs, add tags and hide info for revisions, log entries and file versions. I tested on Patchdemo, and here are the URLs you get when submitting the forms, for comparison (since the demo wikis will be deleted at some point).

WhatOldNew
History:https://patchdemo.wmflabs.org/wikis/392214f3de/w/index.php?title=Main_Page&action=historyhttps://patchdemo.wmflabs.org/wikis/6314a93963/w/index.php?title=Main_Page&action=history
Compare selected revisionshttps://patchdemo.wmflabs.org/wikis/392214f3de/w/index.php?title=Main_Page&diff=6&oldid=2https://patchdemo.wmflabs.org/wikis/6314a93963/w/index.php?title=Main_Page&diff=6&oldid=2
Edit tags of selected revisionshttps://patchdemo.wmflabs.org/wikis/392214f3de/w/index.php?title=Main_Page&type=revision&action=editchangetags&ids[2]=1&ids[1]=1https://patchdemo.wmflabs.org/wikis/6314a93963/w/index.php?type=revision&title=Special%3AEditTags&ids[2]=1&ids[1]=1
Change visibility of selected revisionshttps://patchdemo.wmflabs.org/wikis/392214f3de/w/index.php?title=Main_Page&type=revision&action=revisiondelete&ids[2]=1&ids[1]=1https://patchdemo.wmflabs.org/wikis/6314a93963/w/index.php?type=revision&title=Special%3ARevisionDelete&ids[2]=1&ids[1]=1
File page:https://patchdemo.wmflabs.org/wikis/392214f3de/wiki/File:1.pnghttps://patchdemo.wmflabs.org/wikis/6314a93963/wiki/File:1.png
Change visibility of selected file versionshttps://patchdemo.wmflabs.org/wikis/392214f3de/w/index.php?target=File%3A1.png&action=historysubmit&type=oldimage&revisiondelete=1&ids[20230705212324]=1&ids[20230705212254]=1https://patchdemo.wmflabs.org/wikis/6314a93963/w/index.php?target=File%3A1.png&type=oldimage&title=Special%3ARevisionDelete&ids[20230705212329]=1&ids[20230705212301]=1
Logs:https://patchdemo.wmflabs.org/wikis/392214f3de/wiki/Special:Loghttps://patchdemo.wmflabs.org/wikis/6314a93963/wiki/Special:Log
Edit tags of selected log entrieshttps://patchdemo.wmflabs.org/wikis/392214f3de/w/index.php?action=historysubmit&type=logging&title=Unused&editchangetags=1&ids[3]=1&ids[2]=1https://patchdemo.wmflabs.org/wikis/6314a93963/w/index.php?type=logging&title=Special%3AEditTags&ids[3]=1&ids[2]=1
Change visibility of selected log entrieshttps://patchdemo.wmflabs.org/wikis/392214f3de/w/index.php?action=historysubmit&type=logging&title=Unused&revisiondelete=1&ids[3]=1&ids[2]=1https://patchdemo.wmflabs.org/wikis/6314a93963/w/index.php?type=logging&title=Special%3ARevisionDelete&ids[3]=1&ids[2]=1

However, I also tested on IE 11, since it lacks support for "interleaved" forms like the one we use for history (as discussed in https://gerrit.wikimedia.org/r/c/858586), and the patch causes some issues:

Assuming that we want to continue supporting IE 11 for this feature, then we need to either:

  • Add back 'type=revision' to the history form to support submitting to Special:RevisionDelete
  • Make Special:RevisionDelete assume that 'type' is 'revision' when not provided

Apparently Special:EditTags already behaves that way. If we did the latter, maybe we could remove 'type=revision' from the forms entirely.

Change 935126 merged by jenkins-bot:

[mediawiki/core@master] Use special pages instead of actions for revision delete and edit tags

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

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

[mediawiki/core@master] [WIP] Remove SpecialPageAction and associated logic, route old URLs

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

Change 936027 merged by jenkins-bot:

[mediawiki/core@master] Remove SpecialPageAction and associated logic, route old URLs

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

matmarex updated the task description. (Show Details)