r84610 MediaWiki - Code Review archive

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

bug 28205

Making scaptrap, bumping the version causes mass cache invalidations, right?

Breaks ParserFunctions as well.

What error? The same?

Running test {{#ifexist}}... PHP Warning:  Missing argument 2 for ParserOutput::addImage(), called in /var/www/site/w/extensions/ParserFunctions/ParserFunctions_body.php on line 339 and defined in /var/www/site/w/includes/parser/ParserOutput.php on line 268

Warning: Missing argument 2 for ParserOutput::addImage(), called in /var/www/site/w/extensions/ParserFunctions/ParserFunctions_body.php on line 339 and defined in /var/www/site/w/includes/parser/ParserOutput.php on line 268
PHP Warning:  Missing argument 3 for ParserOutput::addImage(), called in /var/www/site/w/extensions/ParserFunctions/ParserFunctions_body.php on line 339 and defined in /var/www/site/w/includes/parser/ParserOutput.php on line 268

Warning: Missing argument 3 for ParserOutput::addImage(), called in /var/www/site/w/extensions/ParserFunctions/ParserFunctions_body.php on line 339 and defined in /var/www/site/w/includes/parser/ParserOutput.php on line 268
PHP Notice:  Undefined variable: timestamp in /var/www/site/w/includes/parser/ParserOutput.php on line 270

Notice: Undefined variable: timestamp in /var/www/site/w/includes/parser/ParserOutput.php on line 270
PHP Notice:  Undefined variable: sha1 in /var/www/site/w/includes/parser/ParserOutput.php on line 270

Notice: Undefined variable: sha1 in /var/www/site/w/includes/parser/ParserOutput.php on line 270

In the ifexist handler, PFuncs makes use of $parser->mOutput->addImage( $file->getName() );

What is the reason for changing fetchFile() to fetchFileAndTitle()? Is there some scenario where the title it returns is different to $file->getTitle()? Or is it just to save two lines of code in the caller?

Probably was just for consistency with fetchTemplateAndTitle() and convenience.

Comments on this project in general:

I think you should provide an associative array of options to Parser::fetchFileAndTitle(), suitable for passing through to RepoGroup::findFile or RepoGroup::findFileFromKey(), instead of passing through the $time and $sha1 parameters separately. Also, I think instead of adding a $sha1 parameter to the BeforeParserMakeImageLinkObj hook (which you did in r84591), you should give a reference to the options array, and the use of the $time parameter should be deprecated. Then instead of the rather strange and undocumented convention you introduced here, allowing the hook to set $time to the string "0" to indicate that the image should not be shown, you should add an optional element to the options array specifically for that purpose.

Similarly, the time and sha1 tracking introduced here to ParserOutput and OutputPage should be altered to track these options arrays instead. ParserOutput::getImageTimeKeys() could be replaced by ParserOutput::getImageSearchOptions().

The increment to Parser::VERSION is unnecessary and damaging and should be reverted. Instead, make FlaggedRevs gracefully handle the case where the relevant ParserOutput key is missing: have it use fr_fileSHA1Keys instead.

Note that Parser::VERSION is meant to be the MediaWiki version where it was last changed. Presumably if you updated it now you would change it to 1.18.0.

The changes should be more or less done now, except for using ParserOutput::getImageSearchOptions(), which I can't say I'm sold on.

Why are you not sold on it?

I can't quite see the rationale for the change. FlaggedRevs would have to use a hook and the search params to refetch all the files again after Parser (the RepoGroup cache doesn't cover fetch-by-hash). Part of this project was to avoid duplicated queries.

There are two ways files end up fetched on parse: (a) Only current versions are used for all files. In this case, FR wants to know the sha1/time of the current files. (b) FR tries to use stable versions (falling back to the current version if no version is specified) for all files. Here, FR wants to know the template versions as well. Certainly when a current version had to be used a fetch is needed. It's also needed if FR specified the version, since the file may not exist.

We already have template ID tracking. I don't see what's wrong with file tracking.

There's nothing wrong with file tracking. I'm just saying that the time should be stored in an associative array along with any other relevant options, so that the ParserOutput interface can be more stable. Something like

Index: dataclasses/FlaggedRevs.class.php
===================================================================
--- dataclasses/FlaggedRevs.class.php	(revision 96956)
+++ dataclasses/FlaggedRevs.class.php	(working copy)
@@ -595,7 +595,7 @@
 	 * @return bool
 	 */
 	public static function parserOutputIsVersioned( ParserOutput $pOut ) {
-		return ( $pOut->getTemplateIds() !== null && $pOut->getImageTimeKeys() !== null );
+		return ( $pOut->getTemplateIds() !== null && $pOut->getFileSearchOptions() !== null );
 	}
 
 	# ################ Tracking/cache update update functions #################
Index: presentation/FlaggedPageView.php
===================================================================
--- presentation/FlaggedPageView.php	(revision 96956)
+++ presentation/FlaggedPageView.php	(working copy)
@@ -1103,7 +1103,13 @@
 			# Note: showStableVersion() already makes sure that $wgOut has the stable inclusion versions.
 			if ( $this->out->getRevisionId() == $rev->getId() && empty( $this->out->fr_unversionedIncludes ) ) {
 				$tmpVers = $this->out->getTemplateIds();
-				$fileVers = $this->out->getImageTimeKeys();
+				$fileOptions = $this->out->getFileSearchOptions();
+				$fileVers = array();
+				foreach ( $fileOptions as $fileName => $opt ) {
+					if ( isset( $opt['time'] ) ) {
+						$fileVers[$fileName] = $opt['time'];
+					}
+				}
 			} elseif ( $this->oldRevIncludes ) { // e.g. diffonly=1, stable diff
 				# We may have already fetched the inclusion IDs to get the template/file changes.
 				list( $tmpVers, $fileVers ) = $this->oldRevIncludes; // reuse

Oh, I thought you meant "params parser used to search" not "params to search to find the same file parser used".

I don't quite get the foreach loop change above. getImageTimeKeys() already returns an assoc array for each file name with a key for time/sha1. I didn't add 'broken' since only Parser uses that.

The Parser version change has been reverted.