User:Bawolff/review/GWToolset

Review of patchset 5 of https://gerrit.wikimedia.org/r/#/c/59405/ (77074d066072a46e17)

The architecture of this extension seems a bit odd. My understanding is that people want it enabled on commons, but the extension seems to be more constructed as a bot integrated into a MediaWiki install, which edits a different wiki on behalf of the user. Perhaps it might be better to have it enabled on a labs wiki, and have it upload files from there.

Important issues
the current plan is to re-factor the code to use MWHttpRequest; just haven’t gotten to it yet.
 * Why are you using CURL instead of MWHTTPRequest ?

GWToolset.php
if ( $wgHTTPTimeout < 1200 ) { $wgHTTPTimeout = 1200; // 20 minutes, 25 seconds default } This should be set directly on the http requests in question, instead of changing the global limit. this is being used because, as far as i can tell, UploadFromUrl and ApiUpload do not accept a timeout limit as an option and thus use the default value - 25 seconds, which doesn’t work for some of the GLAM asset websites. if there‘s a way to send a timeout option for MWHttpRequest to UploadFromUrl and ApiUpload please let me know and i will use that instead.
 * Fair enough. I've submitted 67229 to address this on the mediawiki side. Bawolff (talk) 04:11, 6 June 2013 (UTC)

line 58 $wgGroupPermissions["gwtoolset"] = $wgGroupPermissions["user"]; This is unnecessary. Permissions inherit. In order to be in gwtoolset group, you must be a user, so you already have user rights.

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.

/* * load extension custom configuraton */ if ( file_exists( $wgGWToolsetDir. DIRECTORY_SEPARATOR. 'includes'. DIRECTORY_SEPARATOR. 'config-custom.php' ) ) { require_once $wgGWToolsetDir. DIRECTORY_SEPARATOR. 'includes'. DIRECTORY_SEPARATOR. 'config-custom.php'; }

This is against MediaWiki coding conventions. All custom configuration should be done in LocalSettings.php this was left-over from a previous patchset. the config-custom.php file has been removed. this section will be removed in the next patchset review.

GWToolset.alias.php
$specialPageAliases['en'] = array(       'MyExtension' => array( 'GWToolset', 'GWToolset' ) );
 * MyExtension is supposed to be replaced with canonical special page name (GWToolset)
 * The array should only have a single entry (since they're both the same)

includes/Models/Menu.php
'Metadata Mapping' Should be using linker. What if the page already has query arguments. Also i18n?

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...

includes/Forms/MetadataMappingForm.php
' ' . wfMessage( 'gwtoolset-metadata-detect-step-2' )->plain. ' ' And other examples of using plain messages in raw html. Plain means unescaped and totally unparsed. You probably want ->escaped instead. Honestly it might be better just to put the header and the instructions in a single message, which gets parsed in this particular case.

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).

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

GWToolset.i18n.php

 * No qqq docs for messages except for the extension description. All of them should have docs.
 * Spelling mistakes are present in the messages. Some of the messages have formatting errors (e.g. Wikitext bullets not being rendered)

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.

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.