Last modified: 2014-01-11 20:23:44 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 12837 - Incorrect MagicVariable hook implementation
Incorrect MagicVariable hook implementation
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
1.12.x
All All
: Low normal (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-30 08:04 UTC by Charlie Huggard
Modified: 2014-01-11 20:23 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch against 30778 (1.62 KB, patch)
2008-02-13 23:54 UTC, Charlie Huggard
Details
Patch to Parser.php against 30778 (502 bytes, patch)
2008-02-14 00:03 UTC, Charlie Huggard
Details
wfRunHooks() return value no longer implies whether variable value was found. Only $ret is important. (723 bytes, patch)
2011-11-08 17:35 UTC, Daniel A. R. Werner
Details

Description Charlie Huggard 2008-01-30 08:04:58 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.
Comment 1 Charlie Huggard 2008-01-30 08:06:35 UTC
strike one of the "handlings"
Comment 2 Charlie Huggard 2008-02-13 23:39:58 UTC
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)
Comment 3 Charlie Huggard 2008-02-13 23:54:43 UTC
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 4 Charlie Huggard 2008-02-14 00:00:12 UTC
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 5 Charlie Huggard 2008-02-14 00:03:06 UTC
Comment on attachment 4649 [details]
Patch against 30778 

this is a botched patch
Comment 6 Charlie Huggard 2008-02-14 00:03:56 UTC
Created attachment 4650 [details]
Patch to Parser.php against 30778 

Trying this again
Comment 7 Daniel A. R. Werner 2011-11-08 17:02:13 UTC
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.
Comment 8 Sumana Harihareswara 2011-11-08 17:34:52 UTC
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.
Comment 9 Daniel A. R. Werner 2011-11-08 17:35:20 UTC
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.
Comment 10 Sumana Harihareswara 2011-11-08 17:36:36 UTC
(adding need-review keyword re: DanWe's new patch)
Comment 11 Sumana Harihareswara 2012-03-23 22:04:03 UTC
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!
Comment 12 au 2012-06-17 18:40:55 UTC
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!
Comment 13 Gerrit Notification Bot 2013-12-28 14:28:45 UTC
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
Comment 14 Gerrit Notification Bot 2013-12-30 06:48:48 UTC
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
Comment 15 Gerrit Notification Bot 2013-12-30 08:28:55 UTC
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
Comment 16 Gerrit Notification Bot 2013-12-30 16:28:30 UTC
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
Comment 17 Gerrit Notification Bot 2013-12-30 16:30:19 UTC
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
Comment 18 Gerrit Notification Bot 2013-12-31 09:10:54 UTC
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
Comment 19 Gerrit Notification Bot 2013-12-31 09:16:43 UTC
Change 104203 merged by jenkins-bot:
wfRunHooks() return value no longer implies whether variable value was found

https://gerrit.wikimedia.org/r/104203
Comment 20 db [inactive,noenotif] 2014-01-11 20:23:44 UTC
Status Merged

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links