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

Important issues

WikiChecks.php

• No longer using its own Curl implementation, but still checking Curl exists in WikiChecks::verifyCurlExists()
  • removed the method Dan-nl (talk) 09:55, 2 October 2013 (UTC)
• If an error happens Special:GWToolset is displayed with no title.
  • added $this->setHeaders() to SpecialGWToolset.php Dan-nl (talk) 09:55, 2 October 2013 (UTC)
• 'gwtoolset-maxuploadsize-exceeds-ini-settings' - This is a valid (And probably common condition). It shouldn't error out on this.
  • i have removed the check. if $wgMaxUploadSize is greater than the php ini setting, upload_max_filesize, or post_max_size is greater upload_max_filesize, won't that cause an issue? Dan-nl (talk) 09:55, 2 October 2013 (UTC)

increaseMemoryLimit

First of all, if messing with ini settings, use wfShorthandToInteger from includes/GlobalFunctions.php (See wfMemoryLimit from includes/GlobalFunctions.php for an example).

• thanks. i don’t know who would need to do this, but t might be worthwhile looking at modifying wfShorthandToInteger to http://www.php.net/manual/en/function.ini-get.php#96996 Dan-nl (talk) 09:55, 2 October 2013 (UTC)

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.

• i re-factored this method earlier and did not change its method signature. it’s now only checking the limit and issuing an E_USER_NOTICE when ImageMagick is not being used and the memory limit is set below the recommendation in Config.php. i have altered the signature to checkMemoryLimit(). Dan-nl (talk) 09:55, 2 October 2013 (UTC)

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

• the extension is not scaling the image. i found that when visiting the media file page after creation, and the memory limit is below 256M, and ImageMagick is not being used, the image thumbnail and its representation on the page would not display. if ImageMagick is being used there is no issue. Dan-nl (talk) 09:55, 2 October 2013 (UTC)
• if you still want me to remove the check let me know and i will do so. Dan-nl (talk) 09:55, 2 October 2013 (UTC)

Models/MediawikiTemplate.php

• Still have user Php\Curl despite that not existing anymore.
  • removed the reference Dan-nl (talk) 09:55, 2 October 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)

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)

• i have re-factored the code to use Title::makeTitleSafe with an appropriate namespace. Dan-nl (talk) 10:35, 3 October 2013 (UTC)
• i did not use wfStripIllegalFilenameChars because it was removing the / when the title was a subpage. Dan-nl (talk) 10:35, 3 October 2013 (UTC)
• titleCheck() has been removed from the extension. Dan-nl (talk) 09:55, 2 October 2013 (UTC)
• created a new method, which i placed in functions/functions.php::stripIllegalTitleChars(). this is necessary because we don’t want certain characters to appear in titles, even if they are valid title characters in a broader sense, these characters are discouraged on Commons @see https://commons.wikimedia.org/wiki/Commons:File_naming. Dan-nl (talk) 11:46, 12 October 2013 (UTC)

Namespaces

When an extension adds a namespace, it needs to do it with an i18n file.

• i have added a similar GWToolset.namespaces.php to the extension. Dan-nl (talk) 09:55, 2 October 2013 (UTC)

You also shouldn't have references to its actual "name" (in for example $metadata_namespace), just its number.

• made the change in Config.php by referencing the constant rather than the number figuring that the number has not yet been determined. Dan-nl (talk) 09:55, 2 October 2013 (UTC)

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

• thanks for the example in LiquidThreads. added a similar hook. Dan-nl (talk) 09:55, 2 October 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)
• the signature has also changed to WikiChecks::checkMemoryLImit(). Dan-nl (talk) 09:55, 2 October 2013 (UTC)

UploadMetadataJob

It would be better if instead of doing $_POST = $this->params['post'];, the specific parameters were passed into the method.

• the idea is to pass the entire $_POST since the fields posted will vary depending on the metadata, mappings, categories, etc. the code that runs against the post makes sure the appropriate parameters are selected from the $_POST; this allows for the logic to reside within the handlers and handle a variety of possible posts. i would prefer not specifying exact parameters, but if this prevents us from getting the extension onto production let me know and i will make the change. Dan-nl (talk) 20:05, 2 October 2013 (UTC)

UploadHandler

UploadHandler::addAllowedExtensions

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

• added a method to restore the original $wgFileExtensions. Dan-nl (talk) 20:08, 2 October 2013 (UTC)

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)

• i have modified the method so that it uses MimeMagic. Dan-nl (talk) 20:11, 2 October 2013 (UTC)

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:

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

etc

• added getTitle() to functions.php to replace WikiPages::getTitleFromUrl() and removed WikiPages.php from the extension. Dan-nl (talk) 10:43, 3 October 2013 (UTC)

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.

• made the change to MWException Dan-nl (talk) 21:11, 2 October 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)

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.

• after the upcoming review i may make this change depending on time and budget. Dan-nl (talk) 10:55, 3 October 2013 (UTC)

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)

• this change is more important and i need to come back to it after the upcoming review. Dan-nl (talk) 10:55, 3 October 2013 (UTC)

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.

• after the upcoming review i may make this change depending on time and budget. still, i find it a bit confusing as the js convention asks for the documentation to be in the format @param {type} $variable_name, which i prefer because the {} visually differentiates between the type and variable name. Dan-nl (talk) 10:55, 3 October 2013 (UTC)

thank you!

thanks bawolff for creating this page and following-up on these items. this page, while not “ideal”, is much more helpful than the comments within gerrit. Dan-nl (talk) 10:46, 3 October 2013 (UTC)