Page MenuHomePhabricator

Incorrect MagicVariable hook implementation
Closed, ResolvedPublic

Description

Author: achuggard

Description:
In Parser.php (HEAD r30284) function getVariableValue there are 2 hooks ParserGetVariableValueSwitch and ParserGetVariableValueVarCache whose return values can affect the return value of the function. Code is as follows:

if ( wfRunHooks( 'ParserGetVariableValueVarCache', array( &$this, &$this->mVarCache ) ) ) {

if ( isset( $this->mVarCache[$index] ) ) {                                                                  
        return $this->mVarCache[$index];                                                                    
}

}

...

if ( wfRunHooks( 'ParserGetVariableValueSwitch', array( &$this, &$this->mVarCache, &$index, &$ret ) ) )

return $ret;

else

return null;

I believe that in both of these cases that the check should take the form of if (!wfRunHooks (blah) ) because currently if an extension function returns false (to indicate no matching variable value per http://www.mediawiki.org/wiki/Manual:Variables#Example) this would prevent other variable extensions hooking these points from running. If the extension function returns true (to indicating the variable was handled by an extensions) later running extensions could handle the same variable and clobber the results of previous extensions.


Version: 1.12.x
Severity: normal

Details

Reference
bz12837

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:01 PM
bzimport added a project: MediaWiki-Parser.
bzimport set Reference to bz12837.
bzimport added a subscriber: Unknown Object (MLST).

achuggard wrote:

strike one of the "handlings"

achuggard wrote:

Thinking and reading some more... "ParserGetVariableValueVarCache" is properly done. It allows someone to bypass the cache by returning false or by removing the appropriate entry. (renamed the bug to reflect this fact)

I still feel that "ParserGetVariableValueSwitch" is not properly implemented. It needs to return $ret if wfRunHooks returns false. In code:

if( !wfRunHooks(blah) )
return $ret;
else
return null;

(line 2587 of Parser.php r30876)

achuggard wrote:

Patch against 30778

Here's a patch against r30778. As since this line hasn't changed since r30106 and surrounding code is even older, it should be ok.

attachment Parser.php.patch ignored as obsolete

achuggard wrote:

Comment on attachment 4649
Patch against 30778

Index: Parser.php

  • Parser.php (revision 30778)

+++ Parser.php (working copy)
@@ -2584,7 +2584,7 @@

				return $wgContLanguageCode;
			default:
				$ret = null;
  • if ( wfRunHooks( 'ParserGetVariableValueSwitch', array( &$this, &$this->mVarCache, &$index, &$ret ) ) )

+ if ( ! wfRunHooks( 'ParserGetVariableValueSwitch', array( &$this, &$this->mVarCache, &$index, &$ret ) ) )

					return $ret;
				else
					return null;

achuggard wrote:

Comment on attachment 4649
Patch against 30778

this is a botched patch

achuggard wrote:

Patch to Parser.php against 30778

Trying this again

attachment Parser.txt ignored as obsolete

The patch of attachment 4650 would break lots of compatibility, right now all functions hooked in return 'true' (or otherwise break things), so if you only return the $ret value on 'false' returned, all of the current calls, even if they set $ret, imply they found nothing.
So the best compromise I think would be to always return $ret (it is set to null before the wfRunHooks() call) and leave the 'false' handling to wfRunHooks(), so returning 'false' simply would improve performance and won't break things no longer in versions after the bugfix.

sumanah wrote:

Charlie -- thanks for the patch. We appreciate it. I'm sorry it took so long for us to respond.

MediaWiki developers are currently rewriting the parser -- you can participate on https://lists.wikimedia.org/mailman/listinfo/wikitext-l and https://www.mediawiki.org/wiki/Future . Thanks again.

wfRunHooks() return value no longer implies whether variable value was found. Only $ret is important.

returning false within hook call will improve performance since it will prevent wfRunHooks() to do any other calls, returning true still works with old extensions and has no specific meaning. This wil primarily fix extensions which have followed the old description on http://www.mediawiki.org/wiki/Manual:Hooks/ParserGetVariableValueSwitch
It allows to improve performance for extensions which do not want to keep compatibility with previous MW versions by returning false when value found.

Attached:

sumanah wrote:

(adding need-review keyword re: DanWe's new patch)

sumanah wrote:

Daniel, sorry for the wait in review. I encourage you to get a Git account

https://www.mediawiki.org/wiki/Git

to make it easier for you to submit patches to MediaWiki and to get them reviewed faster. Thanks!

audreyt wrote:

Hi Daniel, thank you for the patch!

As you may already know, MediaWiki is currently revamping its PHP-based parser
into a "Parsoid" prototype component, to support the rich-text Visual Editor
project:

https://www.mediawiki.org/wiki/Parsoid
https://www.mediawiki.org/wiki/Visual_editor

Folks interested in enhancing the parser's capabilities are very much welcome
to join the Parsoid project, and contribute patches as Git branches:

https://www.mediawiki.org/wiki/Git/Tutorial#How_to_submit_a_patch

Compared to .diff attachments in Bugzilla tickets, Git branches are much easier
for us to review, refine and merge features together.

Each change set has a distinct URL generated by the "git review" tool, which
can be referenced in Bugzilla by pasting its gerrit.wikimedia.org URL as a
comment.

If you run into any issues with the patch process, please feel free to ask on
irc.freenode.net #wikimedia-dev and the wikitext-l mailing list. Thank you!

Change 104203 had a related patch set uploaded by Siebrand:
wfRunHooks() return value no longer implies whether variable value was found. Bug: 12837

https://gerrit.wikimedia.org/r/104203

Change 104206 had a related patch set uploaded by Siebrand:
wfRunHooks() return value no longer implies whether variable value was found

https://gerrit.wikimedia.org/r/104206

Change 104490 had a related patch set uploaded by Vishnunk90:
wfRunHooks() return value no longer implies whether variable value was found

https://gerrit.wikimedia.org/r/104490

Change 104490 abandoned by Vishnunk90:
wfRunHooks() return value no longer implies whether variable value was found

Reason:
Regenerated copy

https://gerrit.wikimedia.org/r/104490

Change 104206 abandoned by Vishnunk90:
wfRunHooks() return value no longer implies whether variable value was found

Reason:
Regenerated one

https://gerrit.wikimedia.org/r/104206

Change 104203 had a related patch set uploaded by Siebrand:
wfRunHooks() return value no longer implies whether variable value was found. Bug: 12837

https://gerrit.wikimedia.org/r/104203

Change 104203 merged by jenkins-bot:
wfRunHooks() return value no longer implies whether variable value was found

https://gerrit.wikimedia.org/r/104203