User:Jack Phoenix/Code reviews/SubscriptionManager

Review of wikiHow's SubscriptionManager extension, as of 2014-07-03


 * General
 * It took me to get to the special page file until I realized that this depends on the wikiHow "email" extension. D'oh! That should be documented somewhere.
 * Yay for protecting against CSRF via the edit token!
 * You should upstream as many of those hook changes to core MW as possible, it makes life easier in the long run


 * SpecialSubscriptionManager.php
 * Both the constructor and execute can be safely marked as public
 * Since you're inside a SpecialPage and special pages are ContextSources, use  instead of
 * ...why? This likely isn't what you intended.
 * lines 16, 18, 24 & friends: superfluous end-of-line whitespace, remember to trim those!
 * I would personally get rid of the newlines 16, 44 & 64
 * lines 48-50: programmatically  would likely be a better way, but given that you're just passing it to a message, maybe you'll want wikitext instead, in which case you could try something like   in the i18n msg
 * lines 62-65: OutputPage's  method is canonically capitalized like that if I remember correctly, but you shouldn't be using   and the like; use OutputPage's   or   as appropriate
 * lines 74 & 78: lowerCamelCase is preferred for variable names, so  instead of
 * lines 75 & 92: so why not to ditch the global altogether and use ?
 * line 79: no need to ask OutputPage for the User, just call  directly
 * lines 81-83, 93, 95: s/wfMessage/$this->msg/g
 * lines 101-132: HTMLForm is probably the recommended approach here, yeah. You could use a HTML template class of some kind (core has QuickTemplate, which provides maximum portability as it's a core thing, but wikiHow has used and uses the EasyTemplate class, which was formerly used by Wikia, in many other extensions)
 * there is one practical issue here: the action URL will work only on wikiHow and other sites with a similar URL structure, and given that a null script path is something we recommend against, chances are there aren't that many sites. Fix: use $this->getTitle->getFullURL instead of hard-coding the form action to "/Special:Unsubscribe"
 * HTML validation: method's value (post) should be written in lowercase; cellspacing, cellpadding are deprecated (although I guess all major browsers still support them properly?)
 * line 150: drop the ampersand, it hasn't been needed in ages
 * line 156: given that you don't have any ORDER BY or such statements, you can drop the 6th param to Database::select, i.e. the empty array on line 156


 * SubscriptionFormValidator.php
 * Assuming I can count to two...well, that's exactly how many comment lines the file has! More documentation would definitely be nice and it helps everyone in the long run.
 * lines 3-11, 39, 43, 55, 64-65, 68, 133-134, 137, 151, 177, 184: spacing, i.e.,  ,
 * lines 49 & 166: first one uses a loose comparison and the number is "double quoted", second one uses a strict comparison and single quotes; why? If there's more logic than meets the eye here, it deserves explaning via comments.
 * lines 30 & 106: I don't think you need the ampersand here


 * SubscriptionManager.alias.php
 * Functionality-wise this is OK, style-wise there are some minor things which I'll list anyway because I'm a horrible person:
 * line 7: missing space before
 * line 8: excess spacing around 'en', just write it as
 * line 9: only one tab for indentation, array definition could do with more spacing like this:


 * SubscriptionManager.i18n.php
 * wikiHow is running on MediaWiki 1.23+, so you're free to ditch .i18n.php files in favor of the JSON-based i18n system. This requires replacing the  definition in the setup file with something like , too.
 * PREFIXES! Pick a prefix &mdash; preferably, since it's the name of the extension &mdash; and use it consistently everywhere, i.e. subscriptionmanager-optin-success instead of optin-success
 * Plenty of "wikiHow" references there, as well as some "raw" wikitext &rarr; use  instead of hard-coding the sitename and   for the various links
 * Would prefer if it'd be possible to have neutral defaults for the submanager-community and error-report-problem messages; for wikiHow they can be customized via the MediaWiki namespace on-site
 * error-submanager-3 & error-submanager-4: you'll probably want three periods instead of two in these messages
 * error-submanager-8: a trailing space in the actual message
 * error-submanager-9: I'm not sure about the context/UI, but should "Preferences" be a link to Special:Preferences here?


 * SubscriptionManager.php
 * This could use a standard commented header, like this:
 * It indeed is a bit verbose; generally speaking I'm interested in the license (although I'm aware that the src.wikihow.com site states that "everything is GPL-licensed", but just to make sure)
 * No need for the space between "!" and "defined" (yes, nitpicking, I know; I'm too good at it)
 * Generally speaking a URL pointing to MediaWiki.org would be nice, but since we're talking about an extension written for wikiHow which isn't available via git.wikimedia.org, the URL is fine
 * should be defined and explained in this file