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. - DONE
 * I tried to follow the coding conventions when writing it, and also I used the tool at http://tools.wmflabs.org/stylize/ to fix any missed simple items
 * I also ran Continuous integration/PHP CodeSniffer as detailed in CC/PHP and found several lines in Memento.php that were over 100 characters because the file names and class names are long; a newline has been added got get them under 100 characters
 * Add type hinting - DONE?
 * I tried this on the hooks in Memento.body.php and it did not go well
 * It has been added to MementoResource.php, TimeMapResource.php
 * 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 was confused by this definition of exists "For historical reasons, this function simply checks for the existence of the title in the page table, and will thus return false for interwiki links, special pages and the like. If you want to know if a title can be meaningfully viewed, you should probably call the isKnown method instead" -- see https://doc.wikimedia.org/mediawiki-core/master/php/html/classTitle.html#aaee062cc380f4a23526db0696b9143b4 
 * I'm not sure what I think about using static members like this. It feels weird. - DONE, non-static now

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
This whole file has been removed from the extension, the user base for it no longer desires this functionality
 * 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