MediaWiki Extension Standards issues

[dantman]

• Extensions must use the ExtName/ExtName.php pattern. This extension should be extensions/MWGeSHiGlue/MWGeSHiGlue.php, and geshi should be put into extensions/MWGeSHiGlue/geshi/ like SyntaxHighlight_GeSHi does, the instructions should not be to put the .php file into the extensions/ folder directly.
• The use of isset($mwggGeSHiPath) instead of setting it unconditionally and requiring it to be set after the require line is a register_globals vulnerability. Config variables must be set after extensions.
• Extensions should have an .i18n.php file and the description should be an i18n'ed message
• No error_log, use wfWarn
• Do not use $wgParser->setHook inside of wgExtensionFunctions. Extensions must use the ParserFirstCallInit hook to register stuff with the passed $parser instance.
• Don't use a $languages global like that. That's extremely bad coding. I recommend using a class. And you should use something like $this->getLanguages(); with a private variable cache like we use through core. The way you call ReadLanguages unconditionally is also inefficient. You do a full directory traversal on every single pageview, wastefully. Even when there is no parsing done. And even when no-one is even asking for a list of languages.
• Your idea of using GET parameters is a bad idea. Parser output is cached, parsing is supposed to be independent of the WebRequest.

Side notes:

• That substr junk is unnecessary. You can get rid of /'s and even \'s with rtrim( $path, '/\' );
• Your $languages is is not html escaped.

[mikemol]

Thanks. I'll get these split out into separate issues, and start getting them cleaned up.

On Wed, Feb 15, 2012 at 4:07 PM, Daniel Friesen reply@reply.github.com wrote:

• Extensions must use the ExtName/ExtName.php pattern. This extension should be extensions/MWGeSHiGlue/MWGeSHiGlue.php, and geshi should be put into extensions/MWGeSHiGlue/geshi/ like SyntaxHighlight_GeSHi does, the instructions should not be to put the .php file into the extensions/ folder directly.
• The use of isset($mwggGeSHiPath) instead of setting it unconditionally and requiring it to be set after the require line is a register_globals vulnerability. Config variables must be set after extensions.
• Extensions should have an .i18n.php file and the description should be an i18n'ed message
• No error_log, use wfWarn
• Do not use $wgParser->setHook inside of wgExtensionFunctions. Extensions must use the ParserFirstCallInit hook to register stuff with the passed $parser instance.
• Don't use a $languages global like that. That's extremely bad coding. I recommend using a class. And you should use something like $this->getLanguages(); with a private variable cache like we use through core. The way you call ReadLanguages unconditionally is also inefficient. You do a full directory traversal on every single pageview, wastefully. Even when there is no parsing done. And even when no-one is even asking for a list of languages.
• Your idea of using GET parameters is a bad idea. Parser output is cached, parsing is supposed to be independent of the WebRequest.

Side notes:

• That substr junk is unnecessary. You can get rid of /'s and even \'s with rtrim( $path, '/\' );
• Your $languages is is not html escaped.

---

Reply to this email directly or view it on GitHub: https://github.com/mikemol/mwgeshiglue/issues/6

:wq