From: Gergő Tisza <tgr.huwiki@gmail.com>
Date: Wed, 11 Jul 2018 08:48:49 +0000 (+0200)
Subject: Make load* methods of DifferenceEngine idempotent
X-Git-Tag: 1.34.0-rc.0~4659^2~1
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=04f16a204b89c037a2a555615ca0cd9a66837c99

Make load* methods of DifferenceEngine idempotent

These methods returned a boolean indicating whether loading the
data was successful, but then always returned true on subsequent
calls. Fix that.

This changes public methods but there's no usage in Gerrit (some
of them are called but the return value is ignored), no use case
for a caller to care, and the previous behavior has been
undocumented and unreliable, so there is no deprecation period.

Change-Id: I3998aeea66972f33274e05fa5a74d6ce7fdc56b6
---

diff --git a/includes/diff/DifferenceEngine.php b/includes/diff/DifferenceEngine.php
index 1d9ad05a36..fbc3dd3c60 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -36,19 +36,19 @@ class DifferenceEngine extends ContextSource {
 	 */
 	const DIFF_VERSION = '1.12';
 
-	/** @var int */
+	/** @var int Revision ID or 0 for current */
 	public $mOldid;
 
-	/** @var int */
+	/** @var int|string Revision ID or null for current or an alias such as 'next' */
 	public $mNewid;
 
 	private $mOldTags;
 	private $mNewTags;
 
-	/** @var Content */
+	/** @var Content|null */
 	public $mOldContent;
 
-	/** @var Content */
+	/** @var Content|null */
 	public $mNewContent;
 
 	/** @var Language */
@@ -60,10 +60,10 @@ class DifferenceEngine extends ContextSource {
 	/** @var Title */
 	public $mNewPage;
 
-	/** @var Revision */
+	/** @var Revision|null */
 	public $mOldRev;
 
-	/** @var Revision */
+	/** @var Revision|null */
 	public $mNewRev;
 
 	/** @var bool Have the revisions IDs been loaded */
@@ -75,6 +75,13 @@ class DifferenceEngine extends ContextSource {
 	/** @var int How many text blobs have been loaded, 0, 1 or 2? */
 	public $mTextLoaded = 0;
 
+	/**
+	 * Was the content overridden via setContent()?
+	 * If the content was overridden, most internal state (e.g. mOldid or mOldRev) should be ignored.
+	 * @var bool
+	 */
+	protected $isContentOverridden = false;
+
 	/** @var bool Was the diff fetched from cache? */
 	public $mCacheHit = false;
 
@@ -1341,6 +1348,7 @@ class DifferenceEngine extends ContextSource {
 
 		$this->mTextLoaded = 2;
 		$this->mRevisionsLoaded = true;
+		$this->isContentOverridden = true;
 	}
 
 	/**
@@ -1420,11 +1428,11 @@ class DifferenceEngine extends ContextSource {
 	 * to false. This is impossible via ordinary user input, and is provided for
 	 * API convenience.
 	 *
-	 * @return bool
+	 * @return bool Whether both revisions were loaded successfully.
 	 */
 	public function loadRevisionData() {
 		if ( $this->mRevisionsLoaded ) {
-			return true;
+			return $this->isContentOverridden || $this->mNewRev && $this->mOldRev;
 		}
 
 		// Whether it succeeds or fails, we don't want to try again
@@ -1500,11 +1508,11 @@ class DifferenceEngine extends ContextSource {
 	/**
 	 * Load the text of the revisions, as well as revision data.
 	 *
-	 * @return bool
+	 * @return bool Whether the content of both revisions could be loaded successfully.
 	 */
 	public function loadText() {
 		if ( $this->mTextLoaded == 2 ) {
-			return true;
+			return $this->loadRevisionData() && $this->mOldContent && $this->mNewContent;
 		}
 
 		// Whether it succeeds or fails, we don't want to try again
@@ -1535,11 +1543,11 @@ class DifferenceEngine extends ContextSource {
 	/**
 	 * Load the text of the new revision, not the old one
 	 *
-	 * @return bool
+	 * @return bool Whether the content of the new revision could be loaded successfully.
 	 */
 	public function loadNewText() {
 		if ( $this->mTextLoaded >= 1 ) {
-			return true;
+			return $this->loadRevisionData();
 		}
 
 		$this->mTextLoaded = 1;