r84619 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r84618‎ | r84619 | r84620 >
Date:18:27, 23 March 2011
Author:aaron
Status:ok (Comments)
Tags:
Comment:
Follow-up r84610: removed fr_fileSHA1Keys and use core file version tracking instead. Handles query spam FIXME in code.
Modified paths:
  • /trunk/extensions/FlaggedRevs/FRInclusionManager.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/FlaggedArticleView.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/FlaggedRevision.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/FlaggedRevs.class.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/FlaggedRevs.hooks.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/FlaggedRevs.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/api/ApiReview.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/forms/RevisionReviewForm.php (modified) (history)
  • /trunk/extensions/FlaggedRevs/maintenance/tests/FRInclusionManagerTest.php (modified) (history)

Diff [purge]

Index: trunk/extensions/FlaggedRevs/FlaggedRevs.php
——@@ -438,9 +438,6 @@
439439 $wgHooks['BeforeParserFetchTemplateAndtitle'][] = 'FlaggedRevsHooks::parserFetchStableTemplate';
440440 $wgHooks['BeforeParserMakeImageLinkObj'][] = 'FlaggedRevsHooks::parserFetchStableFile';
441441 $wgHooks['BeforeGalleryFindFile'][] = 'FlaggedRevsHooks::galleryFetchStableFile';
442 -# Additional parser versioning
443 -$wgHooks['ParserAfterTidy'][] = 'FlaggedRevsHooks::parserInjectTimestamps';
444 -$wgHooks['OutputPageParserOutput'][] = 'FlaggedRevsHooks::outputInjectTimestamps';
445442 # ########
446443
447444 # ######## DB write operations #########
——@@ -631,4 +628,4 @@
632629 }
633630
634631 # B/C ...
635 -$wgLogActions['rights/erevoke'] = 'rights-editor-revoke';
 632+$wgLogActions['rights/erevoke'] = 'rights-editor-revoke';
\ No newline at end of file
Index: trunk/extensions/FlaggedRevs/maintenance/tests/FRInclusionManagerTest.php
——@@ -8,11 +8,11 @@
99 0 => array('ZZ' => 464, '0' => 13)
1010 );
1111 protected static $inputFiles = array(
12 - 'FileXX' => array('ts' => '20100405192110', 'sha1' => 'abc1'),
13 - 'FileYY' => array('ts' => '20000403101300', 'sha1' => 1134),
14 - 'FileZZ' => array('ts' => '0', 'sha1' => ''),
15 - 'Filele' => array('ts' => 0, 'sha1' => ''),
16 - '0' => array('ts' => '20000203101350', 'sha1' => 'ae33'),
 12+ 'FileXX' => array('time' => '20100405192110', 'sha1' => 'abc1'),
 13+ 'FileYY' => array('time' => '20000403101300', 'sha1' => 1134),
 14+ 'FileZZ' => array('time' => '0', 'sha1' => ''),
 15+ 'Filele' => array('time' => 0, 'sha1' => ''),
 16+ '0' => array('time' => '20000203101350', 'sha1' => 'ae33'),
1717 );
1818 /* output to test against (<test,NS,dbkey,expected rev ID>) */
1919 protected static $reviewedOutputTemplates = array(
Index: trunk/extensions/FlaggedRevs/FlaggedRevs.class.php
——@@ -652,7 +652,7 @@
653653 /**
654654 * @param Article $article
655655 * @param parserOutput $parserOut
656 - * Updates the stable-only cache dependancy table
 656+ * Updates the stable-only cache dependency table
657657 */
658658 public static function updateCacheTracking( Article $article, ParserOutput $stableOut ) {
659659 wfProfileIn( __METHOD__ );
——@@ -1269,7 +1269,7 @@
12701270 'img_timestamp' => $fileData['timestamp'],
12711271 'img_sha1' => $fileData['sha1'],
12721272 'templateVersions' => $poutput->mTemplateIds,
1273 - 'fileVersions' => $poutput->fr_fileSHA1Keys
 1273+ 'fileVersions' => $poutput->mImageTimeKeys
12741274 ) );
12751275 $flaggedRevision->insertOn( $auto );
12761276 # Update the article review log
Index: trunk/extensions/FlaggedRevs/forms/RevisionReviewForm.php
——@@ -542,7 +542,7 @@
543543 * Get template and image parameters from parser output to use on forms.
544544 * @param FlaggedArticle $article
545545 * @param array $templateIDs (from ParserOutput/OutputPage->mTemplateIds)
546 - * @param array $imageSHA1Keys (from ParserOutput/OutputPage->fr_fileSHA1Keys)
 546+ * @param array $imageSHA1Keys (from ParserOutput/OutputPage->mImageTimeKeys)
547547 * @returns array( templateParams, imageParams, fileVersion )
548548 */
549549 public static function getIncludeParams(
——@@ -558,7 +558,7 @@
559559 }
560560 # Image -> timestamp -> sha1 mapping
561561 foreach ( $imageSHA1Keys as $dbKey => $timeAndSHA1 ) {
562 - $imageParams .= $dbKey . "|" . $timeAndSHA1['ts'];
 562+ $imageParams .= $dbKey . "|" . $timeAndSHA1['time'];
563563 $imageParams .= "|" . $timeAndSHA1['sha1'] . "#";
564564 }
565565 # For image pages, note the displayed image version
——@@ -577,7 +577,7 @@
578578 * @param string $imageParams
579579 * @returns array( templateIds, fileSHA1Keys )
580580 * templateIds like ParserOutput->mTemplateIds
581 - * fileSHA1Keys like ParserOutput->fr_fileSHA1Keys
 581+ * fileSHA1Keys like ParserOutput->mImageTimeKeys
582582 */
583583 public static function getIncludeVersions( $templateParams, $imageParams ) {
584584 $templateIds = array();
——@@ -613,15 +613,15 @@
614614 if ( !isset( $m[0] ) || !isset( $m[1] ) || !isset( $m[2] ) || !$m[0] ) {
615615 continue;
616616 }
617 - list( $dbkey, $timestamp, $key ) = $m;
 617+ list( $dbkey, $time, $key ) = $m;
618618 # Get the file title
619619 $img_title = Title::makeTitle( NS_IMAGE, $dbkey ); // Normalize
620620 if ( is_null( $img_title ) ) {
621621 continue; // Page must be valid!
622622 }
623623 $fileSHA1Keys[$img_title->getDBkey()] = array();
624 - $fileSHA1Keys[$img_title->getDBkey()]['ts'] = $timestamp;
625 - $fileSHA1Keys[$img_title->getDBkey()]['sha1'] = $key;
 624+ $fileSHA1Keys[$img_title->getDBkey()]['time'] = $time ? $time : '0';
 625+ $fileSHA1Keys[$img_title->getDBkey()]['sha1'] = $key ? $key : '';
626626 }
627627 return array( $templateIds, $fileSHA1Keys );
628628 }
——@@ -753,7 +753,7 @@
754754 $pOutput = $parserCache->get( $article, $wgOut->parserOptions() );
755755 }
756756 # Otherwise (or on cache miss), parse the rev text...
757 - if ( !$pOutput || !isset( $pOutput->fr_fileSHA1Keys ) ) {
 757+ if ( !$pOutput || !isset( $pOutput->mImageTimeKeys ) ) {
758758 $text = $rev->getText();
759759 $title = $article->getTitle();
760760 $options = FlaggedRevs::makeParserOptions();
——@@ -765,7 +765,7 @@
766766 }
767767 }
768768 $templateIDs = $pOutput->mTemplateIds;
769 - $imageSHA1Keys = $pOutput->fr_fileSHA1Keys;
 769+ $imageSHA1Keys = $pOutput->mImageTimeKeys;
770770 }
771771 list( $templateParams, $imageParams, $fileVersion ) =
772772 RevisionReviewForm::getIncludeParams( $article, $templateIDs, $imageSHA1Keys );
Index: trunk/extensions/FlaggedRevs/FlaggedRevision.php
——@@ -292,7 +292,8 @@
293293 'fi_name' => $dbkey,
294294 'fi_img_sha1' => strval( $timeSHA1['sha1'] ),
295295 // b/c: fi_img_timestamp DEFAULT either NULL (new) or '' (old)
296 - 'fi_img_timestamp' => $timeSHA1['ts'] ? $dbw->timestamp( $timeSHA1['ts'] ) : ''
 296+ 'fi_img_timestamp' => $timeSHA1['time'] ?
 297+ $dbw->timestamp( $timeSHA1['time'] ) : ''
297298 );
298299 }
299300 # Our review entry
——@@ -496,7 +497,7 @@
497498 /**
498499 * Get original template versions at time of review
499500 * @param int $flags FR_MASTER
500 - * @return Array file versions (dbKey => array('ts' => MW timestamp,'sha1' => sha1) )
 501+ * @return Array file versions (dbKey => array('time' => MW timestamp,'sha1' => sha1) )
501502 * Note: '0' used for file timestamp if it didn't exist ('' for sha1)
502503 */
503504 public function getFileVersions( $flags = 0 ) {
——@@ -513,7 +514,7 @@
514515 $reviewedTS = trim( $row->fi_img_timestamp ); // may be ''/NULL
515516 $reviewedTS = $reviewedTS ? wfTimestamp( TS_MW, $reviewedTS ) : '0';
516517 $this->mFiles[$row->fi_name] = array();
517 - $this->mFiles[$row->fi_name]['ts'] = $reviewedTS;
 518+ $this->mFiles[$row->fi_name]['time'] = $reviewedTS;
518519 $this->mFiles[$row->fi_name]['sha1'] = $row->fi_img_sha1;
519520 }
520521 }
——@@ -557,7 +558,7 @@
558559 /**
559560 * Get the current stable version of the files used at time of review
560561 * @param int $flags FR_MASTER
561 - * @return Array file versions (dbKey => array('ts' => MW timestamp,'sha1' => sha1) )
 562+ * @return Array file versions (dbKey => array('time' => MW timestamp,'sha1' => sha1) )
562563 * Note: '0' used for file timestamp if it doesn't exist ('' for sha1)
563564 */
564565 public function getStableFileVersions( $flags = 0 ) {
——@@ -587,7 +588,7 @@
588589 $reviewedSha1 = strval( $row->fr_img_sha1 );
589590 }
590591 $this->mStableFiles[$row->fi_name] = array();
591 - $this->mStableFiles[$row->fi_name]['ts'] = $reviewedTS;
 592+ $this->mStableFiles[$row->fi_name]['time'] = $reviewedTS;
592593 $this->mStableFiles[$row->fi_name]['sha1'] = $reviewedSha1;
593594 }
594595 }
——@@ -714,7 +715,7 @@
715716 if ( FlaggedRevs::inclusionSetting() == FR_INCLUDES_STABLE ) {
716717 $stableTS = wfTimestampOrNull( TS_MW, $row->fr_img_timestamp );
717718 # Select newest of (stable rev, rev when reviewed) as "version used"
718 - $tsStable = ( $stableTS >= $reviewedTS ) ? $stableTS : $reviewedTS;
 719+ $tsStable = max( $stableTS, $reviewedTS );
719720 } else {
720721 $tsStable = $reviewedTS;
721722 }
Index: trunk/extensions/FlaggedRevs/FlaggedRevs.hooks.php
——@@ -288,8 +288,7 @@
289289 * Add special fields to parser.
290290 */
291291 public static function parserAddFields( Parser $parser ) {
292 - $parser->mOutput->fr_fileSHA1Keys = array();
293 - $parser->mOutput->fr_includeErrors = array();
 292+ $parser->getOutput()->fr_includeErrors = array();
294293 return true;
295294 }
296295
——@@ -321,7 +320,7 @@
322321 }
323322 # If $id not specified, see if we are allowed to use the current revision
324323 if ( $id === false ) {
325 - $parser->mOutput->fr_includeErrors[] = $title->getPrefixedDBKey(); // unspecified
 324+ $parser->getOutput()->fr_includeErrors[] = $title->getPrefixedDBKey(); // unspecified
326325 # If $id is zero, don't bother loading it
327326 } elseif ( !$id ) {
328327 $skip = true;
——@@ -331,7 +330,7 @@
332331
333332 /**
334333 * (a) Select the desired images based on the selected stable version time/SHA-1
335 - * (b) Set specified versions in fr_fileSHA1Keys
 334+ * (b) Set specified versions in mImageTimeKeys
336335 */
337336 public static function parserFetchStableFile(
338337 $parser, Title $nt, &$skip, &$time, &$query, &$sha1
——@@ -349,7 +348,7 @@
350349 } else {
351350 $title =& $nt;
352351 }
353 - # Get version, update fr_fileSHA1Keys...
 352+ # Get version, update mImageTimeKeys...
354353 list( $time, $sha1 ) = self::parserFindStableFile( $parser, $title );
355354 # Stabilize the file link
356355 if ( $time ) {
——@@ -361,7 +360,7 @@
362361
363362 /**
364363 * (a) Select the desired images based on the selected stable version time/SHA-1
365 - * (b) Set specified versions in fr_fileSHA1Keys
 364+ * (b) Set specified versions in mImageTimeKeys
366365 */
367366 public static function galleryFetchStableFile( $ig, Title $nt, &$time, &$query, &$sha1 ) {
368367 $parser =& $ig->mParser; // convenience
——@@ -371,7 +370,7 @@
372371 if ( !FRInclusionManager::singleton()->parserOutputIsStabilized() ) {
373372 return true; // trigger for stable version parsing only
374373 }
375 - # Get version, update fr_fileSHA1Keys...
 374+ # Get version, update mImageTimeKeys...
376375 list( $time, $sha1 ) = self::parserFindStableFile( $parser, $nt );
377376 # Stabilize the file link
378377 if ( $time ) {
——@@ -383,7 +382,7 @@
384383
385384 /**
386385 * (a) Select the desired images based on the selected stable version time/SHA-1
387 - * (b) Set specified versions in fr_fileSHA1Keys
 386+ * (b) Set specified versions in mImageTimeKeys
388387 */
389388 protected static function parserFindStableFile( Parser $parser, Title $title ) {
390389 $time = false; // current version
——@@ -407,72 +406,13 @@
408407 # If $time not specified, see if we are allowed to use the current revision
409408 if ( $time === false ) {
410409 # May want to give an error, so track these...
411 - $parser->mOutput->fr_includeErrors[] = $title->getPrefixedDBKey();
 410+ $parser->getOutput()->fr_includeErrors[] = $title->getPrefixedDBKey();
412411 } elseif ( !$time ) {
413412 $time = "0"; // make sure this the string '0'
414413 }
415 - # Add specified image metadata to parser output
416 - if ( $time !== false ) {
417 - $parser->mOutput->fr_fileSHA1Keys[$title->getDBkey()] = array();
418 - $parser->mOutput->fr_fileSHA1Keys[$title->getDBkey()]['ts'] = $time;
419 - $parser->mOutput->fr_fileSHA1Keys[$title->getDBkey()]['sha1'] = $sha1;
420 - }
421414 return array( $time, $sha1 );
422415 }
423416
424 - /**
425 - * Insert image timestamps/SHA-1 keys into parser output
426 - */
427 - public static function parserInjectTimestamps( Parser $parser ) {
428 - $pOutput =& $parser->mOutput; // convenience
429 - if ( !isset( $pOutput->mImages ) ) {
430 - return true; // sanity check
431 - }
432 - # Fetch the current timestamps of the images.
433 - foreach ( $pOutput->mImages as $filename => $x ) {
434 - # Stable output with versions specified
435 - if ( isset( $pOutput->fr_fileSHA1Keys[$filename] ) ) {
436 - // Fetch file with $time to confirm the specified version exists
437 - $time = $pOutput->fr_fileSHA1Keys[$filename]['ts'];
438 - $sha1 = $pOutput->fr_fileSHA1Keys[$filename]['sha1'];
439 - # FIXME: don't double fetch these (Parser just did)!!!
440 - $file = RepoGroup::singleton()->findFileFromKey(
441 - $sha1, array( 'time' => $time ) );
442 - } else {
443 - $title = Title::makeTitleSafe( NS_FILE, $filename );
444 - # FIXME: don't double fetch these (Parser just did)!!!
445 - $file = wfFindFile( $title );
446 - }
447 - # Update tracking vars...
448 - $pOutput->fr_fileSHA1Keys[$filename] = array();
449 - if ( $file ) {
450 - $pOutput->fr_fileSHA1Keys[$filename]['ts'] = $file->getTimestamp();
451 - $pOutput->fr_fileSHA1Keys[$filename]['sha1'] = $file->getSha1();
452 - } else {
453 - $pOutput->fr_fileSHA1Keys[$filename]['ts'] = '0';
454 - $pOutput->fr_fileSHA1Keys[$filename]['sha1'] = '';
455 - }
456 - }
457 - return true;
458 - }
459 -
460 - /**
461 - * Insert image timestamps/SHA-1s into page output
462 - */
463 - public static function outputInjectTimestamps( OutputPage $out, ParserOutput $parserOut ) {
464 - # Set first time
465 - if ( !isset( $out->fr_fileSHA1Keys ) ) {
466 - $out->fr_fileSHA1Keys = array();
467 - }
468 - # Leave as defaults if missing. Relevant things will be updated only when needed.
469 - # We don't want to go around resetting caches all over the place if avoidable...
470 - $fileSHA1Keys = isset( $parserOut->fr_fileSHA1Keys ) ?
471 - $parserOut->fr_fileSHA1Keys : array();
472 - # Add on any new items
473 - $out->fr_fileSHA1Keys = wfArrayMerge( $out->fr_fileSHA1Keys, $fileSHA1Keys );
474 - return true;
475 - }
476 -
477417 public static function onParserFirstCallInit( &$parser ) {
478418 $parser->setFunctionHook( 'pagesusingpendingchanges',
479419 'FlaggedRevsHooks::parserPagesUsingPendingChanges' );
——@@ -488,9 +428,9 @@
489429 }
490430
491431 public static function onParserGetVariableValueSwitch( &$parser, &$cache, &$word, &$ret ) {
492 - if( $word == 'pendingchangelevel' ) {
 432+ if ( $word == 'pendingchangelevel' ) {
493433 $title = $parser->getTitle();
494 - if( !FlaggedRevs::inReviewNamespace( $title ) ) {
 434+ if ( !FlaggedRevs::inReviewNamespace( $title ) ) {
495435 $ret = '';
496436 } else {
497437 $config = FlaggedPageConfig::getPageStabilitySettings( $title );
——@@ -2034,4 +1974,4 @@
20351975 }
20361976 return true;
20371977 }
2038 -}
\ No newline at end of file
 1978+}
Index: trunk/extensions/FlaggedRevs/api/ApiReview.php
——@@ -72,7 +72,7 @@
7373 $parserCache = ParserCache::singleton();
7474 $parserOutput = $parserCache->get( $article, $wgOut->parserOptions() );
7575 }
76 - if ( !$parserOutput || !isset( $parserOutput->fr_fileSHA1Keys ) ) {
 76+ if ( !$parserOutput || !isset( $parserOutput->mImageTimeKeys ) ) {
7777 // Miss, we have to reparse the page
7878 $text = $article->getContent();
7979 $options = FlaggedRevs::makeParserOptions();
——@@ -82,7 +82,7 @@
8383 // Set version parameters for review submission
8484 list( $templateParams, $imageParams, $fileVersion ) =
8585 RevisionReviewForm::getIncludeParams( $article,
86 - $parserOutput->mTemplateIds, $parserOutput->fr_fileSHA1Keys );
 86+ $parserOutput->mTemplateIds, $parserOutput->mImageTimeKeys );
8787 $form->setTemplateParams( $templateParams );
8888 $form->setFileParams( $imageParams );
8989 $form->setFileVersion( $fileVersion );
Index: trunk/extensions/FlaggedRevs/FRInclusionManager.php
——@@ -39,7 +39,7 @@
4040 * (a) Stabilize inclusions in Parser output
4141 * (b) Set the template/image versions used in the flagged version of a revision
4242 * @param array $tmpParams (ns => dbKey => revId )
43 - * @param array $imgParams (dbKey => array('ts' => MW timestamp,'sha1' => sha1) )
 43+ * @param array $imgParams (dbKey => array('time' => MW timestamp,'sha1' => sha1) )
4444 */
4545 public function setReviewedVersions( array $tmpParams, array $imgParams ) {
4646 $this->reviewedVersions = array();
——@@ -50,7 +50,7 @@
5151 /**
5252 * Set the stable versions of some template/images
5353 * @param array $tmpParams (ns => dbKey => revId )
54 - * @param array $imgParams (dbKey => array('ts' => MW timestamp,'sha1' => sha1) )
 54+ * @param array $imgParams (dbKey => array('time' => MW timestamp,'sha1' => sha1) )
5555 */
5656 public function setStableVersionCache( array $tmpParams, array $imgParams ) {
5757 $this->stableVersions['templates'] = $tmpParams;
——@@ -116,7 +116,7 @@
117117 $dbKey = $title->getDBkey();
118118 # All NS_FILE, no need to check namespace
119119 if ( isset( $this->reviewedVersions['files'][$dbKey] ) ) {
120 - $time = $this->reviewedVersions['files'][$dbKey]['ts'];
 120+ $time = $this->reviewedVersions['files'][$dbKey]['time'];
121121 $sha1 = $this->reviewedVersions['files'][$dbKey]['sha1'];
122122 return array( $time, $sha1 );
123123 }
——@@ -154,7 +154,7 @@
155155 $sha1 = '';
156156 # All NS_FILE, no need to check namespace
157157 if ( isset( $this->stableVersions['files'][$dbKey] ) ) {
158 - $time = $this->stableVersions['files'][$dbKey]['ts'];
 158+ $time = $this->stableVersions['files'][$dbKey]['time'];
159159 $sha1 = $this->stableVersions['files'][$dbKey]['sha1'];
160160 return array( $time, $sha1 );
161161 }
——@@ -164,7 +164,7 @@
165165 $sha1 = $srev->getFileSha1();
166166 }
167167 $this->stableVersions['files'][$dbKey] = array();
168 - $this->stableVersions['files'][$dbKey]['ts'] = $time;
 168+ $this->stableVersions['files'][$dbKey]['time'] = $time;
169169 $this->stableVersions['files'][$dbKey]['sha1'] = $sha1;
170170 return array( $time, $sha1 );
171171 }
Index: trunk/extensions/FlaggedRevs/FlaggedArticleView.php
——@@ -1083,10 +1083,10 @@
10841084 $templateIDs = $fileSHA1Keys = null;
10851085 if ( $wgOut->getRevisionId() == $rev->getId()
10861086 && isset( $wgOut->mTemplateIds )
1087 - && isset( $wgOut->fr_fileSHA1Keys ) )
 1087+ && isset( $wgOut->mImageTimeKeys ) )
10881088 {
10891089 $templateIDs = $wgOut->mTemplateIds;
1090 - $fileSHA1Keys = $wgOut->fr_fileSHA1Keys;
 1090+ $fileSHA1Keys = $wgOut->mImageTimeKeys;
10911091 }
10921092 # Review notice box goes in top of form
10931093 $form = RevisionReviewForm::buildQuickReview(

Follow-up revisions

RevisionCommit summaryAuthorDate
r85230* Removed isset() checks - redundant given Parser::version...aaron05:53, 3 April 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r84610* Put parser output file version tracking to core...aaron17:35, 23 March 2011

Comments

#Comment by Tim Starling (talk | contribs)   05:33, 2 September 2011

Needs fixing per my comment on CR r84610.

Status & tagging log