r84591 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r84590‎ | r84591 | r84592 >
Date:03:13, 23 March 2011
Author:aaron
Status:ok (Comments)
Tags:
Comment:
* Made BeforeParserMakeImageLinkObj/BeforeGalleryFindFile let hooks set sha1 parameter
* Made FlaggedRevs specify files by sha1,timestamp to handle renames with no redirects. This makes them handled as well as templates in this regard. (bug 27836)
* Moved BeforeGalleryFindFile hook to proper place (don't trigger for non-NS_FILE titles)
* Removed unused mRevisionId field from ImageGallery
* Removed old hotfix from makeMediaLinkObj(); all the current callers would crash beforehand if the title was null anyway
* Updated hook docs (some prior params were missing)
* Broke some long lines and cleaned up some whitespace
* TODO: track file info in core rather than fr_fileSHA1Keys and ugly, duplicated, queries. This should be easy to do now.
Modified paths:
  • /trunk/extensions/FlaggedRevs/FlaggedRevs.hooks.php (modified) (history)
  • /trunk/phase3/docs/hooks.txt (modified) (history)
  • /trunk/phase3/includes/ImageGallery.php (modified) (history)
  • /trunk/phase3/includes/Linker.php (modified) (history)
  • /trunk/phase3/includes/parser/Parser.php (modified) (history)

Diff [purge]

Index: trunk/phase3/docs/hooks.txt
——@@ -553,7 +553,9 @@
554554 'BeforeGalleryFindFile': before an image is fetched for a gallery
555555 &$gallery,: the gallery object
556556 &$nt: the image title
557 -&$time: image timestamp
 557+&$time: image timestamp (used to specify the file)
 558+&$descQuery: query string to add to thumbnail URL
 559+&$sha1: image base 36 sha1 (used to specify the file, $nt will be ignored if this is set)
558560
559561 'BeforeInitialize': before anything is initialized in performRequestForTitle()
560562 &$title: Title being used for request
——@@ -578,6 +580,8 @@
579581 &$nt: the image title
580582 &$skip: skip this image and link it?
581583 &$time: the image timestamp
 584+&$descQuery: query string to add to thumbnail URL
 585+&$sha1: image base 36 sha1 (used to specify the file, $nt will be ignored if this is set)
582586
583587 'BeforeParserrenderImageGallery': before an image gallery is rendered by Parser
584588 &$parser: Parser object
Index: trunk/phase3/includes/parser/Parser.php
——@@ -1862,11 +1862,11 @@
18631863 $holders->merge( $this->replaceInternalLinks2( $text ) );
18641864 }
18651865 # cloak any absolute URLs inside the image markup, so replaceExternalLinks() won't touch them
1866 - $s .= $prefix . $this->armorLinks( $this->makeImage( $nt, $text, $holders ) ) . $trail;
 1866+ $s .= $prefix . $this->armorLinks(
 1867+ $this->makeImage( $nt, $text, $holders ) ) . $trail;
18671868 } else {
18681869 $s .= $prefix . $trail;
18691870 }
1870 - $this->mOutput->addImage( $nt->getDBkey() );
18711871 wfProfileOut( __METHOD__."-image" );
18721872 continue;
18731873
——@@ -1910,16 +1910,22 @@
19111911 if ( $ns == NS_MEDIA ) {
19121912 wfProfileIn( __METHOD__."-media" );
19131913 # Give extensions a chance to select the file revision for us
1914 - $skip = $time = false;
1915 - wfRunHooks( 'BeforeParserMakeImageLinkObj', array( &$this, &$nt, &$skip, &$time ) );
 1914+ $skip = $time = $sha1 = $descQuery = false;
 1915+ wfRunHooks( 'BeforeParserMakeImageLinkObj',
 1916+ array( &$this, &$nt, &$skip, &$time, &$descQuery, &$sha1 ) );
19161917 if ( $skip ) {
 1918+ $this->mOutput->addImage( $nt->getDBkey() ); // register
19171919 $link = $sk->link( $nt );
19181920 } else {
1919 - $link = $sk->makeMediaLinkObj( $nt, $text, $time );
 1921+ # Fetch and register the file
 1922+ $file = $this->fetchFile( $nt, $time, $sha1 );
 1923+ if ( $file ) {
 1924+ $nt = $file->getTitle(); // file title may be different (via hooks)
 1925+ }
 1926+ $link = $sk->makeMediaLinkFile( $nt, $file, $text );
19201927 }
19211928 # Cloak with NOPARSE to avoid replacement in replaceExternalLinks
19221929 $s .= $prefix . $this->armorLinks( $link ) . $trail;
1923 - $this->mOutput->addImage( $nt->getDBkey() );
19241930 wfProfileOut( __METHOD__."-media" );
19251931 continue;
19261932 }
——@@ -3340,14 +3346,16 @@
33413347 for ( $i = 0; $i < 2 && is_object( $title ); $i++ ) {
33423348 # Give extensions a chance to select the revision instead
33433349 $id = false; # Assume current
3344 - wfRunHooks( 'BeforeParserFetchTemplateAndtitle', array( $parser, &$title, &$skip, &$id ) );
 3350+ wfRunHooks( 'BeforeParserFetchTemplateAndtitle',
 3351+ array( $parser, &$title, &$skip, &$id ) );
33453352
33463353 if ( $skip ) {
33473354 $text = false;
33483355 $deps[] = array(
3349 - 'title' => $title,
3350 - 'page_id' => $title->getArticleID(),
3351 - 'rev_id' => null );
 3356+ 'title' => $title,
 3357+ 'page_id' => $title->getArticleID(),
 3358+ 'rev_id' => null
 3359+ );
33523360 break;
33533361 }
33543362 $rev = $id ? Revision::newFromId( $id ) : Revision::newFromTitle( $title );
——@@ -3359,9 +3367,9 @@
33603368 }
33613369
33623370 $deps[] = array(
3363 - 'title' => $title,
3364 - 'page_id' => $title->getArticleID(),
3365 - 'rev_id' => $rev_id );
 3371+ 'title' => $title,
 3372+ 'page_id' => $title->getArticleID(),
 3373+ 'rev_id' => $rev_id );
33663374
33673375 if ( $rev ) {
33683376 $text = $rev->getText();
——@@ -3390,6 +3398,23 @@
33913399 }
33923400
33933401 /**
 3402+ * Fetch a file and register a reference to it.
 3403+ * @TODO: register and track file version info too
 3404+ */
 3405+ function fetchFile( $title, $time = false, $sha1 = false ) {
 3406+ if ( $sha1 ) { // get by (sha1,timestamp)
 3407+ $file = RepoGroup::singleton()->findFileFromKey( $sha1, array( 'time' => $time ) );
 3408+ if ( $file ) {
 3409+ $title = $file->getTitle(); // file title may not match $title
 3410+ }
 3411+ } else { // get by (name,timestamp)
 3412+ $file = wfFindFile( $title, array( 'time' => $time ) );
 3413+ }
 3414+ $this->mOutput->addImage( $title->getDBkey() );
 3415+ return $file;
 3416+ }
 3417+
 3418+ /**
33943419 * Transclude an interwiki link.
33953420 */
33963421 function interwikiTransclude( $title, $action ) {
——@@ -4506,7 +4531,6 @@
45074532 $ig->setHideBadImages();
45084533 $ig->setAttributes( Sanitizer::validateTagAttributes( $params, 'table' ) );
45094534 $ig->useSkin( $this->mOptions->getSkin( $this->mTitle ) );
4510 - $ig->mRevisionId = $this->mRevisionId;
45114535
45124536 if ( isset( $params['showfilename'] ) ) {
45134537 $ig->setShowFilename( true );
——@@ -4560,11 +4584,6 @@
45614585 $html = $this->recursiveTagParse( trim( $label ) );
45624586
45634587 $ig->add( $nt, $html );
4564 -
4565 - # Only add real images (bug #5586)
4566 - if ( $nt->getNamespace() == NS_FILE ) {
4567 - $this->mOutput->addImage( $nt->getDBkey() );
4568 - }
45694588 }
45704589 return $ig->toHTML();
45714590 }
——@@ -4615,6 +4634,7 @@
46164635 * @param $title Title
46174636 * @param $options String
46184637 * @param $holders LinkHolderArray
 4638+ * @return string HTML
46194639 */
46204640 function makeImage( $title, $options, $holders = false ) {
46214641 # Check if the options text is of the form "options|alt text"
——@@ -4646,15 +4666,18 @@
46474667 $sk = $this->mOptions->getSkin( $this->mTitle );
46484668
46494669 # Give extensions a chance to select the file revision for us
4650 - $skip = $time = $descQuery = false;
4651 - wfRunHooks( 'BeforeParserMakeImageLinkObj', array( &$this, &$title, &$skip, &$time, &$descQuery ) );
4652 -
 4670+ $skip = $time = $sha1 = $descQuery = false;
 4671+ wfRunHooks( 'BeforeParserMakeImageLinkObj',
 4672+ array( &$this, &$title, &$skip, &$time, &$descQuery, &$sha1 ) );
46534673 if ( $skip ) {
 4674+ $this->mOutput->addImage( $title->getDBkey() ); // register
46544675 return $sk->link( $title );
46554676 }
4656 -
4657 - # Get the file
4658 - $file = wfFindFile( $title, array( 'time' => $time ) );
 4677+ # Fetch and register the file
 4678+ $file = $this->fetchFile( $title, $time, $sha1 );
 4679+ if ( $file ) {
 4680+ $title = $file->getTitle(); // file title may be different (via hooks)
 4681+ }
46594682 # Get parameter map
46604683 $handler = $file ? $file->getHandler() : false;
46614684
——@@ -4809,7 +4832,8 @@
48104833 wfRunHooks( 'ParserMakeImageParams', array( $title, $file, &$params ) );
48114834
48124835 # Linker does the rest
4813 - $ret = $sk->makeImageLink2( $title, $file, $params['frame'], $params['handler'], $time, $descQuery, $this->mOptions->getThumbSize() );
 4836+ $ret = $sk->makeImageLink2( $title, $file, $params['frame'], $params['handler'],
 4837+ $time, $descQuery, $this->mOptions->getThumbSize() );
48144838
48154839 # Give the handler a chance to modify the parser object
48164840 if ( $handler ) {
Index: trunk/phase3/includes/Linker.php
——@@ -794,31 +794,39 @@
795795 *
796796 * @param $title Title object.
797797 * @param $text String: pre-sanitized HTML
798 - * @param $time string: time image was created
 798+ * @param $time string: MW timestamp of file creation time
799799 * @return String: HTML
 800+ */
 801+ public function makeMediaLinkObj( $title, $text = '', $time = false ) {
 802+ $img = wfFindFile( $title, array( 'time' => $time ) );
 803+ return $this->makeMediaLinkFile( $title, $img, $text );
 804+ }
 805+
 806+ /**
 807+ * Create a direct link to a given uploaded file.
 808+ * This will make a broken link if $file is false.
800809 *
 810+ * @param $title Title object.
 811+ * @param $file mixed File object or false
 812+ * @param $text String: pre-sanitized HTML
 813+ * @return String: HTML
 814+ *
801815 * @todo Handle invalid or missing images better.
802816 */
803 - public function makeMediaLinkObj( $title, $text = '', $time = false ) {
804 - if ( is_null( $title ) ) {
805 - # # # HOTFIX. Instead of breaking, return empty string.
806 - return $text;
 817+ public function makeMediaLinkFile( Title $title, $file, $text = '' ) {
 818+ if ( $file && $file->exists() ) {
 819+ $url = $file->getURL();
 820+ $class = 'internal';
807821 } else {
808 - $img = wfFindFile( $title, array( 'time' => $time ) );
809 - if ( $img ) {
810 - $url = $img->getURL();
811 - $class = 'internal';
812 - } else {
813 - $url = $this->getUploadUrl( $title );
814 - $class = 'new';
815 - }
816 - $alt = htmlspecialchars( $title->getText(), ENT_QUOTES );
817 - if ( $text == '' ) {
818 - $text = $alt;
819 - }
820 - $u = htmlspecialchars( $url );
821 - return "<a href=\"{$u}\" class=\"$class\" title=\"{$alt}\">{$text}</a>";
 822+ $url = $this->getUploadUrl( $title );
 823+ $class = 'new';
822824 }
 825+ $alt = htmlspecialchars( $title->getText(), ENT_QUOTES );
 826+ if ( $text == '' ) {
 827+ $text = $alt;
 828+ }
 829+ $u = htmlspecialchars( $url );
 830+ return "<a href=\"{$u}\" class=\"$class\" title=\"{$alt}\">{$text}</a>";
823831 }
824832
825833 /**
Index: trunk/phase3/includes/ImageGallery.php
——@@ -14,7 +14,6 @@
1515 var $mImages, $mShowBytes, $mShowFilename;
1616 var $mCaption = false;
1717 var $mSkin = false;
18 - var $mRevisionId = 0;
1918
2019 /**
2120 * Hide blacklisted images?
——@@ -253,12 +252,16 @@
254253 $nt = $pair[0];
255254 $text = $pair[1]; # "text" means "caption" here
256255
257 - # Give extensions a chance to select the file revision for us
258 - $time = $descQuery = false;
259 - wfRunHooks( 'BeforeGalleryFindFile', array( &$this, &$nt, &$time, &$descQuery ) );
260 -
261256 if ( $nt->getNamespace() == NS_FILE ) {
262 - $img = wfFindFile( $nt, array( 'time' => $time ) );
 257+ # Give extensions a chance to select the file revision for us
 258+ $time = $sha1 = $descQuery = false;
 259+ wfRunHooks( 'BeforeGalleryFindFile',
 260+ array( &$this, &$nt, &$time, &$descQuery, &$sha1 ) );
 261+ # Get the file and register it
 262+ $img = $this->mParser->fetchFile( $nt, $time, $sha1 );
 263+ if ( $img ) {
 264+ $nt = $img->getTitle(); // file title may be different (via hooks)
 265+ }
263266 } else {
264267 $img = false;
265268 }
Index: trunk/extensions/FlaggedRevs/FlaggedRevs.hooks.php
——@@ -334,7 +334,7 @@
335335 * (b) Set specified versions in fr_fileSHA1Keys
336336 */
337337 public static function parserFetchStableFile(
338 - $parser, Title $nt, &$skip, &$time, &$query = false
 338+ $parser, Title $nt, &$skip, &$time, &$query, &$sha1
339339 ) {
340340 if ( !( $parser instanceof Parser ) ) {
341341 return true; // nothing to do
——@@ -363,7 +363,7 @@
364364 * (a) Select the desired images based on the selected stable version time/SHA-1
365365 * (b) Set specified versions in fr_fileSHA1Keys
366366 */
367 - public static function galleryFetchStableFile( $ig, Title $nt, &$time, &$query = false ) {
 367+ public static function galleryFetchStableFile( $ig, Title $nt, &$time, &$query, &$sha1 ) {
368368 $parser =& $ig->mParser; // convenience
369369 if ( !( $parser instanceof Parser ) || $nt->getNamespace() != NS_FILE ) {
370370 return true; // nothing to do
——@@ -387,7 +387,7 @@
388388 */
389389 protected static function parserFindStableFile( Parser $parser, Title $title ) {
390390 $time = false; // current version
391 - $sha1 = null; // corresponds to $time
 391+ $sha1 = false; // corresponds to $time
392392 # Check for the version of this file used when reviewed.
393393 $incManager = FRInclusionManager::singleton();
394394 list( $maybeTS, $maybeSha1 ) = $incManager->getReviewedFileVersion( $title );
——@@ -430,15 +430,20 @@
431431 }
432432 # Fetch the current timestamps of the images.
433433 foreach ( $pOutput->mImages as $filename => $x ) {
434 - # FIXME: it would be nice not to double fetch these!
435 - $time = false; // current
436434 # Stable output with versions specified
437435 if ( isset( $pOutput->fr_fileSHA1Keys[$filename] ) ) {
438436 // Fetch file with $time to confirm the specified version exists
439437 $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 );
440446 }
441 - $title = Title::makeTitleSafe( NS_FILE, $filename );
442 - $file = wfFindFile( $title, array( 'time' => $time ) );
 447+ # Update tracking vars...
443448 $pOutput->fr_fileSHA1Keys[$filename] = array();
444449 if ( $file ) {
445450 $pOutput->fr_fileSHA1Keys[$filename]['ts'] = $file->getTimestamp();

Follow-up revisions

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

Comments

#Comment by TheDJ (talk | contribs)   20:38, 23 March 2011

Fatal error: Call to a member function fetchFileAndTitle() on a non-object in /Users/hartman/Development/phase3/includes/ImageGallery.php on line 261

#Comment by TheDJ (talk | contribs)   20:56, 23 March 2011

It was actually r84610 to be exact.

#Comment by Reedy (talk | contribs)   20:40, 23 March 2011

Already logged as bug 28205

#Comment by Tim Starling (talk | contribs)   07:38, 1 September 2011

Please try to resist the temptation to make unrelated code style changes in the same commit as functional changes, it makes your changes more difficult to review. Try doing code cleanup during the design and analysis phase, before you make any substantive changes, then commit before you start the real work.

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

Seems OK except for my comments on r84610.

Status & tagging log