r91871 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r91870‎ | r91871 | r91872 >
Date:10:57, 11 July 2011
Author:diebuche
Status:reverted (Comments)
Tags:bug27930 
Comment:
Add action-* class to body. Bug 4438. Based on patch by Subfader
Modified paths:
  • /trunk/phase3/includes/Skin.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Skin.php
——@@ -473,6 +473,7 @@
474474 * @return String
475475 */
476476 function getPageClasses( $title ) {
 477+ global $wgRequest;
477478 $numeric = 'ns-' . $title->getNamespace();
478479
479480 if ( $title->getNamespace() == NS_SPECIAL ) {
——@@ -491,8 +492,13 @@
492493 }
493494
494495 $name = Sanitizer::escapeClass( 'page-' . $title->getPrefixedText() );
495 -
496 - return "$numeric $type $name";
 496+
 497+ if ( $wgRequest->getVal('action') ) {
 498+ $action = 'action-' . $wgRequest->getVal('action');
 499+ } else {
 500+ $action = 'action-view';
 501+ }
 502+ return "$numeric $type $name $action";
497503 }
498504
499505 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r91942Followup r91871 per CR. Write back the actually executed action to . Ping r57415diebuche09:42, 12 July 2011
r91949Revert r91942, r91943 & reimplement. wgActions doesn't contain all possible a...diebuche11:53, 12 July 2011
r94131Reverted r91871 per CR: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/......aaron18:36, 9 August 2011
r108345[Skin] Add CSS hook for action ("action-.." class on body)...krinkle01:49, 8 January 2012

Comments

#Comment by Brion VIBBER (talk | contribs)   00:11, 12 July 2011

Doesn't validate input; while the output will be HTML-escaped, it's a bit awkward and could inject classes to the body on the resulting error page (eg if the parameter includes spaces). For instance try ?action=fake%20error -- you end up with bigger text on the page because the generic 'error' class bumps up the font size on the body.

The same full var gets re-exported to JS as wgAction as well (pre-existing code elsewhere); while the construction of the vars is fine and it's all escaped there, I suspect we should be forcing a placeholder action for the error page so it's always a predictable value/format.

A fix that updates the actual action parameter in the request if it's bogus before running output should fix both of these; or we could start storing it more explicitly in the request context or such, where we can have a clean chance to change it like we change the title when handling view redirects.

#Comment by Brion VIBBER (talk | contribs)   17:58, 12 July 2011

r94942, r91949 handle this to my satisfaction; the couple specific switch hacks we have now update the value to the one we interpret, and invalid values get replaced with a safe placeholder.

In the long run we might want to move the normalized action into the context data rather than changing the thingy directly, but it'll all be going through the same place now mostly. :)

#Comment by Krinkle (talk | contribs)   20:56, 12 July 2011

r94942 Did I miss something :O ?

#Comment by Brion VIBBER (talk | contribs)   21:19, 12 July 2011

ahem make that r91942 :D

#Comment by Krinkle (talk | contribs)   11:01, 12 July 2011

There is a reason that bug depends on bug 27930 which needs fixing first. we don't want to end up with yet-another-inconstitent representation of the current action.

Status & tagging log