User:Bawolff/review/GWToolset

For review of patchset 5 - https://gerrit.wikimedia.org/r/#/c/59405/ (77074d066072a46e17) see: https://www.mediawiki.org/w/index.php?title=User:Bawolff/review/GWToolset&oldid=731872

WikiChecks.php

 * No longer using its own Curl implementation, but still checking Curl exists in WikiChecks::verifyCurlExists
 * If an error happens Special:GWToolset is displayed with no title.
 * 'gwtoolset-maxuploadsize-exceeds-ini-settings' - This is a valid (And probably common condition). It shouldn't error out on this.

increaseMemoryLimit
First of all, if messing with ini settings, use wfShorthandToInteger from includes/GlobalFunctions.php (See wfMemoryLimit from includes/GlobalFunctions.php for an example). Second of all don't mess with ini settings. Users have valid reasons to set the memory limit to whatever they set it to. If folks run out of memory, that's usually a pretty easy problem to diagnose.

Third, this shouldn't be an issue with image scalars, unless the extension is scaling the image. If the extension is scaling images during the upload process, something is wrong. It should not make thumbnails until they are needed, which should be not during the execution of this extension (but later, when user visits the image page).

Models/MediawikiTemplate.php

 * Still have user Php\Curl despite that not existing anymore.

permission via group vs right
Additionally, the special page seems to be responding to being in the group, instead of having a specific right. This is wrong. Special page access should correspond to a specific right.
 * we only want people who are part of the gwtoolset group to be able to access the extension. if we need to instead base this permission on rights, which right(s) should we test against? Dan-nl (talk) 03:20, 10 July 2013 (UTC)

includes/functions/functions.php
set_error_handler('\GWToolset\handleError'); This causes massive amounts of debugging info to be output, even for warnings unrelated to GWToolset. If you want to output debugging info, use the debug log. If its important to output extended warnings, check for errors at appropriate time instead of installing a global error handler. At the very least this should be disabled by default...

Also, this seems to break special pages...
 * i like having this handler in place because any php errors within the extension become obvious and i can address them immediately. i have moved the method into the special page of the extension in the upcoming Patch Set 8; this should make it so that the code only affects this extension. it's also only “on” when the wiki has display_errors turned on an error level >= E_ALL. Dan-nl (talk) 03:20, 10 July 2013 (UTC)
 * patch set 8 is now in gerrit with the change mentioned above Dan-nl (talk) 18:31, 12 July 2013 (UTC)

includes/Helpers/FileChecks.php
public static function getValidTitle( $title, $replacement = '-' ) { if ( strlen( $title ) > 220 ) { throw new Exception( wfMessage( 'gwtoolset-title-too-long', Filter::evaluate( $title ) )->plain ); $title = mb_strcut( $title, 0, 220, 'UTF-8' ); }

return str_replace( array( '#','<','>','[',']','|','{','}',':','¬','`','!','"','£','$','^','&','*','(',')','+','=','~','?','/',',',Config::$metadata_separator,';',"'",'@' ), $replacement, $title );       } Should first of all be using MW's internal title validation, so it stays in sync. Second of all should have a comment explaining why these characters are invalid, as most of those are perfectly valid in a title. Third, should be using MWException class not the Exception class. (Provided you always catch the exception, its not critical which exception class you use, but its still preferred you use MWException).
 * i have re-factored title generation to use Title::newFromText in Patch Set 7. Dan-nl (talk) 03:20, 10 July 2013 (UTC)
 * i have re-factored this method in Patch Set 7 and placed it in WikiPages::titleCheck, which only replaces : and / now. i have added comments as to why those characters are being replaced. Dan-nl (talk) 03:20, 10 July 2013 (UTC)
 * the exception has been removed in Patch Set 7. Dan-nl (talk) 03:20, 10 July 2013 (UTC)
 * Can this just be replaced with a call to wfStripIllegalFilenameChars ? Bawolff (talk) 23:47, 25 September 2013 (UTC)

Namespaces
When an extension adds a namespace, it needs to do it with an i18n file. You also shouldn't have references to its actual "name" (in for example $metadata_namespace), just its number.

For example, in the LiquidThreads extension, see the file LiquidThreads/i18n/Lqt.namespaces.php, the namespace part of LiquidThreads/LiquidThreads.php and LqtHooks::onCanonicalNamespaces in LiquidThreads/classes/Hooks.php

Less important issues
Things that aren't critical, but probably should be different

GWToolset.php
/** * check environment variables */ // needed to allow for creation of thumbnails for large images if ( (int)ini_get('memory_limit') < 256) { ini_set('memory_limit', '256M'); // 128M default } Should change $wgMemoryLimit instead.
 * looking into $wgMemoryLimit, i saw that it’s a global setting controlled by the wiki admin, so i added an install note to the upcoming Patch Set 8, but i still want the extension to check and increase the value in case the wiki admin forgot to make the change. i re-factored this code and placed it into a method within WikiChecks::increaseMemoryLimit in Patch Set 7. Dan-nl (talk) 03:20, 10 July 2013 (UTC)

UploadMetadataJob
It would be better if instead of doing, the specific parameters were passed into the method.

UploadHandler::addAllowedExtensions
Anytime you modify a global like $wgFileExtensions, make sure you change it back when you are done with it.

UploadHandler::evaluateMediafileUrl
I would prefer if this used $wgFileExtensions and MimeMagic (eg the getTypeForExtension method) to  determine if the file is an allowed type. Or since the file is already downloaded at this point, you could generate the extension based on the content-type (extensions in urls aren't really supposed to mean anything), and pass it off to the Upload classes to verify it really is the right type)

WikiPages
Mildly confusing, since there's a very important class in core named WikiPage. But given you don't seem to use WikiPage and WikiPages in the same context, I guess its not too bad.

WikiPages::getTitleFromUrl
Trying to extract a pagename from a url has about a billion edge cases, and quite honestly is not a good idea. If you want the user to specify some page on the wiki, tell them to put in the page name, not the url.

Examples where this will fail: etc
 * http://commons.wikimedia.org/wiki/Main_Page ( $wgServer is probably not what you expect it to be. $wgServer on commons is '//commons.wikimedia.org' )
 * //commons.wikimedia.org/wiki/Foo?bar (Article path version can have query args)
 * //commons.wikimedia.org/w/index.php?title=Foo (No stripping of the /w/index.php part)
 * //commons.wikimedia.org/w/index.php?title=Foo&somethingElse (&somethingElse is not part of title)

Exceptions
Make all your exceptions be either MWException's or a subclass of MWException, unless you have a good reason. Its ok for non-MWException exceptions to be thrown, provided they are always caught, but its still really not preferred.

Things that are really nitpicky
You could probably ignore these if you want

GWToolset.php
line 82: if ( !empty( $values['group'] ) ) { $wgSpecialPageGroups[$page] = $values['group']; } empty is generally against mw coding conventions. Use isset instead. There are many other examples of this.
 * i will ignore this for now, but if it needs to change let me know. Dan-nl (talk) 03:20, 10 July 2013 (UTC)

Variable naming conventions
MediaWiki never uses $variable_names_with_camal_case (Except for constants and class constants which are ALL_UPPERCASE_WITH_UNDERSCORE). However, in an extension you have more freedom to not follow such conventions super-strictly, and it would probably not be worth it to change the extension. Given the way the config class is used, it might make sense to change those to be class constants. But again, probably not worth fretting about. (If the stuff in the Config file is meant to be settable, the MediaWiki of doing so is generally to make globals of the form $wgGWToolsetConfigName)

On a similar note, usually we document classes like @param $accepted_types Array Description of argument. not @param {array} $accepted_types. Again, its probably not worth the effort to change.