r96015 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96014‎ | r96015 | r96016 >
Date:17:23, 1 September 2011
Author:krinkle
Status:resolved (Comments)
Tags:
Comment:
Pass OutputPage instance to MakeGlobalVariablesScript. Allows extensions to getTitle()->equals( .. ) and add config vars depending on title
Modified paths:
  • /trunk/phase3/docs/hooks.txt (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/docs/hooks.txt
——@@ -1221,6 +1221,8 @@
12221222 through ResourceLoaderGetConfigVars instead.
12231223 &$vars: variable (or multiple variables) to be added into the output
12241224 of Skin::makeVariablesScript
 1225+&$out: The OutputPage which called the hook,
 1226+ can be used to get the real title
12251227
12261228 'MarkPatrolled': before an edit is marked patrolled
12271229 $rcid: ID of the revision to be marked patrolled
Index: trunk/phase3/includes/OutputPage.php
——@@ -2658,7 +2658,7 @@
26592659 }
26602660
26612661 // Allow extensions to add their custom variables to the global JS variables
2662 - wfRunHooks( 'MakeGlobalVariablesScript', array( &$vars ) );
 2662+ wfRunHooks( 'MakeGlobalVariablesScript', array( &$vars, &$this ) );
26632663
26642664 return $vars;
26652665 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r101896FU r96015: removed & in MakeGlobalVariablesScript hookaaron22:21, 3 November 2011

Comments

#Comment by Nikerabbit (talk | contribs)   20:16, 1 September 2011

Why &$this instead of just $this?

#Comment by Dantman (talk | contribs)   04:02, 2 September 2011

Seconded, there's no use for it here. I desperately want to start marking as a fixme any commit adding a new hook with a & that isn't there because the purpose of the hook is to let the hook user alter the contents of the variable. After I look at BeforePageDisplay, our context code, and make a change in the code a few lines below, see the &'s that don't need to be there, and realize what a horrible situation that hook is in.

We can add a & to a hook later on if we find a need for a caller to alter the variable. However we cannot remove a & from an existing hook, because by all chances functions defined for those hooks will already be including a & even if they don't use it and if we remove the & from the wfRunHooks calls then a pile of extensions start throwing php warning errors.

#Comment by Krinkle (talk | contribs)   14:17, 2 September 2011

I know it's not needed technically (object being passed by reference already). After asking Roan, I kept it in to be consistent with other places.

#Comment by 😂 (talk | contribs)   14:21, 2 September 2011

There's no need though. All the other places as wrong

#Comment by 😂 (talk | contribs)   14:22, 2 September 2011

s/as/are/

Status & tagging log