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.
 * Use proper PHPDoc annotations: @param type $varname brief description if not obvious

Memento.php

 * Convert i18n to JSON-based i18n
 * "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".
 * The main Memento class should be moved into it's own file (Memento.body.php) and autoloaded.


 * Memento class
 * In onImageBeforeProduceHTML: $config->get should use === true
 * In onImageBeforeProduceHTML: function looks like it could be really expensive for non-locally hosted files...
 * 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.

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.
 * MementoResourceException should extend MWException.


 * MementoResource
 * Member variable should not be named $dbr since a non-slave database could be passed in the constructor.
 * fetchMementoFromDatabase: Use $db->selectRow
 * getFirstMemento: Should check if Title::getFirstRevision returned null
 * getLastMemento: Use Revision::newFromTitle( $title ) instead of creating a WikiPage object.
 * getCurrentMemento/getNextMemento/getPrevMemento: No need to assign $this->dbr to a local variable, just use it directly.
 * parseRequestDateTime: MediaWiki*, also, why is the str_replace necessary? wfTimestamp/MWTimestamp should just handle it properly.
 * chooseBestTimestamp: Documentation says it already is in TS_MW format, so why do you need to pass it through wfTimestamp again?
 * 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.
 * constructTimeMapLinkHeader: Will using a PROTO_RELATIVE url be ok?
 * getFullNamespacePageTitle: Use Title::getPrefixedText
 * generateRecommendedLinkHeaderRelations: Why array_push over $linkRelations[] = $entry;?
 * renderError: I would recommend using ErrorPageError rather than writing your own implementation.
 * mementoPageResourceFactory: Use === 0
 * fixTemplate: As stated in the @fixme, this should support the parser cache. Also, use dbr->selectRow.
 * Constructor should be at the top of the class definition for ease of reading.

MementoResourceDirectlyAccessed.php

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

MementoResourceFrom200TimeNegotiation.php

 * alterHeaders: You can use !$titleObj->inNamespaces( $this->conf->get('ExcludeNamespaces') ). I think this can be used in a few other places too.
 * 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...

OriginalResourceDirectlyAccessed.php

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

TimeGateResourceFrom302TimeNegotiation.php

 * alterHeaders: If you want to redirect to a URL, just use OutputPage::redirect.