r109223 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r109222‎ | r109223 | r109224 >
Date:21:49, 17 January 2012
Author:krinkle
Status:deferred (Comments)
Tags:bug27930 
Comment:
[Actions] Move action logic out of MediaWiki::getAction/MediaWiki::performAction into Action::getActionName.
* Follows-up r109195
* Reverts/Redoes r108342, r108343, r108345

* Contributes to solution of bug 27930 - Ability to get current action (The Right Way)
Modified paths:
  • /trunk/phase3/includes/Action.php (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/Wiki.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
@@ -2380,7 +2380,7 @@
23812381 * @return String: The doctype, opening <html>, and head element.
23822382 */
23832383 public function headElement( Skin $sk, $includeStyle = true ) {
2384 - global $wgContLang, $mediaWiki;
 2384+ global $wgContLang;
23852385
23862386 $userdir = $this->getLanguage()->getDir();
23872387 $sitedir = $wgContLang->getDir();
@@ -2426,7 +2426,7 @@
24272427 }
24282428 $bodyAttrs['class'] .= ' ' . $sk->getPageClasses( $this->getTitle() );
24292429 $bodyAttrs['class'] .= ' skin-' . Sanitizer::escapeClass( $sk->getSkinName() );
2430 - $bodyAttrs['class'] .= ' action-' . Sanitizer::escapeClass( $mediaWiki->getPerformedAction() );
 2430+ $bodyAttrs['class'] .= ' action-' . Sanitizer::escapeClass( Action::getActionName( $this->getContext() ) );
24312431
24322432 $sk->addToBodyAttributes( $this, $bodyAttrs ); // Allow skins to add body attributes they need
24332433 wfRunHooks( 'OutputPageBodyAttributes', array( $this, $sk, &$bodyAttrs ) );
@@ -2828,7 +2828,7 @@
28292829 * @return array
28302830 */
28312831 public function getJSVars() {
2832 - global $wgUseAjax, $wgEnableMWSuggest, $mediaWiki;
 2832+ global $wgUseAjax, $wgEnableMWSuggest;
28332833
28342834 $title = $this->getTitle();
28352835 $ns = $title->getNamespace();
@@ -2864,7 +2864,7 @@
28652865 'wgCurRevisionId' => $title->getLatestRevID(),
28662866 'wgArticleId' => $title->getArticleId(),
28672867 'wgIsArticle' => $this->isArticle(),
2868 - 'wgAction' => $mediaWiki->getPerformedAction(),
 2868+ 'wgAction' => Action::getActionName( $this->getContext() ),
28692869 'wgUserName' => $this->getUser()->isAnon() ? null : $this->getUser()->getName(),
28702870 'wgUserGroups' => $this->getUser()->getEffectiveGroups(),
28712871 'wgCategories' => $this->getCategories(),
Index: trunk/phase3/includes/Wiki.php
@@ -34,11 +34,6 @@
3535 private $context;
3636
3737 /**
38 - * @var string
39 - */
40 - private $performedAction = 'nosuchaction';
41 -
42 - /**
4338 * @param $x null|WebRequest
4439 * @return WebRequest
4540 */
@@ -81,6 +76,7 @@
8277 $request = $this->context->getRequest();
8378 $curid = $request->getInt( 'curid' );
8479 $title = $request->getVal( 'title' );
 80+ $action = $request->getVal( 'action', 'view' );
8581
8682 if ( $request->getCheck( 'search' ) ) {
8783 // Compatibility with old search URLs which didn't use Special:Search
@@ -90,7 +86,7 @@
9187 } elseif ( $curid ) {
9288 // URLs like this are generated by RC, because rc_title isn't always accurate
9389 $ret = Title::newFromID( $curid );
94 - } elseif ( $title == '' && $this->getAction() != 'delete' ) {
 90+ } elseif ( $title == '' && $action != 'delete' ) {
9591 $ret = Title::newMainPage();
9692 } else {
9793 $ret = Title::newFromURL( $title );
@@ -310,40 +306,17 @@
311307 }
312308
313309 /**
314 - * Returns the action that will be executed, not necessarily the one passed
315 - * passed through the "action" parameter. Actions disabled in
316 - * $wgDisabledActions will be replaced by "nosuchaction".
 310+ * Returns the name of the action that will be executed.
317311 *
318 - * The return value is merely a suggestion, not the actually performed action,
319 - * which may be different. The actually performed action is determined by performAction().
320 - * Requests like action=nonsense will make this function return "nonsense".
321 - * Use getPerformedAction() to get the performed action.
322 - *
323312 * @return string: action
324313 */
325314 public function getAction() {
326 - global $wgDisabledActions;
327 -
328 - $request = $this->context->getRequest();
329 - $action = $request->getVal( 'action', 'view' );
330 -
331 - // Check for disabled actions
332 - if ( in_array( $action, $wgDisabledActions ) ) {
333 - return 'nosuchaction';
 315+ static $action = null;
 316+
 317+ if ( $action === null ) {
 318+ $action = Action::getActionName( $this->context );
334319 }
335320
336 - // Workaround for bug #20966: inability of IE to provide an action dependent
337 - // on which submit button is clicked.
338 - if ( $action === 'historysubmit' ) {
339 - if ( $request->getBool( 'revisiondelete' ) ) {
340 - return 'revisiondelete';
341 - } else {
342 - return 'view';
343 - }
344 - } elseif ( $action == 'editredlink' ) {
345 - return 'edit';
346 - }
347 -
348321 return $action;
349322 }
350323
@@ -510,14 +483,12 @@
511484
512485 $action = Action::factory( $act, $article );
513486 if ( $action instanceof Action ) {
514 - $this->performedAction = $act;
515487 $action->show();
516488 wfProfileOut( __METHOD__ );
517489 return;
518490 }
519491
520492 if ( wfRunHooks( 'UnknownAction', array( $act, $article ) ) ) {
521 - $this->performedAction = 'nosuchaction';
522493 $output->showErrorPage( 'nosuchaction', 'nosuchactiontext' );
523494 }
524495
@@ -525,18 +496,6 @@
526497 }
527498
528499 /**
529 - * Returns the real action as determined by performAction.
530 - * Do not use internally in this class as it depends on the actions by this class.
531 - *
532 - * @since 1.19
533 - *
534 - * @return string: action
535 - */
536 - public function getPerformedAction() {
537 - return $this->performedAction;
538 - }
539 -
540 - /**
541500 * Run the current MediaWiki instance
542501 * index.php just calls this
543502 */
Index: trunk/phase3/includes/Action.php
@@ -87,6 +87,45 @@
8888 }
8989
9090 /**
 91+ * Get the action that will be executed, not necessarily the one passed
 92+ * passed through the "action" request parameter. Actions disabled in
 93+ * $wgDisabledActions will be replaced by "nosuchaction".
 94+ *
 95+ * @param $context IContextSource
 96+ * @return string: action name
 97+ */
 98+ public final static function getActionName( IContextSource $context ) {
 99+ global $wgDisabledActions;
 100+
 101+ $request = $context->getRequest();
 102+ $actionName = $request->getVal( 'action', 'view' );
 103+
 104+ // Check for disabled actions
 105+ if ( in_array( $actionName, $wgDisabledActions ) ) {
 106+ $actionName = 'nosuchaction';
 107+ }
 108+
 109+ // Workaround for bug #20966: inability of IE to provide an action dependent
 110+ // on which submit button is clicked.
 111+ if ( $actionName === 'historysubmit' ) {
 112+ if ( $request->getBool( 'revisiondelete' ) ) {
 113+ $actionName = 'revisiondelete';
 114+ } else {
 115+ $actionName = 'view';
 116+ }
 117+ } elseif ( $actionName == 'editredlink' ) {
 118+ $actionName = 'edit';
 119+ }
 120+
 121+ $action = Action::factory( $actionName, $context->getWikiPage() );
 122+ if ( $action instanceof Action ) {
 123+ return $action->getName();
 124+ }
 125+
 126+ return 'nosuchaction';
 127+ }
 128+
 129+ /**
91130 * Check if a given action is recognised, even if it's disabled
92131 *
93132 * @param $name String: name of an action

Follow-up revisions

RevisionCommit summaryAuthorDate
r109243Reverted r109223 per CRaaron23:07, 17 January 2012
r109678Reinstate r109223 per CR + fixes...krinkle06:57, 21 January 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r108342Implement MediaWiki::getPerformedAction()...krinkle01:31, 8 January 2012
r108343[mw.config] wgAction shouldn't use direct URL values...krinkle01:34, 8 January 2012
r108345[Skin] Add CSS hook for action ("action-.." class on body)...krinkle01:49, 8 January 2012
r109195[Actions] Move the remaining actions out of MediaWiki::performAction into sin...krinkle19:56, 17 January 2012

Comments

#Comment by Raymond (talk | contribs)   22:15, 17 January 2012

Seen on Translatewiki:

2012-01-17 22:11:38 mediawiki-bw_: /wiki/index.php?title=Special:UserLogin&type=signup Exception from line 75 of /www/w/includes/WikiPage.php: Invalid or virtual namespace -1 given.
2012-01-17 22:12:22 mediawiki-bw_: /wiki/Special:Contributions/Delhovlyn Exception from line 75 of /www/w/includes/WikiPage.php: Invalid or virtual namespace -1 given.
2012-01-17 22:13:09 mediawiki-bw_: /w/i.php?title=Special:RecentChanges&translations=only&trailer=/fi Exception from line 75 of /www/w/includes/WikiPage.php: Invalid or virtual namespace -1 given.
2012-01-17 22:13:37 mediawiki-bw_: /w/i.php?title=Special:UserLogin&returnto=Translating%3AMediaWiki&returntoquery=action%3Dhistory Exception from line 75 of /www/w/includes/WikiPage.php: Invalid or virtual namespace -1 given.
#Comment by Krinkle (talk | contribs)   23:25, 19 January 2012

Uh ? That looks like a bigger problem. SpecialPage extends from WikiPage, not ?

#Comment by Krinkle (talk | contribs)   23:35, 19 January 2012

Okay apparently not.

The exceptions are caused by the call added on this line:

+		$action = Action::factory( $actionName, $context->getWikiPage() );

@IAlex/Aaron: This is imho really a bigger issue in general. Any suggestions ? Aside from the bigger problem, as far as "action" is concerned, NS_SPECIAL should always yield action=view.

#Comment by Krinkle (talk | contribs)   06:46, 21 January 2012

Okay, chatter on IRC with Aaron resulted in that this revision can be re-instated. But we need to use $context->canUseWikiPage() before calling $context->getWikiPage() so that if the current context is in NS_SPECIAL, we won't try to fabricate a WikiPage object, which cases the exceptions to be thrown as shown above by Raymond.

#Comment by Krinkle (talk | contribs)   00:25, 24 January 2012

Marking deferred as the revert is undone and to be reviewed as r109678

Status & tagging log