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 mw: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 mw: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 - 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

Ripped out MementoConfig.php altogether

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

One issue remains: Within MementoConfig.php I could calculate the namespaces to exclude with

   array_diff( MWNamespace::getValidNamespaces(), MWNamespace::getContentNamespaces() );

I cannot do this now, presumably because the data for getValidNamespaces() isn't fully loaded yet. Any suggestions?

I just made the values explicitly for the array to 1 through 15 rather than calculating it; not quite as flexible as I would like, but still a sensible default.

MementoResource.php

Removed MementoResourceException altogether and replaced with ErrorPageError

• 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. - DONE
• 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