"Tim Starling" changed the status of MediaWiki.r98414 to "fixme" and commented
it.
URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98414#c27299
Old Status: new
> New Status: fixme
Commit summary for MediaWiki.r98414:
Add LilyPond module
Tim Starling's comment:
Whole extension review as of r100547:
<pre>
$pre = "<a href=\"" . $wgMathPath . "/" . $md5 . ".midi\"> " .
$wgLilypondPreMidi;
</pre>
Html::openElement() etc. should be used in new code. Several instances. Also
I'm not sure what $wgLilypondPreMidi and $wgLilypondPostMidi are meant to be
used for, but whatever it is, I'm sure it shouldn't be a global variable.
What is the #t, ##f stuff meant to do, e.g. "-dsafe='#t'"? It looks incomplete,
like it's meant to be substituted with something but the substitution code is
missing.
<pre>
$cmd = $wgLilypond .
" -dsafe='#t' -dbackend=eps --png --header=texidoc " .
escapeshellarg( $lyFile ) . " 2>&1";
</pre>
wfEscapeShellArg() should be used, not escapeshellarg(). This gives Windows
compatibility and a workaround for some Linux locale issues.
<pre>
exec( $cmd, $output, $ret );
</pre>
wfShellExec() should be used.
<pre>
nl2br( htmlentities( join( "\n",
$output ) ) ) )
</pre>
Use htmlspecialchars() not htmlentities().
<pre>
return "<b>$mf (" . wfMsg( "math_bad_tmpdir" ) .
")</b>";
</pre>
In new code, messages should generally be plain text, not HTML, so escaping is
recommended here.
<pre>
$out = fopen( $wgTmpDirectory . "/" . $lyFile, "w" );
...
fwrite( $out, $lilypond_code );
fclose( $out );
</pre>
file_put_contents() would probably be more convenient here.
<pre>
$files = opendir( $wgTmpDirectory );
$last_page = 0;
while ( false !== ( $file = readdir( $files ) ) ) {
</pre>
This might be a very large list indeed, depending on what else is running on
the server. I suggest creating a separate directory for each LilyPond run, and
then deleting all the files in it. If the directory has a random name instead
of being a guessable hash, that avoids the symlink vulnerability in /tmp which
the current code appears to suffer from.
$wgMathPath and $wgMathDirectory should not be used, the extension should be
separately configured.
<pre>
if ( $wgLilypondTrim ) {
$imgFile = $wgMathDirectory . "/" . $md5 . ".png";
self::trimImage( $imgFile, $imgFile, 0xFFFFFF );
}
</pre>
For efficiency, trimming should be done while the image is still in the
temporary directory.
<pre>
$ymin = self::findMin( $height, $width, $srcImage, $bgColour );
</pre>
This won't have the intended result. findMin() and findMax() will only find
bounds in the X direction, and the first parameter is unconditionally a width,
just swapping the parameters won't work.
You may want to consider adding an option to use ImageMagick "convert -trim"
instead.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
