r114081 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r114080‎ | r114081 | r114082 >
Date:22:26, 17 March 2012
Author:jeroendedauw
Status:reverted (Comments)
Tags:gerritmigration 
Comment:
adding special page with scaffolding for caching chunks of HTML
Modified paths:
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialCachedPage.php (added) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/language/messages.inc
@@ -1749,6 +1749,11 @@
17501750 'allpagesbadtitle',
17511751 'allpages-bad-ns',
17521752 ),
 1753+ 'cachedspecial' => array(
 1754+ 'cachedspecial-viewing-cached-ttl',
 1755+ 'cachedspecial-viewing-cached-ts',
 1756+ 'cachedspecial-refresh-now',
 1757+ ),
17531758 'categories' => array(
17541759 'categories',
17551760 'categories-summary',
@@ -3935,4 +3940,5 @@
39363941 'feedback' => 'Feedback',
39373942 'apierrors' => 'API errors',
39383943 'duration' => 'Durations',
 3944+ 'cachedspecial' => 'SpecialCachedPage',
39393945 );
Index: trunk/phase3/includes/AutoLoader.php
@@ -823,6 +823,7 @@
824824 'SpecialBlockList' => 'includes/specials/SpecialBlockList.php',
825825 'SpecialBlockme' => 'includes/specials/SpecialBlockme.php',
826826 'SpecialBookSources' => 'includes/specials/SpecialBooksources.php',
 827+ 'SpecialCachedPage' => 'includes/specials/SpecialCachedPage.php',
827828 'SpecialCategories' => 'includes/specials/SpecialCategories.php',
828829 'SpecialChangeEmail' => 'includes/specials/SpecialChangeEmail.php',
829830 'SpecialChangePassword' => 'includes/specials/SpecialChangePassword.php',
Index: trunk/phase3/includes/specials/SpecialCachedPage.php
@@ -0,0 +1,215 @@
 2+<?php
 3+
 4+/**
 5+ * Abstract special page class with scaffolding for caching the HTML output.
 6+ *
 7+ * To enable the caching functionality, the cacheExpiry field should be set
 8+ * in the constructor.
 9+ *
 10+ * To add HTML that should be cached, use addCachedHTML like this:
 11+ * $this->addCachedHTML( array( $this, 'displayCachedContent' ) );
 12+ *
 13+ * After adding the last HTML that should be cached, call $this->saveCache();
 14+ *
 15+ * @since 1.20
 16+ *
 17+ * @file SpecialCachedPage.php
 18+ * @ingroup SpecialPage
 19+ *
 20+ * @licence GNU GPL v2 or later
 21+ * @author Jeroen De Dauw < jeroendedauw@gmail.com >
 22+ */
 23+abstract class SpecialCachedPage extends SpecialPage {
 24+
 25+ /**
 26+ * The time to live for the cache, in seconds or a unix timestamp indicating the point of expiry.
 27+ *
 28+ * @since 1.20
 29+ * @var integer|null
 30+ */
 31+ protected $cacheExpiry = null;
 32+
 33+ /**
 34+ * List of HTML chunks to be cached (if !hasCached) or that where cashed (of hasCached).
 35+ * If no cached already, then the newly computed chunks are added here,
 36+ * if it as cached already, chunks are removed from this list as they are needed.
 37+ *
 38+ * @since 1.20
 39+ * @var array
 40+ */
 41+ protected $cachedChunks;
 42+
 43+ /**
 44+ * Indicates if the to be cached content was already cached.
 45+ * Null if this information is not available yet.
 46+ *
 47+ * @since 1.20
 48+ * @var boolean|null
 49+ */
 50+ protected $hasCached = null;
 51+
 52+ /**
 53+ * Main method.
 54+ *
 55+ * @since 1.20
 56+ *
 57+ * @param string|null $subPage
 58+ */
 59+ public function execute( $subPage ) {
 60+ if ( $this->getRequest()->getText( 'action' ) === 'purge' ) {
 61+ $this->hasCached = false;
 62+ }
 63+
 64+ if ( !is_null( $this->cacheExpiry ) ) {
 65+ $this->initCaching();
 66+
 67+ if ( $this->hasCached === true ) {
 68+ $this->getOutput()->setSubtitle( $this->getCachedNotice( $subPage ) );
 69+ }
 70+ }
 71+ }
 72+
 73+ /**
 74+ * Returns a message that notifies the user he/she is looking at
 75+ * a cached version of the page, including a refresh link.
 76+ *
 77+ * @since 1.20
 78+ *
 79+ * @param string|null $subPage
 80+ *
 81+ * @return string
 82+ */
 83+ protected function getCachedNotice( $subPage ) {
 84+ $refreshArgs = $_GET;
 85+ unset( $refreshArgs['title'] );
 86+ $refreshArgs['action'] = 'purge';
 87+
 88+ $refreshLink = Linker::link(
 89+ $this->getTitle( $subPage ),
 90+ $this->msg( 'cachedspecial-refresh-now' )->escaped(),
 91+ array(),
 92+ $refreshArgs
 93+ );
 94+
 95+ if ( $this->cacheExpiry < 1000000000 ) {
 96+ $message = $this->msg(
 97+ 'cachedspecial-viewing-cached-ttl',
 98+ $this->getLanguage()->duration( $this->cacheExpiry )
 99+ )->escaped();
 100+ }
 101+ else {
 102+ $message = $this->msg(
 103+ 'cachedspecial-viewing-cached-ts'
 104+ )->escaped();
 105+ }
 106+
 107+ return $message . ' ' . $refreshLink;
 108+ }
 109+
 110+ /**
 111+ * Initializes the caching if not already done so.
 112+ * Should be called before any of the caching functionality is used.
 113+ *
 114+ * @since 1.20
 115+ */
 116+ protected function initCaching() {
 117+ if ( is_null( $this->hasCached ) ) {
 118+ $cachedChunks = wfGetCache( CACHE_ANYTHING )->get( $this->getCacheKey() );
 119+
 120+ $this->hasCached = is_array( $cachedChunks );
 121+ $this->cachedChunks = $this->hasCached ? $cachedChunks : array();
 122+ }
 123+ }
 124+
 125+ /**
 126+ * Add some HTML to be cached.
 127+ * This is done by providing a callback function that should
 128+ * return the HTML to be added. It will only be called if the
 129+ * item is not in the cache yet or when the cache has been invalidated.
 130+ *
 131+ * @since 1.20
 132+ *
 133+ * @param {function} $callback
 134+ * @param array $args
 135+ * @param string|null $key
 136+ */
 137+ public function addCachedHTML( $callback, $args = array(), $key = null ) {
 138+ $this->initCaching();
 139+
 140+ if ( $this->hasCached ) {
 141+ $html = '';
 142+
 143+ if ( is_null( $key ) ) {
 144+ $itemKey = array_keys( array_slice( $this->cachedChunks, 0, 1 ) );
 145+ $itemKey = array_shift( $itemKey );
 146+
 147+ if ( !is_integer( $itemKey ) ) {
 148+ wfWarn( "Attempted to get item with non-numeric key while the next item in the queue has a key ($itemKey) in " . __METHOD__ );
 149+ }
 150+ elseif ( is_null( $itemKey ) ) {
 151+ wfWarn( "Attempted to get an item while the queue is empty in " . __METHOD__ );
 152+ }
 153+ else {
 154+ $html = array_shift( $this->cachedChunks );
 155+ }
 156+ }
 157+ else {
 158+ if ( array_key_exists( $key, $this->cachedChunks ) ) {
 159+ $html = $this->cachedChunks[$key];
 160+ unset( $this->cachedChunks[$key] );
 161+ }
 162+ else {
 163+ wfWarn( "There is no item with key '$key' in this->cachedChunks in " . __METHOD__ );
 164+ }
 165+ }
 166+ }
 167+ else {
 168+ $html = call_user_func_array( $callback, $args );
 169+
 170+ if ( is_null( $key ) ) {
 171+ $this->cachedChunks[] = $html;
 172+ }
 173+ else {
 174+ $this->cachedChunks[$key] = $html;
 175+ }
 176+ }
 177+
 178+ $this->getOutput()->addHTML( $html );
 179+ }
 180+
 181+ /**
 182+ * Saves the HTML to the cache in case it got recomputed.
 183+ * Should be called after the last time anything is added via addCachedHTML.
 184+ *
 185+ * @since 1.20
 186+ */
 187+ public function saveCache() {
 188+ if ( $this->hasCached === false && !empty( $this->cachedChunks ) ) {
 189+ wfGetCache( CACHE_ANYTHING )->set( $this->getCacheKey(), $this->cachedChunks, $this->cacheExpiry );
 190+ }
 191+ }
 192+
 193+ /**
 194+ * Sets the time to live for the cache, in seconds or a unix timestamp indicating the point of expiry..
 195+ *
 196+ * @since 1.20
 197+ *
 198+ * @param integer $cacheExpiry
 199+ */
 200+ protected function setExpirey( $cacheExpiry ) {
 201+ $this->cacheExpiry = $cacheExpiry;
 202+ }
 203+
 204+ /**
 205+ * Returns the cache key to use to cache this page's HTML output.
 206+ * Is constructed from the special page name and language code.
 207+ *
 208+ * @since 1.20
 209+ *
 210+ * @return string
 211+ */
 212+ protected function getCacheKey() {
 213+ return wfMemcKey( $this->mName, $this->getLanguage()->getCode() );
 214+ }
 215+
 216+}
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -2661,6 +2661,11 @@
26622662 It may contain one or more characters which cannot be used in titles.',
26632663 'allpages-bad-ns' => '{{SITENAME}} does not have namespace "$1".',
26642664
 2665+# SpecialCachedPage
 2666+'cachedspecial-viewing-cached-ttl' => 'You are viewing a cached version of this page, which can be up to $1 old.',
 2667+'cachedspecial-viewing-cached-ts' => 'You are viewing a cached version of this page, which might not be completely actual.',
 2668+'cachedspecial-refresh-now' => 'View latest.',
 2669+
26652670 # Special:Categories
26662671 'categories' => 'Categories',
26672672 'categories-summary' => '', # do not translate or duplicate this message to other languages

Follow-up revisions

RevisionCommit summaryAuthorDate
r114083follow up to r114081jeroendedauw22:26, 17 March 2012
r114101follow up to r114081: qqq, and to r114082: match renamejeroendedauw16:27, 18 March 2012
r114137Follow up to r114081;jeroendedauw13:47, 19 March 2012
r114147Follow up to r114081; address krinkles commentjeroendedauw16:14, 19 March 2012
r114164some refactoring to allow for nicer usage in deriving classesjeroendedauw18:40, 19 March 2012
r114326Revert r114067, r114071, r114075, r114079, r114081, r114082, r114084, r114086......catrope23:03, 20 March 2012

Comments

#Comment by Raymond (talk | contribs)   09:27, 18 March 2012

Especially for the content of $1: Please add message documentation for the newly added messages. Thanks.

#Comment by Reedy (talk | contribs)   16:13, 18 March 2012

duration needs to become formatDuration

#Comment by Nikerabbit (talk | contribs)   09:01, 19 March 2012

$_GET ? Use WebRequest.

#Comment by Jeroen De Dauw (talk | contribs)   13:48, 19 March 2012

r114137 :)

#Comment by Krinkle (talk | contribs)   14:15, 19 March 2012
+		if ( $this->cacheExpiry < 1000000000 ) {
..
+			wfGetCache( CACHE_ANYTHING )->set( $this->getCacheKey(), $this->cachedChunks, $this->cacheExpiry );

The BagOStuff class (of which an instance is returned via ObjectCache via wfGetCache) uses 86400 * 3650 (10 years) which doesn't match 1000000000 (32 years).

This should match the BagOStuff::convertExpiry (source code) of 86400 * 3650, else weird edge cases will occur.

Marking FIXME for the above.


+'cachedspecial-viewing-cached-ttl' => 'You are viewing a cached version of this page, which can be up to $1 old.',
+'cachedspecial-viewing-cached-ts' => 'You are viewing a cached version of this page, which might not be completely actual.',
 ...
+		if ( $this->cacheExpiry < 1000000000 ) {
+			$message = $this->msg(
+				'cachedspecial-viewing-cached-ttl',
+				$this->getLanguage()->duration( $this->cacheExpiry )
+			)->escaped();
+		}
+		else {
+			$message = $this->msg(
+				'cachedspecial-viewing-cached-ts'
+			)->escaped();
+		}

The UI can do than this. I think the user wants to know in both cases:

  • how old it is (store "last modified", should be simple)

And maybe:

  • how long until the next refresh (computable in both cases I think)
  • or: how often it will be refreshed

So that it says something like "cached on date X, refreshed every 30 days" and not "cache can be up to 30 days old". Basically matching the QueryPage type special pages.


Right now it is showing how it it could be (in ttl case), and nothing useful at all (in the ts case).

#Comment by Jeroen De Dauw (talk | contribs)   16:20, 19 March 2012

Ok, changed to the value you suggested in r114147. However do note that I got the value that I used by looking through the code as well, which was in MWMemcached, line 984.

The main purpose of this message is to notify the user that what they are seeing might not be the most recent. Note how this is already more info then you get on actual article pages. In case of the ttl, showing the max age is a nice addition since it can easily be done. Unless for some good reason you want to provide the user with more info, storing the date somewhere and doing a query for it is waaaaay to much work. If you have some use case for it, then I suggest adding it as new feature, which is off unless enabled in the deriving class.

Status & tagging log