r107162 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r107161‎ | r107162 | r107163 >
Date:16:41, 23 December 2011
Author:grafzahl
Status:resolved (Comments)
Tags:
Comment:
Do not unconditionally set global variables that might be used by other extensions, too
Modified paths:
  • /trunk/extensions/Score/Score.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Score/Score.php
——@@ -46,9 +46,13 @@
4747 * Default is $wgUseImageMagick and set in efScoreExtension */
4848 $wgScoreTrim = null;
4949 /* Path of lilypond executable */
50 -$wgLilyPond = '/usr/bin/lilypond';
 50+if ( !isset( $wgLilyPond ) ) {
 51+ $wgLilyPond = '/usr/bin/lilypond';
 52+}
5153 /* Path to converter from ABC */
52 -$wgAbc2Ly = '/usr/bin/abc2ly';
 54+if ( !isset( $wgAbc2Ly ) ) {
 55+ $wgAbc2Ly = '/usr/bin/abc2ly';
 56+}
5357
5458 /*
5559 * Extension credits

Follow-up revisions

RevisionCommit summaryAuthorDate
r107215Monopolise our globals and set them unconditionally....grafzahl18:18, 24 December 2011

Comments

#Comment by 😂 (talk | contribs)   16:31, 24 December 2011

And by conditionally setting them like this you make the code vulnerable to register globals. Globals must be set.

#Comment by GrafZahl (talk | contribs)   17:23, 24 December 2011

OK, then what's the best solution to this dilemma? One that comes to mind is to just duplicate the globals (e.g. use $wgScoreAbc2Ly instead of $wgAbc2Ly) and thus make the globals "mine" (or the extension's). The downside is that it duplicates work for the user, but, given the current globals, I guess it should be OK for the time being.

#Comment by GrafZahl (talk | contribs)   18:20, 24 December 2011

Fixed in r107215, thanks!

#Comment by Reedy (talk | contribs)   17:25, 24 December 2011

If the other extensions are loaded, in theory you should know that the paths should have been set to some value at least, whether it's true/valid is a different issue///

#Comment by GrafZahl (talk | contribs)   17:52, 24 December 2011

OK, in Register globals it says that users should always configure extensions after inclusion of the setup file. Conceivably, this will work only if all globals "belong" to at most one extension, as otherwise you'd have to separate the require directives from the setting of the vars. Therefore, I will implement my comment above.

Status & tagging log