r78023 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r78022‎ | r78023 | r78024 >
Date:22:27, 7 December 2010
Author:tparscal
Status:reverted (Comments)
Tags:
Comment:
Improves on r77693 by placing ResourceLoader "only styles" CSS links together with the other CSS links in buildCssLink rather than getHeadItems which is used for meta information.
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
——@@ -2231,8 +2231,8 @@
22322232 $ret .= Html::element( 'title', null, $this->getHTMLTitle() ) . "\n";
22332233
22342234 $ret .= implode( "\n", array(
2235 - $this->getHeadLinks( $sk ),
2236 - $this->buildCssLinks(),
 2235+ $this->getHeadLinks(),
 2236+ $this->buildCssLinks( $sk ),
22372237 $this->getHeadItems(),
22382238 ) );
22392239 if ( $sk->usercss ) {
——@@ -2517,7 +2517,7 @@
25182518 /**
25192519 * @return string HTML tag links to be put in the header.
25202520 */
2521 - public function getHeadLinks( Skin $sk ) {
 2521+ public function getHeadLinks() {
25222522 global $wgFeed;
25232523
25242524 // Ideally this should happen earlier, somewhere. :P
——@@ -2588,19 +2588,6 @@
25892589 }
25902590 }
25912591
2592 - // Split the styles into three groups
2593 - $styles = array( 'other' => array(), 'user' => array(), 'site' => array() );
2594 - $resourceLoader = $this->getResourceLoader();
2595 - foreach ( $this->getModuleStyles() as $name ) {
2596 - $group = $resourceLoader->getModule( $name )->getGroup();
2597 - // Modules in groups named "other" or anything different than "user" or "site" will
2598 - // be placed in the "other" group
2599 - $styles[isset( $style[$group] ) ? $group : 'other'][] = $name;
2600 - }
2601 - // Add styles to tags, user modules last
2602 - $tags[] = $this->makeResourceLoaderLink(
2603 - $sk, array_merge( $styles['other'], $styles['site'], $styles['user'] ), 'styles'
2604 - );
26052592 return implode( "\n", $tags );
26062593 }
26072594
——@@ -2658,8 +2645,23 @@
26592646 * Build a set of <link>s for the stylesheets specified in the $this->styles array.
26602647 * These will be applied to various media & IE conditionals.
26612648 */
2662 - public function buildCssLinks() {
2663 - return implode( "\n", $this->buildCssLinksArray() );
 2649+ public function buildCssLinks( $sk ) {
 2650+ // Split the styles into three groups
 2651+ $styles = array( 'other' => array(), 'user' => array(), 'site' => array() );
 2652+ $resourceLoader = $this->getResourceLoader();
 2653+ foreach ( $this->getModuleStyles() as $name ) {
 2654+ $group = $resourceLoader->getModule( $name )->getGroup();
 2655+ // Modules in groups named "other" or anything different than "user" or "site" will
 2656+ // be placed in the "other" group
 2657+ $styles[isset( $style[$group] ) ? $group : 'other'][] = $name;
 2658+ }
 2659+ // Add tags created using legacy methods
 2660+ $tags = $this->buildCssLinksArray();
 2661+ // Add ResourceLoader module style tags
 2662+ $tags[] = $this->makeResourceLoaderLink(
 2663+ $sk, array_merge( $styles['other'], $styles['site'], $styles['user'] ), 'styles'
 2664+ );
 2665+ return implode( "\n", $tags );
26642666 }
26652667
26662668 public function buildCssLinksArray() {

Follow-up revisions

RevisionCommit summaryAuthorDate
r78027Fix for bug 26128. Also needs r78023 and r78018pdhanda22:47, 7 December 2010
r78802Followup r78023, change usage of $style to $styles, not introduced in this re...reedy16:16, 22 December 2010
r78933Revert r78023 and its follow-up r78802: change seems to have been made for so...catrope23:26, 23 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r77693Resolves bug #26131 by always placing user scripts and styles at the very end...tparscal23:59, 3 December 2010

Comments

#Comment by Catrope (talk | contribs)   19:52, 8 December 2010 
-	public function getHeadLinks( Skin $sk ) {
+	public function getHeadLinks() {
[...]
-	public function buildCssLinks() {
+	public function buildCssLinks( $sk ) {

Grep for callers when you change function signatures. Both of these functions are called by SkinTemplate.php 

+		// Add tags created using legacy methods
+		$tags = $this->buildCssLinksArray();
+		// Add ResourceLoader module style tags
+		$tags[] = $this->makeResourceLoaderLink(
+			$sk, array_merge( $styles['other'], $styles['site'], $styles['user'] ), 'styles'
+		);

This adds legacy stuff before RL stuff; did it work this way previously? Is this desirable?

#Comment by Tbleher (talk | contribs)   10:32, 12 December 2010

The changed function signature breaks the DumpHTML extension: 

Warning: Missing argument 1 for OutputPage::buildCssLinks(), called in /srv/www/mediawiki/code/includes/SkinTemplate.php on line 222 and defined in /srv/www/mediawiki/code/includes/OutputPage.php on line 2633
Catchable fatal error: Argument 1 passed to OutputPage::makeResourceLoaderLink() must be an instance of Skin, null given, called in /srv/www/mediawiki/code/includes/OutputPage.php on line 2648 and defined in /srv/www/mediawiki/code/includes/OutputPage.php on line 2295
#Comment by Tbleher (talk | contribs)   11:03, 12 December 2010

Proposed trivial fix (tested with DumpHTML): 

--- a/includes/SkinTemplate.php
+++ b/includes/SkinTemplate.php
@@ -219,7 +219,7 @@ class SkinTemplate extends Skin {
                        $tpl->set( 'xhtmlnamespaces', $wgXhtmlNamespaces );
                        $tpl->set( 'html5version', $wgHtml5Version );
                        $tpl->set( 'headlinks', $out->getHeadLinks( $this ) );
-                       $tpl->set( 'csslinks', $out->buildCssLinks() );
+                       $tpl->set( 'csslinks', $out->buildCssLinks( $this ) );
 
                        if( $wgUseTrackbacks && $out->isArticleRelated() ) {
                                $tpl->set( 'trackbackhtml', $out->getTitle()->trackbackRDF() );
#Comment by Trevor Parscal (WMF) (talk | contribs)   15:43, 13 December 2010

I would love some help with this - what I was going for is that we need to add user and site CSS last. This moves the CSS links to a more sane place (not int the getHeadItems method anymore) but perhaps fails to quite hit the target, and in the mean time modifies a method signature in a way that can cause other side-effects.

Status & tagging log