r98414 MediaWiki - Code Review archive

• $wgExtensionCredits credits are missing (and a localisable extension description for Special:Version)
• Should probably use ParserFirstCallInit instead of $wgExtensionFunctions[]
• parser tag methods should be in a separate class file.

Guard for register globals exploits is missing:

if ( !defined( 'MEDIAWIKI' ) ) {
	die( "This is not a valid entry point.\n" );
}

And even it is insufficient for stuff like

if( !isset( $wgLilypond ) )
		$wgLilypond = "PATH=\$PATH:/usr/local/bin /usr/local/bin/lilypond";

Probably worth adding some wfDebugLog() and maybe some profiling...

Ignore me (aka bug 22094)

Whole extension review as of r100547:

$pre = "<a href=\"" . $wgMathPath . "/" . $md5 . ".midi\"> " . $wgLilypondPreMidi;

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.

		$cmd = $wgLilypond .
			" -dsafe='#t' -dbackend=eps --png --header=texidoc " .
			escapeshellarg( $lyFile ) . " 2>&1";

wfEscapeShellArg() should be used, not escapeshellarg(). This gives Windows compatibility and a workaround for some Linux locale issues.

		exec( $cmd, $output, $ret );

wfShellExec() should be used.

					nl2br( htmlentities( join( "\n", $output ) ) ) )

Use htmlspecialchars() not htmlentities().

			return "<b>$mf (" . wfMsg( "math_bad_tmpdir" ) . ")</b>";

In new code, messages should generally be plain text, not HTML, so escaping is recommended here.

		$out = fopen( $wgTmpDirectory . "/" . $lyFile, "w" );
...
		fwrite( $out, $lilypond_code );
		fclose( $out );

file_put_contents() would probably be more convenient here.

		$files = opendir( $wgTmpDirectory );
		$last_page = 0;

		while ( false !== ( $file = readdir( $files ) ) ) {

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.

		if ( $wgLilypondTrim ) {
			$imgFile = $wgMathDirectory . "/" . $md5 . ".png";
			self::trimImage( $imgFile, $imgFile, 0xFFFFFF );
		}

For efficiency, trimming should be done while the image is still in the temporary directory.

		$ymin = self::findMin( $height, $width, $srcImage, $bgColour );

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.

Thanks for the review! I'll try to get these addressed.

set to deferred since development has moved to the Score extension. Also added a security warning to the Lilypond page.

If no one is going to main this and development has moved to another extension, this whole extension should be deleted from the repo.

I agree. Not a lot of point keeping a non working, unsecure extension about if there is a better alternative available

Is there an equivalent of w:Wikipedia:How to fix cut-and-paste moves for code revisions? I mean, is it posible to move the old one to the new name and merge its histories?

You can svn move and "replace a file". Or just paste the new files over the old, and fget a diff commit

I don't think there is a way to completely merge histories...