r108274 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r108273‎ | r108274 | r108275 >
Date:20:00, 6 January 2012
Author:ialex
Status:resolved (Comments)
Tags:brion 
Comment:
* Added WikiPage to RequestContext and related so that it can be shared to avoid creating a new object each time and thus avoiding database queries to load the state of the object
* Added Article::getPage() as accessor to the WikiPage object so that it can be set in the context from MediaWiki::initializeArticle()
* Use it WikiPage::main() to call doViewUpdates()

I'm doing to this now so that I can revert r105790 and use the WikiPage object before the 1.19 release
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/Wiki.php (modified) (history)
  • /trunk/phase3/includes/context/ContextSource.php (modified) (history)
  • /trunk/phase3/includes/context/DerivativeContext.php (modified) (history)
  • /trunk/phase3/includes/context/IContextSource.php (modified) (history)
  • /trunk/phase3/includes/context/RequestContext.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Article.php
——@@ -150,6 +150,7 @@
151151
152152 /**
153153 * Get the title object of the article
 154+ *
154155 * @return Title object of this page
155156 */
156157 public function getTitle() {
——@@ -157,6 +158,16 @@
158159 }
159160
160161 /**
 162+ * Get the WikiPage object of this instance
 163+ *
 164+ * @since 1.19
 165+ * @return WikiPage
 166+ */
 167+ public function getPage() {
 168+ return $this->mPage;
 169+ }
 170+
 171+ /**
161172 * Clear the object
162173 */
163174 public function clear() {
Index: trunk/phase3/includes/context/RequestContext.php
——@@ -40,6 +40,11 @@
4141 private $title;
4242
4343 /**
 44+ * @var WikiPage
 45+ */
 46+ private $wikipage;
 47+
 48+ /**
4449 * @var OutputPage
4550 */
4651 private $output;
——@@ -104,6 +109,33 @@
105110 }
106111
107112 /**
 113+ * Set the WikiPage object
 114+ *
 115+ * @since 1.19
 116+ * @param $p WikiPage object
 117+ */
 118+ public function setWikiPage( WikiPage $p ) {
 119+ $this->wikipage = $p;
 120+ }
 121+
 122+ /**
 123+ * Get the WikiPage object
 124+ *
 125+ * @since 1.19
 126+ * @return WikiPage
 127+ */
 128+ public function getWikiPage() {
 129+ if ( $this->wikipage === null ) {
 130+ $title = $this->getTitle();
 131+ if ( $title === null ) {
 132+ throw new MWException( __METHOD__ . ' called without Title object set' );
 133+ }
 134+ $this->wikipage = WikiPage::factory( $title );
 135+ }
 136+ return $this->wikipage;
 137+ }
 138+
 139+ /**
108140 * @param $o OutputPage
109141 */
110142 public function setOutput( OutputPage $o ) {
Index: trunk/phase3/includes/context/IContextSource.php
——@@ -43,6 +43,14 @@
4444 public function getTitle();
4545
4646 /**
 47+ * Get the WikiPage object
 48+ *
 49+ * @since 1.19
 50+ * @return WikiPage
 51+ */
 52+ public function getWikiPage();
 53+
 54+ /**
4755 * Get the OutputPage object
4856 *
4957 * @return OutputPage object
Index: trunk/phase3/includes/context/ContextSource.php
——@@ -79,6 +79,16 @@
8080 }
8181
8282 /**
 83+ * Get the WikiPage object
 84+ *
 85+ * @since 1.19
 86+ * @return WikiPage
 87+ */
 88+ public function getWikiPage() {
 89+ return $this->getContext()->getWikiPage();
 90+ }
 91+
 92+ /**
8393 * Get the OutputPage object
8494 *
8595 * @since 1.18
Index: trunk/phase3/includes/context/DerivativeContext.php
——@@ -42,6 +42,11 @@
4343 private $title;
4444
4545 /**
 46+ * @var WikiPage
 47+ */
 48+ private $wikipage;
 49+
 50+ /**
4651 * @var OutputPage
4752 */
4853 private $output;
——@@ -114,6 +119,32 @@
115120 }
116121
117122 /**
 123+ * Set the WikiPage object
 124+ *
 125+ * @since 1.19
 126+ * @param $p WikiPage object
 127+ */
 128+ public function setWikiPage( WikiPage $p ) {
 129+ $this->wikipage = $p;
 130+ }
 131+
 132+ /**
 133+ * Get the WikiPage object
 134+ *
 135+ * @since 1.19
 136+ * @return WikiPage
 137+ */
 138+ public function getWikiPage() {
 139+ if ( !is_null( $this->wikipage ) ) {
 140+ return $this->wikipage;
 141+ } else {
 142+ return $this->getContext()->getWikiPage();
 143+ }
 144+ }
 145+
 146+ /**
 147+ * Set the OutputPage object
 148+ *
118149 * @param $o OutputPage
119150 */
120151 public function setOutput( OutputPage $o ) {
Index: trunk/phase3/includes/Wiki.php
——@@ -337,19 +337,21 @@
338338
339339 wfProfileIn( __METHOD__ );
340340
341 - $request = $this->context->getRequest();
342341 $title = $this->context->getTitle();
343 -
344 - $action = $request->getVal( 'action', 'view' );
345342 $article = Article::newFromTitle( $title, $this->context );
 343+ $this->context->setWikiPage( $article->getPage() );
346344 // NS_MEDIAWIKI has no redirects.
347345 // It is also used for CSS/JS, so performance matters here...
348346 if ( $title->getNamespace() == NS_MEDIAWIKI ) {
349347 wfProfileOut( __METHOD__ );
350348 return $article;
351349 }
 350+
 351+ $request = $this->context->getRequest();
 352+
352353 // Namespace might change when using redirects
353354 // Check for redirects ...
 355+ $action = $request->getVal( 'action', 'view' );
354356 $file = ( $title->getNamespace() == NS_FILE ) ? $article->getFile() : null;
355357 if ( ( $action == 'view' || $action == 'render' ) // ... for actions that show content
356358 && !$request->getVal( 'oldid' ) && // ... and are not old revisions
——@@ -384,10 +386,12 @@
385387 $rarticle->setRedirectedFrom( $title );
386388 $article = $rarticle;
387389 $this->context->setTitle( $target );
 390+ $this->context->setWikiPage( $article->getPage() );
388391 }
389392 }
390393 } else {
391394 $this->context->setTitle( $article->getTitle() );
 395+ $this->context->setWikiPage( $article->getPage() );
392396 }
393397 }
394398
——@@ -604,8 +608,7 @@
605609 $cache->loadFromFileCache( $this->context );
606610 }
607611 # Do any stats increment/watchlist stuff
608 - $page = WikiPage::factory( $this->getTitle() );
609 - $page->doViewUpdates( $this->context->getUser() );
 612+ $this->context->getWikiPage()->doViewUpdates( $this->context->getUser() );
610613 # Tell OutputPage that output is taken care of
611614 $this->context->getOutput()->disable();
612615 wfProfileOut( 'main-try-filecache' );

Follow-up revisions

RevisionCommit summaryAuthorDate
r108902Per Aaron, fix for r108274: added canUseWikiPage() to context objects to know...ialex14:27, 14 January 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r105790Moved view count from WikiPage to Title; avoids an extra DB query when showin...ialex11:30, 11 December 2011

Comments

#Comment by Reedy (talk | contribs)   22:59, 6 January 2012 
+	/**
+	 * Get the WikiPage object
+	 *
+	 * @since 1.19
+	 * @return WikiPage
+	 */
+	public function getWikiPage() {
+		if ( !is_null( $this->wikipage ) ) {
+			return $this->wikipage;
+		} else {
+			return $this->getContext()->getWikiPage();
+		}
+	}

Shouldn't that be caching the result from $this->getContext()->getWikiPage() ?

#Comment by IAlex (talk | contribs)   08:48, 7 January 2012

DerivativeContext only stores overrides that you have to set with set*() methods.

#Comment by Aaron Schulz (talk | contribs)   00:27, 13 January 2012

I kind of dislike how getWikiPage can throw exceptions for null or NS_SPECIAL titles in RequestContext.php.

#Comment by IAlex (talk | contribs)   14:58, 13 January 2012

The only other solution I see is to silently return null, but this will certainly throw a fatal error at some point because someone forgot to check that the page is not in a special namespace.

#Comment by Aaron Schulz (talk | contribs)   00:29, 13 January 2012 
$this->context->getWikiPage()->doViewUpdates( $this->context->getUser() );

This assumes that only wikipages are cacheable. I'd rather this be a bit more general, such as being able to check *if* a wikipage is set first. This relates to the exception problem.

#Comment by IAlex (talk | contribs)   15:00, 13 January 2012

What's the difference with the previous code?

#Comment by Aaron Schulz (talk | contribs)   18:32, 13 January 2012 
$page = WikiPage::factory( $this->getTitle() );

It always created a page from the title. Now it has to worry about the wikipage being set. I'd recommend letting getWikiPage() return null and then having the doViewUpdates() code check if a wiki page is set before calling that function.

#Comment by IAlex (talk | contribs)   19:53, 13 January 2012

It doesn't have to worry about WikiPage being set since one will be created if needed (and it will since MediaWiki::initializeArticle() is not called at that point).

Also I dislike returning null because you can be sure that someone will forget to do appropriate checks and thus will get a fatal error, an exception is much better for debugging.

#Comment by Aaron Schulz (talk | contribs)   20:40, 13 January 2012

That works unless we change filecache to work on some special pages :)

Maybe we can keep the exceptions and make a hasWikiPage() function or some clear safety check function?

#Comment by IAlex (talk | contribs)   14:28, 14 January 2012

Added canUseWikiPage() in r108902.

Status & tagging log