r87340 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r87339‎ | r87340 | r87341 >
Date:17:23, 3 May 2011
Author:ialex
Status:resolved (Comments)
Tags:
Comment:
Moved "printfooter" and debug HTML away from "bodytext" so that they can be easily modified by extensions with the SkinTemplateOutputPageBeforeExec hook; always moved generation of debug HTML just before executing the hook so that it includes more items
Modified paths:
  • /trunk/extensions/skins/Daddio/Daddio.class.php (modified) (history)
  • /trunk/phase3/includes/SkinLegacy.php (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/skins/Modern.php (modified) (history)
  • /trunk/phase3/skins/MonoBook.php (modified) (history)
  • /trunk/phase3/skins/Vector.php (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/Vector.php
@@ -177,6 +177,16 @@
178178 <!-- bodytext -->
179179 <?php $this->html( 'bodytext' ) ?>
180180 <!-- /bodytext -->
 181+ <?php if ( $this->data['printfooter'] ): ?>
 182+ <!-- printfooter -->
 183+ <div class="printfooter">
 184+ <?php $this->html( 'printfooter' ); ?>
 185+ </div>
 186+ <!-- /printfooter -->
 187+ <?php endif; ?>
 188+ <!-- debughtml -->
 189+ <?php $this->html('debughtml'); ?>
 190+ <!-- /debughtml -->
181191 <?php if ( $this->data['catlinks'] ): ?>
182192 <!-- catlinks -->
183193 <?php $this->html( 'catlinks' ); ?>
Index: trunk/phase3/skins/MonoBook.php
@@ -91,6 +91,8 @@
9292 <?php } ?>
9393 <!-- start content -->
9494 <?php $this->html('bodytext') ?>
 95+ <?php if($this->data['printfooter']) { ?><div class="printfooter"><?php $this->html('printfooter'); ?></div><?php } ?>
 96+ <?php $this->html('debughtml'); ?>
9597 <?php if($this->data['catlinks']) { $this->html('catlinks'); } ?>
9698 <!-- end content -->
9799 <?php if($this->data['dataAfterContent']) { $this->html ('dataAfterContent'); } ?>
Index: trunk/phase3/skins/Modern.php
@@ -85,6 +85,8 @@
8686 <?php if($this->data['showjumplinks']) { ?><div id="jump-to-nav"><?php $this->msg('jumpto') ?> <a href="#mw_portlets"><?php $this->msg('jumptonavigation') ?></a>, <a href="#searchInput"><?php $this->msg('jumptosearch') ?></a></div><?php } ?>
8787
8888 <?php $this->html('bodytext') ?>
 89+ <?php if($this->data['printfooter']) { ?><div class="printfooter"><?php $this->html('printfooter'); ?></div><?php } ?>
 90+ <?php $this->html('debughtml'); ?>
8991 <div class='mw_clear'></div>
9092 <?php if($this->data['catlinks']) { $this->html('catlinks'); } ?>
9193 <?php $this->html ('dataAfterContent') ?>
Index: trunk/phase3/includes/SkinLegacy.php
@@ -68,6 +68,10 @@
6969 $this->html( 'headelement' );
7070 echo $this->beforeContent();
7171 $this->html( 'bodytext' );
 72+ echo '<div class=\"printfooter\">';
 73+ $this->html( 'printfooter' );
 74+ echo '</div>';
 75+ $this->html( 'debughtml' );
7276 echo "\n";
7377 echo $this->afterContent();
7478 $this->html( 'dataAfterContent' );
Index: trunk/phase3/includes/SkinTemplate.php
@@ -452,15 +452,13 @@
453453 $tpl->set( 'reporttime', wfReportTime() );
454454 $tpl->set( 'sitenotice', $this->getSiteNotice() );
455455 $tpl->set( 'bottomscripts', $this->bottomScripts( $out ) );
 456+ $tpl->set( 'printfooter', $this->printSource() );
456457
457 - // @todo Give printfooter userlangattributes
458 - $printfooter = "<div class=\"printfooter\">\n" . $this->printSource() . "</div>\n";
459458 global $wgBetterDirectionality;
460459 if ( $wgBetterDirectionality ) {
461460 $realBodyAttribs = array( 'lang' => $wgLanguageCode, 'dir' => $wgContLang->getDir() );
462461 $out->mBodytext = Html::rawElement( 'div', $realBodyAttribs, $out->mBodytext );
463462 }
464 - $out->mBodytext .= $printfooter . $this->generateDebugHTML( $out );
465463 $tpl->setRef( 'bodytext', $out->mBodytext );
466464
467465 # Language links
@@ -508,6 +506,8 @@
509507 $tpl->set( 'headscripts', $out->getScript() );
510508 }
511509
 510+ $tpl->set( 'debughtml', $this->generateDebugHTML( $out ) );
 511+
512512 // original version by hansm
513513 if( !wfRunHooks( 'SkinTemplateOutputPageBeforeExec', array( &$this, &$tpl ) ) ) {
514514 wfDebug( __METHOD__ . ": Hook SkinTemplateOutputPageBeforeExec broke outputPage execution!\n" );
Index: trunk/extensions/skins/Daddio/Daddio.class.php
@@ -99,6 +99,8 @@
100100 <?php if($this->data['showjumplinks']) { ?><div id="jump-to-nav"><?php $this->msg('jumpto') ?> <a href="#column-one"><?php $this->msg('jumptonavigation') ?></a>, <a href="#searchInput"><?php $this->msg('jumptosearch') ?></a></div><?php } ?>
101101
102102 <?php $this->html('bodytext') ?>
 103+ <?php if(isset($this->data['printfooter'])) { ?><div class="printfooter"><?php $this->html('printfooter'); ?></div><?php } ?>
 104+ <?php if(isset($this->data['debughtml'])) { $this->html('debughtml'); } ?>
103105 <div class='mw_clear'></div>
104106 <?php if($this->data['catlinks']) { $this->html('catlinks'); } ?>
105107 <?php $this->html('dataAfterContent') ?>

Follow-up revisions

RevisionCommit summaryAuthorDate
r87749Fix fixme: r87340: Remove slashes from double quote escape in single quote st...krinkle17:51, 9 May 2011
r96217Followup r87340: Post-hook swap bodytext into a new bodycontent key and appen...dantman12:38, 4 September 2011

Comments

#Comment by Krinkle (talk | contribs)   17:50, 9 May 2011

"printfooter" in SkinLegacy.php is escaped in single quotes. Causes slashes to end up in the actual HTML:

<div class=\"printfooter\">Retrieved from "<a href="http:
#Comment by Krinkle (talk | contribs)   17:51, 9 May 2011

Fixed in r87749.

#Comment by Brion VIBBER (talk | contribs)   20:25, 7 June 2011

Looks ok other than the thing above (fixed).

#Comment by Dantman (talk | contribs)   23:22, 1 September 2011

I don't know how well this was thought out. It means that every 3rd party skin already using bodytext needs to be updated to add printfooter and debughtml after it to opt-back-in to the same behaviour they had before.

This seams like a change that would have been better done by creating a replacement for bodytext, and combining the 3 keys back into bodytext after or before the hook (not sure which is best). Then skins can opt-in to controlling where things go by using the new messages, but extensions will still get most of the same behaviour even if the skin is still using the same bodytext as before.

Status & tagging log