r102997 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r102996‎ | r102997 | r102998 >
Date:18:05, 14 November 2011
Author:ialex
Status:resolved (Comments)
Tags:
Comment:
Some cleanup to Article::view() and related:
* Moved the page check if oldid was given from Article::fetchContent() to Article::getOldidIDFromRequest() so that it's done directly when executing Article::view(); this should not happen though for normal view requests since oldid and related are checked in MediaWiki::parseTitle()
* Removed $oldid parameter from Article::fetchContent() and always use the oldid parameter passed either in the constructor or the request; also changed call from Article::loadContent() to Article::fromContent() since the former is now only a redirect to the latter
* Moved the 'read' permission check to the beginning of Article::view() since the Title is now correct directly after calling Article::getOldID()
* Merged the two calls to the parser cache and make WikiPage::isParserCacheUsed() also return true when the latest revision id is given. Article::setOldSubtitle() is still called when the oldid is passed to display the "You are view the current revision of this article".
* Also moved the non-existing page check a bit up to avoid running a good part of useless code when the page doesn't exist
* Merged two calls to Title::quickUserCan( 'edit' ) to set edit sections to false
Modified paths:
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/WikiPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Article.php
——@@ -186,7 +186,7 @@
187187
188188 return $text;
189189 } else {
190 - $this->loadContent();
 190+ $this->fetchContent();
191191 wfProfileOut( __METHOD__ );
192192
193193 return $this->mContent;
——@@ -215,27 +215,37 @@
216216
217217 $this->mRedirectUrl = false;
218218
219 - $oldid = $wgRequest->getVal( 'oldid' );
 219+ $oldid = $wgRequest->getIntOrNull( 'oldid' );
220220
221 - if ( isset( $oldid ) ) {
222 - $oldid = intval( $oldid );
223 - if ( $wgRequest->getVal( 'direction' ) == 'next' ) {
224 - $nextid = $this->getTitle()->getNextRevisionID( $oldid );
225 - if ( $nextid ) {
226 - $oldid = $nextid;
227 - } else {
228 - $this->mRedirectUrl = $this->getTitle()->getFullURL( 'redirect=no' );
 221+ if ( $oldid === null ) {
 222+ return 0;
 223+ }
 224+
 225+ if ( $oldid !== 0 ) {
 226+ # Load the given revision and check whether the page is another one.
 227+ # In that case, update this instance to reflect the change.
 228+ $this->mRevision = Revision::newFromId( $oldid );
 229+ if ( $this->mRevision !== null ) {
 230+ // Revision title doesn't match the page title given?
 231+ if ( $this->mPage->getID() != $this->mRevision->getPage() ) {
 232+ $function = array( get_class( $this->mPage ), 'newFromID' );
 233+ $this->mPage = call_user_func( $function, $revision->getPage() );
229234 }
230 - } elseif ( $wgRequest->getVal( 'direction' ) == 'prev' ) {
231 - $previd = $this->getTitle()->getPreviousRevisionID( $oldid );
232 - if ( $previd ) {
233 - $oldid = $previd;
234 - }
235235 }
236236 }
237237
238 - if ( !$oldid ) {
239 - $oldid = 0;
 238+ if ( $wgRequest->getVal( 'direction' ) == 'next' ) {
 239+ $nextid = $this->getTitle()->getNextRevisionID( $oldid );
 240+ if ( $nextid ) {
 241+ $oldid = $nextid;
 242+ } else {
 243+ $this->mRedirectUrl = $this->getTitle()->getFullURL( 'redirect=no' );
 244+ }
 245+ } elseif ( $wgRequest->getVal( 'direction' ) == 'prev' ) {
 246+ $previd = $this->getTitle()->getPreviousRevisionID( $oldid );
 247+ if ( $previd ) {
 248+ $oldid = $previd;
 249+ }
240250 }
241251
242252 return $oldid;
——@@ -243,31 +253,30 @@
244254
245255 /**
246256 * Load the revision (including text) into this object
 257+ *
 258+ * @deprecated in 1.19; use fetchContent()
247259 */
248260 function loadContent() {
249 - if ( $this->mContentLoaded ) {
250 - return;
251 - }
252 -
253 - wfProfileIn( __METHOD__ );
254 -
255 - $this->fetchContent( $this->getOldID() );
256 -
257 - wfProfileOut( __METHOD__ );
 261+ $this->fetchContent();
258262 }
259263
260264 /**
261265 * Get text of an article from database
262266 * Does *NOT* follow redirects.
263267 *
264 - * @param $oldid Int: 0 for whatever the latest revision is
265268 * @return mixed string containing article contents, or false if null
266269 */
267 - function fetchContent( $oldid = 0 ) {
 270+ function fetchContent() {
268271 if ( $this->mContentLoaded ) {
269272 return $this->mContent;
270273 }
271274
 275+ wfProfileIn( __METHOD__ );
 276+
 277+ $this->mContentLoaded = true;
 278+
 279+ $oldid = $this->getOldID();
 280+
272281 # Pre-fill content with error message so that if something
273282 # fails we'll have something telling us what we intended.
274283 $t = $this->getTitle()->getPrefixedText();
——@@ -275,17 +284,11 @@
276285 $this->mContent = wfMsgNoTrans( 'missing-article', $t, $d ) ;
277286
278287 if ( $oldid ) {
279 - $revision = Revision::newFromId( $oldid );
280 - if ( !$revision ) {
281 - wfDebug( __METHOD__ . " failed to retrieve specified revision, id $oldid\n" );
282 - return false;
283 - }
284 - // Revision title doesn't match the page title given?
285 - if ( $this->mPage->getID() != $revision->getPage() ) {
286 - $function = array( get_class( $this->mPage ), 'newFromID' );
287 - $this->mPage = call_user_func( $function, $revision->getPage() );
288 - if ( !$this->mPage->getId() ) {
289 - wfDebug( __METHOD__ . " failed to get page data linked to revision id $oldid\n" );
 288+ # $this->mRevision might already be fetched by getOldIDFromRequest()
 289+ if ( !$this->mRevision ) {
 290+ $this->mRevision = Revision::newFromId( $oldid );
 291+ if ( !$this->mRevision ) {
 292+ wfDebug( __METHOD__ . " failed to retrieve specified revision, id $oldid\n" );
290293 return false;
291294 }
292295 }
——@@ -295,8 +298,8 @@
296299 return false;
297300 }
298301
299 - $revision = $this->mPage->getRevision();
300 - if ( !$revision ) {
 302+ $this->mRevision = $this->mPage->getRevision();
 303+ if ( !$this->mRevision ) {
301304 wfDebug( __METHOD__ . " failed to retrieve current page, rev_id " . $this->mPage->getLatest() . "\n" );
302305 return false;
303306 }
——@@ -304,14 +307,13 @@
305308
306309 // @todo FIXME: Horrible, horrible! This content-loading interface just plain sucks.
307310 // We should instead work with the Revision object when we need it...
308 - $this->mContent = $revision->getText( Revision::FOR_THIS_USER ); // Loads if user is allowed
 311+ $this->mContent = $this->mRevision->getText( Revision::FOR_THIS_USER ); // Loads if user is allowed
 312+ $this->mRevIdFetched = $this->mRevision->getId();
309313
310 - $this->mRevIdFetched = $revision->getId();
311 - $this->mContentLoaded = true;
312 - $this->mRevision =& $revision;
313 -
314314 wfRunHooks( 'ArticleAfterFetchContent', array( &$this, &$this->mContent ) );
315315
 316+ wfProfileOut( __METHOD__ );
 317+
316318 return $this->mContent;
317319 }
318320
——@@ -361,9 +363,20 @@
362364 wfProfileIn( __METHOD__ );
363365
364366 # Get variables from query string
 367+ # As side effect this will load the revision and update the title
 368+ # in a revision ID is passed in the request, so this should remain
 369+ # the first call of this method even if $oldid is used way below.
365370 $oldid = $this->getOldID();
366371
367 - # getOldID may want us to redirect somewhere else
 372+ # Another whitelist check in case getOldID() is altering the title
 373+ $permErrors = $this->getTitle()->getUserPermissionsErrors( 'read', $wgUser );
 374+ if ( count( $permErrors ) ) {
 375+ wfDebug( __METHOD__ . ": denied on secondary read check\n" );
 376+ wfProfileOut( __METHOD__ );
 377+ throw new PermissionsError( 'read', $permErrors );
 378+ }
 379+
 380+ # getOldID() may as well want us to redirect somewhere else
368381 if ( $this->mRedirectUrl ) {
369382 $wgOut->redirect( $this->mRedirectUrl );
370383 wfDebug( __METHOD__ . ": redirecting due to oldid\n" );
——@@ -372,9 +385,6 @@
373386 return;
374387 }
375388
376 - # Set page title (may be overridden by DISPLAYTITLE)
377 - $wgOut->setPageTitle( $this->getTitle()->getPrefixedText() );
378 -
379389 # If we got diff in the query, we want to see a diff page instead of the article.
380390 if ( $wgRequest->getCheck( 'diff' ) ) {
381391 wfDebug( __METHOD__ . ": showing diff page\n" );
——@@ -384,6 +394,9 @@
385395 return;
386396 }
387397
 398+ # Set page title (may be overridden by DISPLAYTITLE)
 399+ $wgOut->setPageTitle( $this->getTitle()->getPrefixedText() );
 400+
388401 $wgOut->setArticleFlag( true );
389402 # Allow frames by default
390403 $wgOut->allowClickjacking();
——@@ -395,7 +408,7 @@
396409 if ( $wgOut->isPrintable() ) {
397410 $parserOptions->setIsPrintable( true );
398411 $parserOptions->setEditSection( false );
399 - } elseif ( $wgUseETag && !$this->getTitle()->quickUserCan( 'edit' ) ) {
 412+ } elseif ( !$this->getTitle()->quickUserCan( 'edit' ) ) {
400413 $parserOptions->setEditSection( false );
401414 }
402415
——@@ -423,12 +436,8 @@
424437 }
425438 }
426439
427 - if ( !$wgUseETag && !$this->getTitle()->quickUserCan( 'edit' ) ) {
428 - $parserOptions->setEditSection( false );
429 - }
430 -
431440 # Should the parser cache be used?
432 - $useParserCache = $this->useParserCache( $oldid );
 441+ $useParserCache = $this->mPage->isParserCacheUsed( $wgUser, $oldid );
433442 wfDebug( 'Article::view using parser cache: ' . ( $useParserCache ? 'yes' : 'no' ) . "\n" );
434443 if ( $wgUser->getStubThreshold() ) {
435444 wfIncrStats( 'pcache_miss_stub' );
——@@ -449,12 +458,25 @@
450459 wfRunHooks( 'ArticleViewHeader', array( &$this, &$outputDone, &$useParserCache ) );
451460 break;
452461 case 2:
 462+ # Early abort if the page doesn't exist
 463+ if ( !$this->mPage->exists() ) {
 464+ wfDebug( __METHOD__ . ": showing missing article\n" );
 465+ $this->showMissingArticle();
 466+ wfProfileOut( __METHOD__ );
 467+ return;
 468+ }
 469+
453470 # Try the parser cache
454471 if ( $useParserCache ) {
455472 $this->mParserOutput = $parserCache->get( $this, $parserOptions );
456473
457474 if ( $this->mParserOutput !== false ) {
458 - wfDebug( __METHOD__ . ": showing parser cache contents\n" );
 475+ if ( $oldid ) {
 476+ wfDebug( __METHOD__ . ": showing parser cache contents for current rev permalink\n" );
 477+ $this->setOldSubtitle( $oldid );
 478+ } else {
 479+ wfDebug( __METHOD__ . ": showing parser cache contents\n" );
 480+ }
459481 $wgOut->addParserOutput( $this->mParserOutput );
460482 # Ensure that UI elements requiring revision ID have
461483 # the correct version information.
——@@ -468,24 +490,11 @@
469491 }
470492 break;
471493 case 3:
472 - $text = $this->getContent();
473 - if ( $text === false || $this->mPage->getID() == 0 ) {
474 - wfDebug( __METHOD__ . ": showing missing article\n" );
475 - $this->showMissingArticle();
476 - wfProfileOut( __METHOD__ );
477 - return;
478 - }
 494+ # This will set $this->mRevision if needed
 495+ $this->fetchContent();
479496
480 - # Another whitelist check in case oldid is altering the title
481 - $permErrors = $this->getTitle()->getUserPermissionsErrors( 'read', $wgUser );
482 - if ( count( $permErrors ) ) {
483 - wfDebug( __METHOD__ . ": denied on secondary read check\n" );
484 - wfProfileOut( __METHOD__ );
485 - throw new PermissionsError( 'read', $permErrors );
486 - }
487 -
488497 # Are we looking at an old revision
489 - if ( $oldid && !is_null( $this->mRevision ) ) {
 498+ if ( $oldid && $this->mRevision ) {
490499 $this->setOldSubtitle( $oldid );
491500
492501 if ( !$this->showDeletedRevisionHeader() ) {
——@@ -493,18 +502,6 @@
494503 wfProfileOut( __METHOD__ );
495504 return;
496505 }
497 -
498 - # If this "old" version is the current, then try the parser cache...
499 - if ( $oldid === $this->mPage->getLatest() && $this->useParserCache( false ) ) {
500 - $this->mParserOutput = $parserCache->get( $this, $parserOptions );
501 - if ( $this->mParserOutput ) {
502 - wfDebug( __METHOD__ . ": showing parser cache for current rev permalink\n" );
503 - $wgOut->addParserOutput( $this->mParserOutput );
504 - $wgOut->setRevisionId( $this->mPage->getLatest() );
505 - $outputDone = true;
506 - break;
507 - }
508 - }
509506 }
510507
511508 # Ensure that UI elements requiring revision ID have
——@@ -520,6 +517,7 @@
521518 # Allow extensions do their own custom view for certain pages
522519 $outputDone = true;
523520 } else {
 521+ $text = $this->getContent();
524522 $rt = Title::newFromRedirectArray( $text );
525523 if ( $rt ) {
526524 wfDebug( __METHOD__ . ": showing redirect=no page\n" );
Index: trunk/phase3/includes/WikiPage.php
——@@ -718,7 +718,7 @@
719719 return $wgEnableParserCache
720720 && $user->getStubThreshold() == 0
721721 && $this->exists()
722 - && empty( $oldid )
 722+ && ( $oldid === null || $oldid === 0 || $oldid === $this->getLatest() )
723723 && $this->mTitle->isWikitextPage();
724724 }
725725

Follow-up revisions

RevisionCommit summaryAuthorDate
r103039Fix for r102997: forgot to change that variableialex21:42, 14 November 2011
r103189Fixup for r102997. If you add returns to a profiled function,...platonides16:35, 15 November 2011
r112819* (bug 34849) Diff when editing an old version show the comparison with the c...ialex17:29, 1 March 2012

Comments

#Comment by Aaron Schulz (talk | contribs)   00:37, 15 November 2011

Took a while to inspect this.

#Comment by MrBlueSky (talk | contribs)   21:52, 28 January 2012

Bug 33967 seems to be caused by this change. In fetchContent() the page contents of the $oldid is only loaded if (!$this->mRevision), but $this->mRevision still has the old revision. getOldIDFromRequest() doesn't fetch a new revision into $this->mRevision.

#Comment by IAlex (talk | contribs)   19:06, 29 January 2012

Fixed in r110251.

#Comment by Hashar (talk | contribs)   17:14, 1 March 2012

Cause regression Bug 34849 - diff during editing an old version compares not to the current but to the old version

Status & tagging log