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.

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)

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)

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)