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.

Curl.php
Why are you using CURL instead of MWHTTPRequest ?
 * the current plan is to re-factor the code to use MWHttpRequest; just haven’t gotten to it yet. Dan-nl (talk) 07:07, 5 June 2013 (UTC)
 * code has been re-factored, Curl.php is no longer used in Patch Set 7. Dan-nl (talk) 03:20, 10 July 2013 (UTC)

$wgHTTPTimeout
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. Dan-nl (talk) 07:07, 5 June 2013 (UTC)
 * Fair enough. I've submitted 67229 to address this on the mediawiki side. Bawolff (talk) 04:11, 6 June 2013 (UTC)
 * for now i have created a static method in Patch Set 7, WikiChecks::increaseHTTPTimeout, which is used only when the extension is uploading metadata or mediafiles. Dan-nl (talk) 03:20, 10 July 2013 (UTC)

$wgGroupPermissions
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.
 * i have removed this line in Patch Set 7 Dan-nl (talk) 03:20, 10 July 2013 (UTC)

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)

custom configuration
/* * 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. Dan-nl (talk) 07:07, 5 June 2013 (UTC)
 * this section has been removed in Patch Set 7 Dan-nl (talk) 03:20, 10 July 2013 (UTC)

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)
 * this adjustment has been made in Patch Set 7 Dan-nl (talk) 03:20, 10 July 2013 (UTC)

includes/Models/Menu.php
'Metadata Mapping' Should be using linker. What if the page already has query arguments. Also i18n?
 * this has been refactored in Patch Set 7. the Menu.php file was removed and the link is put in place using Linker::link and an i18n message. 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)

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.
 * i have re-factored all ->plain messages to ->escaped and re-worked some into ->parse in Patch Set 7. Dan-nl (talk) 03:20, 10 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)

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)
 * plan to address this in a future Patch Set. Dan-nl (talk) 03:20, 10 July 2013 (UTC)

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)

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)