MediaWiki r86131 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:< r86130‎ | r86131 (on ViewVC)‎ | r86132 >
Date:18:40, 15 April 2011
Author:aaron
Status:ok (Comments)
Tags:
Comment:
* Pass around parser options instead of users and made some parser options consistency fixes
* Moved makeParserOptions to Article.php
* Renamed currentIncludeVersions -> getRevIncludes
* Renamed updatePageCache -> setPageCache
* Moved FlaggedRevs::getCacheKey up
Modified paths:

Diff [purge]

Index: trunk/phase3/includes/Article.php
——@@ -4262,23 +4262,30 @@
42634263
42644264 /**
42654265 * Get parser options suitable for rendering the primary article wikitext
4266 - * @return mixed ParserOptions object or boolean false
 4266+ * @return ParserOptions object
42674267 */
42684268 public function getParserOptions() {
42694269 global $wgUser;
4270 -
42714270 if ( !$this->mParserOptions ) {
4272 - $this->mParserOptions = new ParserOptions( $wgUser );
4273 - $this->mParserOptions->setTidy( true );
4274 - $this->mParserOptions->enableLimitReport();
 4271+ $this->mParserOptions = $this->makeParserOptions( $wgUser );
42754272 }
4276 -
4277 - // Clone to allow modifications of the return value without affecting
4278 - // the cache
 4273+ // Clone to allow modifications of the return value without affecting cache
42794274 return clone $this->mParserOptions;
42804275 }
42814276
42824277 /**
 4278+ * Get parser options suitable for rendering the primary article wikitext
 4279+ * @param User $user
 4280+ * @return ParserOptions
 4281+ */
 4282+ public function makeParserOptions( User $user ) {
 4283+ $options = ParserOptions::newFromUser( $user );
 4284+ $options->enableLimitReport(); // show inclusion/loop reports
 4285+ $options->setTidy( true ); // fix bad HTML
 4286+ return $options;
 4287+ }
 4288+
 4289+ /**
42834290 * Updates cascading protections
42844291 *
42854292 * @param $parserOutput mixed ParserOptions object, or boolean false
Index: trunk/extensions/FlaggedRevs/dataclasses/FlaggedRevs.class.php
——@@ -513,9 +513,10 @@
514514 * @param Title $title
515515 * @param string $text wikitext
516516 * @param int $id Source revision Id
 517+ * @param ParserOptions $pOpts
517518 * @return array( string wikitext, array of template versions )
518519 */
519 - public static function expandText( Title $title, $text, $id ) {
 520+ public static function expandText( Title $title, $text, $id, ParserOptions $pOpts ) {
520521 global $wgParser;
521522 # Notify Parser if includes should be stabilized
522523 $resetManager = false;
——@@ -531,8 +532,7 @@
532533 }
533534 }
534535 }
535 - $options = self::makeParserOptions(); // default options
536 - $outputText = $wgParser->preprocess( $text, $title, $options, $id );
 536+ $outputText = $wgParser->preprocess( $text, $title, $pOpts, $id );
537537 $pOutput = $wgParser->getOutput();
538538 # Stable parse done!
539539 if ( $resetManager ) {
——@@ -547,9 +547,10 @@
548548 * @param Title $title
549549 * @param string $text
550550 * @param int $id Source revision Id
 551+ * @param ParserOptions $pOpts
551552 * @return ParserOutput
552553 */
553 - public static function parseStableText( Title $title, $text, $id, $parserOptions ) {
 554+ public static function parseStableText( Title $title, $text, $id, ParserOptions $pOpts ) {
554555 global $wgParser;
555556 # Notify Parser if includes should be stabilized
556557 $resetManager = false;
——@@ -566,7 +567,7 @@
567568 }
568569 }
569570 # Parse the new body, wikitext -> html
570 - $parserOut = $wgParser->parse( $text, $title, $parserOptions, true, true, $id );
 571+ $parserOut = $wgParser->parse( $text, $title, $pOpts, true, true, $id );
571572 # Stable parse done!
572573 if ( $resetManager ) {
573574 $incManager->clear(); // reset the FRInclusionManager as needed
——@@ -575,28 +576,21 @@
576577 }
577578
578579 /**
579 - * Get standard parser options
580 - * @param User $user (optional)
581 - * @return ParserOptions
582 - */
583 - public static function makeParserOptions( $user = null ) {
584 - global $wgUser;
585 - $user = $user ? $user : $wgUser; // assume current
586 - $options = ParserOptions::newFromUser( $user );
587 - # Show inclusion/loop reports
588 - $options->enableLimitReport();
589 - # Fix bad HTML
590 - $options->setTidy( true );
591 - return $options;
 580+ * Like ParserCache::getKey() with stable-pcache instead of pcache
 581+ */
 582+ protected static function getCacheKey( ParserCache $parserCache, Article $article, $popts ) {
 583+ $key = $parserCache->getKey( $article, $popts );
 584+ $key = str_replace( ':pcache:', ':stable-pcache:', $key );
 585+ return $key;
592586 }
593587
594588 /**
595589 * Get the page cache for the stable version of an article
596590 * @param Article $article
597 - * @param User $user
 591+ * @param ParserOptions $opts
598592 * @return mixed (ParserOutput/false)
599593 */
600 - public static function getPageCache( Article $article, $user ) {
 594+ public static function getPageCache( Article $article, ParserOptions $popts ) {
601595 global $parserMemc, $wgCacheEpoch;
602596 wfProfileIn( __METHOD__ );
603597 # Make sure it is valid
——@@ -605,7 +599,7 @@
606600 return null;
607601 }
608602 $parserCache = ParserCache::singleton();
609 - $key = self::getCacheKey( $parserCache, $article, $user );
 603+ $key = self::getCacheKey( $parserCache, $article, $popts );
610604 # Get the cached HTML
611605 wfDebug( "Trying parser cache $key\n" );
612606 $value = $parserMemc->get( $key );
——@@ -638,25 +632,13 @@
639633 }
640634
641635 /**
642 - * Like ParserCache::getKey() with stable-pcache instead of pcache
643 - */
644 - protected static function getCacheKey( $parserCache, Article $article, $popts ) {
645 - if( $popts instanceof User ) {
646 - $popts = ParserOptions::newFromUser( $popts );
647 - }
648 - $key = $parserCache->getKey( $article, $popts );
649 - $key = str_replace( ':pcache:', ':stable-pcache:', $key );
650 - return $key;
651 - }
652 -
653 - /**
654636 * @param Article $article
655637 * @param ParserOptions $popts
656638 * @param parserOutput $parserOut
657639 * Updates the stable cache of a page with the given $parserOut
658640 */
659 - public static function updatePageCache(
660 - Article $article, $popts, ParserOutput $parserOut = null
 641+ public static function setPageCache(
 642+ Article $article, ParserOptions $popts, ParserOutput $parserOut = null
661643 ) {
662644 global $parserMemc, $wgParserCacheExpireTime, $wgEnableParserCache;
663645 wfProfileIn( __METHOD__ );
Index: trunk/extensions/FlaggedRevs/api/actions/ApiReview.php
——@@ -69,7 +69,7 @@
7070 $article = new FlaggedPage( $title );
7171 // Now get the template and image parameters needed
7272 list( $templateIds, $fileTimeKeys ) =
73 - RevisionReviewForm::currentIncludeVersions( $article, $rev );
 73+ RevisionReviewForm::getRevIncludes( $article, $rev, $wgUser );
7474 // Get version parameters for review submission (flat strings)
7575 list( $templateParams, $imageParams, $fileVersion ) =
7676 RevisionReviewForm::getIncludeParams( $article, $templateIds, $fileTimeKeys );
Index: trunk/extensions/FlaggedRevs/business/RevisionReviewForm.php
——@@ -575,29 +575,29 @@
576576 * Get template and image versions from parsing a revision.
577577 * @param Article $article
578578 * @param Revision $rev
 579+ * @param User $user
579580 * @return array( templateIds, fileSHA1Keys )
580581 * templateIds like ParserOutput->mTemplateIds
581582 * fileSHA1Keys like ParserOutput->mImageTimeKeys
582583 */
583 - public static function currentIncludeVersions( Article $article, Revision $rev ) {
 584+ public static function getRevIncludes( Article $article, Revision $rev, User $user ) {
584585 global $wgParser, $wgOut, $wgEnableParserCache;
585586 wfProfileIn( __METHOD__ );
586587 $pOutput = false;
 588+ $pOpts = $article->makeParserOptions( $user );
 589+ $parserCache = ParserCache::singleton();
587590 # Current version: try parser cache
588591 if ( $rev->isCurrent() ) {
589 - $parserCache = ParserCache::singleton();
590 - $pOutput = $parserCache->get( $article, $wgOut->parserOptions() );
 592+ $pOutput = $parserCache->get( $article, $pOpts );
591593 }
592594 # Otherwise (or on cache miss), parse the rev text...
593595 if ( !$pOutput ) {
594596 $text = $rev->getText();
595597 $title = $article->getTitle();
596 - $options = FlaggedRevs::makeParserOptions();
597 - $pOutput = $wgParser->parse(
598 - $text, $title, $options, true, true, $rev->getId() );
 598+ $pOutput = $wgParser->parse( $text, $title, $pOpts, true, true, $rev->getId() );
599599 # Might as well save the cache while we're at it
600600 if ( $rev->isCurrent() && $wgEnableParserCache ) {
601 - $parserCache->save( $pOutput, $article, $options );
 601+ $parserCache->save( $pOutput, $article, $pOpts );
602602 }
603603 }
604604 wfProfileOut( __METHOD__ );
Index: trunk/extensions/FlaggedRevs/presentation/FlaggedPageView.php
——@@ -598,7 +598,7 @@
599599
600600 # Parse and output HTML
601601 if ( $redirHtml == '' ) {
602 - $parserOptions = FlaggedRevs::makeParserOptions();
 602+ $parserOptions = $this->article->makeParserOptions( $wgUser );
603603 $parserOut = FlaggedRevs::parseStableText(
604604 $this->article->getTitle(), $text, $frev->getRevId(), $parserOptions );
605605 $this->addParserOutput( $parserOut );
——@@ -671,7 +671,8 @@
672672 }
673673
674674 # Get parsed stable version and output HTML
675 - $parserOut = FlaggedRevs::getPageCache( $this->article, $wgUser );
 675+ $parserOptions = $this->article->makeParserOptions( $wgUser );
 676+ $parserOut = FlaggedRevs::getPageCache( $this->article, $parserOptions );
676677 if ( $parserOut ) {
677678 $this->addParserOutput( $parserOut );
678679 } else {
——@@ -681,11 +682,10 @@
682683 # Don't parse redirects, use separate handling...
683684 if ( $redirHtml == '' ) {
684685 # Get the new stable output
685 - $parserOptions = FlaggedRevs::makeParserOptions();
686686 $parserOut = FlaggedRevs::parseStableText(
687687 $this->article->getTitle(), $text, $srev->getRevId(), $parserOptions );
688688 # Update the stable version cache
689 - FlaggedRevs::updatePageCache( $this->article, $parserOptions, $parserOut );
 689+ FlaggedRevs::setPageCache( $this->article, $parserOptions, $parserOut );
690690 # Add the stable output to the page view
691691 $this->addParserOutput( $parserOut );
692692
Index: trunk/extensions/FlaggedRevs/presentation/RevisionReviewFormUI.php
——@@ -385,7 +385,7 @@
386386 # Do we need to get inclusion IDs from parser output?
387387 if ( $this->templateIDs === null || $this->imageSHA1Keys === null ) {
388388 list( $this->templateIDs, $this->imageSHA1Keys ) =
389 - RevisionReviewForm::currentIncludeVersions( $this->article, $this->rev );
 389+ RevisionReviewForm::getRevIncludes( $this->article, $this->rev, $this->user );
390390 }
391391 return array( $this->templateIDs, $this->imageSHA1Keys );
392392 }

Follow-up revisions

Rev.Commit summaryAuthorDate
r89706Reinstate r79122 (fix for bug 14404), reverting r83868. The real bug seem to ...platonides22:28, 7 June 2011

Comments

#Comment by Platonides (talk | contribs)   22:19, 7 June 2011

Why didn't you make makeParserOptions static?

#Comment by Platonides (talk | contribs)   22:22, 7 June 2011

Also, I don't see the need for creating a new method. Seems that just calling getParserOptions() would have worked for you.

#Comment by Aaron Schulz (talk | contribs)   23:29, 7 June 2011

I'd rather dependency inject the User rather than always using $wgUser.

#Comment by Platonides (talk | contribs)   14:06, 8 June 2011

In which case does your $user not match $wgUser?

#Comment by Tim Starling (talk | contribs)   06:39, 2 September 2011

A case where it does not match was introduced in r89124, and another potential use for $user not matching $wgUser was introduced in r89706.

Status & tagging log

Retrieved from "http://www.mediawiki.org/wiki/Special:Code/MediaWiki/86131"