User:Legoktm/Memento

Memento extension review

General stuff

 * Extension should be in Gerrit to get better reviews from MediaWiki developers
 * Code conventions should be followed, see CC/PHP. Mainly spacing issues I noticed.
 * Add type hinting
 * The link to "@see http://www.mediawiki.org/wiki/Security_for_developers" at the top of every file is unnecessary. - DONE
 * Use proper PHPDoc annotations: @param type $varname brief description if not obvious - Waiting till rest of the fixes from this review are done

Memento.php

 * Convert i18n to JSON-based i18n - DONE
 * "extension-overview" is too general of a key name, it should be prefixed with "memento" at the very least. Typically the message key used here is "memento-desc". - DONE
 * The main Memento class should be moved into it's own file (Memento.body.php) and autoloader. - DONE


 * Memento class
 * In onImageBeforeProduceHTML: $config->get should use === true - DONE, needs regression testing
 * In onImageBeforeProduceHTML: function looks like it could be really expensive for non-locally hosted files... - I don't know how else to resolve this; the $wgMementoTimeNegotiationForThumbnails option is what controls this, and is off by default
 * In onArticleViewHeader: Use ->getTitle->exists, to check whether the page exists in the database, which is what I think you want. ->isKnown returns true for special pages.
 * I'm not sure what I think about using static members like this. It feels weird. - I saw several extensions doing this, and yes it doesn't seem right. I need a way to pass values between hooks, so I kept the static class.  I didn't think I could invoke hooks with non-static methods and using a global felt equally dirty. I reread the section on hooks and realize that I can create my own class with a public function for this purpose.  That feels better.

Memento.config.php

 * This should be a singleton class.
 * This is vulnerable to register_globals. What you should do is in Memento.php (the main entry point), set default values there, and then let the user override them in their own LocalSettings.php. The setDefault function shouldn't be used. This also allows you to document the parameters and what they do in the code itself
 * Not really important since MW1.24+ no longer supports register_globals.

MementoResource.php

 * MementoResourceException is large enough to have its own file. - DONE
 * MementoResourceException should extend MWException. - DONE


 * MementoResource
 * Member variable should not be named $dbr since a non-slave database could be passed in the constructor. - DONE
 * fetchMementoFromDatabase: Use $db->selectRow - Failed regression testing, set back to $db->select and now it works
 * getFirstMemento: Should check if Title::getFirstRevision returned null - DONE, TODO: new error state that needs an exit path, needs regression testing
 * getLastMemento: Use Revision::newFromTitle( $title ) instead of creating a WikiPage object. - DONE, TODO: new error state that needs an exit path, needs regression testing
 * getCurrentMemento/getNextMemento/getPrevMemento: No need to assign $this->dbr to a local variable, just use it directly. - DONE
 * parseRequestDateTime: MediaWiki*, also, why is the str_replace necessary? wfTimestamp/MWTimestamp should just handle it properly. - DONE
 * chooseBestTimestamp: Documentation says it already is in TS_MW format, so why do you need to pass it through wfTimestamp again? - DONE
 * constructMementoLinkHeaderRelationEntry/constructLinkRelationHeader: For easier security review, it would be good to specify if this is going into raw HTML, or what. Is everything already escaped? Ok, found where it is populated. Just add a comment explaining what inputs they will be. - DONE
 * constructTimeMapLinkHeader: Will using a PROTO_RELATIVE url be ok? - Needs clarification, Memento was written with standards-based URIs in mind (containing schemes), not sure if Memento would correctly work with just //example.com/path in the headers
 * getFullNamespacePageTitle: Use Title::getPrefixedText - Can't use getPrefixedText, need URL-version of the page title, without spaces
 * generateRecommendedLinkHeaderRelations: Why array_push over $linkRelations[] = $entry;? - DONE, quite an elegant suggestion, replaced all instances of array_push throughout the extension
 * renderError: I would recommend using ErrorPageError rather than writing your own implementation. - We need the ability to support 'traditional' status codes (like 404) for some of our users, hence the if/else block; normally we would use showErrorPage
 * mementoPageResourceFactory: Use === 0 - DONE
 * fixTemplate: As stated in the @fixme, this should support the parser cache. Also, use dbr->selectRow. - changed to dbr->selectRow; still not sure how to support the parser cache properly yet; chose $parser->disableCache for now; needs regression testing
 * Constructor should be at the top of the class definition for ease of reading.- DONE

MementoResourceDirectlyAccessed.php

 * alterHeaders: $pageID and $oldID are never used, use $uri === $tguri - DONE

MementoResourceFrom200TimeNegotiation.php

 * alterHeaders: You can use !$titleObj->inNamespaces( $this->conf->get('ExcludeNamespaces') ). I think this can be used in a few other places too. - That's what I get for thinking in Python while writing PHP
 * alterEntity: $pageID is not used. When constructing an Article object, you just pass the arguments directly, no need to do $title = $titleObj, that actually just creates a local variable named $title. (PHP doesn't support keyword arguments like say, Python.) That said, do you need to construct an Article object? Will Revision::newFromId( $id ) work? I don't like how the clearHTML and addWikiText part works... - We'll have to discuss further if there is a better way of accomplishing this, I didn't see any other way of accomplishing this; this whole class is only instantiated if the option $wgMementoTimeNegotiation = '200' 

OriginalResourceDirectlyAccessed.php

 * alterHeaders: $requestURL not used anywhere. Strict comparison for $uri === $tguri. $pageID isn't used anywhere. - DONE

TimeGateResourceFrom302TimeNegotiation.php

 * alterHeaders: If you want to redirect to a URL, just use OutputPage::redirect. -  DONE, needs regression testing OutputPage::redirect($url, 302) didn't work, it gave a 200 instead