Last modified: 2014-01-11 20:23:44 UTC
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.
strike one of the "handlings"
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)
Created attachment 4649 [details] 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.
Comment on attachment 4649 [details] 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;
Comment on attachment 4649 [details] Patch against 30778 this is a botched patch
Created attachment 4650 [details] Patch to Parser.php against 30778 Trying this again
The patch of attachment 4650 [details] 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.
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.
Created attachment 9380 [details] 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.
(adding need-review keyword re: DanWe's new patch)
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!
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 merged by jenkins-bot: wfRunHooks() return value no longer implies whether variable value was found https://gerrit.wikimedia.org/r/104203
Status Merged
