Page MenuHomePhabricator

Resolve revisiondelete/editchangetags debt around fake action/specialpage handling
Open, MediumPublic

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

  • Change HistoryAction and HistoryPager to no longer use the action=historysubmit hack.

Instead, use action=revisiondelete directly as we already do with JavaScript today, and as we used to prior to 2009's IE6 hack at T22966. As part of this, we can custom JavaScript code, and after one week of allowing for HTML caches to roll over we can also delete the custom ActionFactory code.

  • Port SpecialRevisionDelete into a new RevisiondeleteAction class.
  • Port SpecialEditTags into a new EditchangetagsAction class.
  • Remove SpecialPageAction and associated logic.
  • Preserve SpecialRevisionDelete and SpecialEditTags as unlisted redirecting specials.

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.