Page MenuHomePhabricator

Getting Action name is too heavy on dependencies
Closed, DuplicatePublic


Currently in order to find out the name of the Action in Action::getActionName we need to instantiate the Action, which requires a Context and (after completing WikiPage/Article split, would require an Article). This brings in a lot of dependencies, and can also cause infinite loops with extension hooks (see T249162 for example).

Similar problems apply to Action::getRestriction.

Action needs to be instantiated cause both name and restriction are returned from Action::getName and Action::getRestriction, which are overridden by individual actions. Vast majority of actions just return a hard-coded string from these methods.

There should be a more lightweight way of resolving the action name/restriction.

Event Timeline

As a long-term solution, this would probably be addressed by introducing page types T209924

As a more short term solution, we need to verify what the individual actions return from getName method, and figure out if there are valid cases when the returned name is different from the name which the action is registered under. In case they normally don't, we can deprecate Action::getName and start entirely relying on the names actions are registered under. This will allow getting the action name without constructing the action itself.

So, a todo now is to look over all the Action::getName overrides.

Elevator pitch of why it's important

Inside MediaWiki main class, the code flow related to Action instantiation looks somewhat like this:

$this->context = REquestContext::getMain;
$this->context->setTitle( $this->parseTitle );
$act = Action::getActionName
$action = Action::factory( $act )

The last two calls actually instantiate the Action and all it's dependencies twice. Given that Action bring in quite a few dependencies, that might be pretty heavy. Additionally, once we switch Action to depend on Article instead of WikiPage, the call to Action::getActionName would instantiate Article too early in the initialization flow, which might become a problem.

Additionally, note that Action::factory is called essentially with the result of Action->getName. ActionFactory does a lookup of an appropriate implementation class within wgActions, so if an action overrides the Action::getName and returns anything other then the name it is registered under - it will break.

So, the current situation is not only pointless, but probably has some performance impact and is extremely fragile.


I have made an overview of all the Action::getName overrides, and all of them are returning the same key the action is registered under.

One exception is SpecialPageAction, however the modifications it does were added as a workaround to support IE6 in T22966, thus it can be removed now. Flow overrides action to return a private field set in constructor, but that's just to support make it simpler to override different flow actions.

The action itself needs to know it's name sometimes, most heavily used in FormAction::getForm.


  • Remove workaround for T22966 from Action::getActionName and SpecialPageAction::getName
  • Under the Stable interface policy make Action non-newable, thus deprecate and remove access to the constructor from anything except subclasses
  • Modify Action constructor signature to __construct( string $name, Article $article ) - note that we drop the context, since Article should already hold the context. This is not a part of this issue, but since we would be deprecating the constructor signature, we might as well do this. Have b/c support for old signature, deprecate it.
  • Store the $name into a private field in the class, make Action::getName non-abstract, return the name passed in the constructor. Deprecate Action::getName for overriding in subclasses
  • Remove Action subclasses getName overrides
  • Make Action::getName final.

This plan is only needed for consistency, it's possible to get the correct action name without doing all this, demonstrated in - some Action tests are failing, but those test cases actually seem misleading - if Action::getName would be overridden like this in production, it will actually break MediaWiki.

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

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

Change 858436 merged by jenkins-bot:

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