From 7732c79f67a2a30e48cc523c2ea333171885139c Mon Sep 17 00:00:00 2001
From: daniel <daniel.kinzler@wikimedia.de>
Date: Tue, 19 Jun 2018 18:35:11 +0200
Subject: [PATCH] Improve documentation of fields in EditPage

This is an attempt to clarify the semantics of several fields
and methods in EditPage that represent some kind of parent or base
revision.

Bug: T197685
Change-Id: I37b3803fc558fecc0c7b0c3cfb4ec93dce6997a5
---
 includes/EditPage.php | 49 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/includes/EditPage.php b/includes/EditPage.php
index 92097616f7..3c97fe66c6 100644
--- a/includes/EditPage.php
+++ b/includes/EditPage.php
@@ -235,7 +235,10 @@ class EditPage {
 	/** @var string */
 	public $action = 'submit';
 
-	/** @var bool */
+	/** @var bool Whether an edit conflict needs to be resolved. Detected based on whether
+	 * $editRevId is different from the current revision. When a conflict has successfully
+	 * been resolved by a 3-way-merge, this field is set to false.
+	 */
 	public $isConflict = false;
 
 	/** @var bool New page or new section */
@@ -301,7 +304,7 @@ class EditPage {
 	/** @var bool Has a summary been preset using GET parameter &summary= ? */
 	public $hasPresetSummary = false;
 
-	/** @var Revision|bool|null */
+	/** @var Revision|bool|null A revision object corresponding to $this->editRevId. */
 	public $mBaseRevision = false;
 
 	/** @var bool */
@@ -342,7 +345,16 @@ class EditPage {
 	/** @var string */
 	public $edittime = '';
 
-	/** @var int */
+	/** @var int ID of the current revision at the time editing was initiated on the client.
+	 * This is used to detect and resolve edit conflicts.
+	 *
+	 * @note 0 if the page did not exist at that time.
+	 * @note When starting an edit from an old revision, this still records the current
+	 * revision at the time , not the one the edit is based on.
+	 *
+	 * @see $oldid
+	 * @see getBaseRevision()
+	 */
 	private $editRevId = null;
 
 	/** @var string */
@@ -354,10 +366,16 @@ class EditPage {
 	/** @var string */
 	public $starttime = '';
 
-	/** @var int */
+	/** @var int Revision ID the edit is based on, or 0 if it's the current revision.
+	 * @see $editRevId
+	 */
 	public $oldid = 0;
 
-	/** @var int */
+	/** @var int Revision ID the edit is based on, adjusted when an edit conflict is resolved.
+	 * @see $editRevId
+	 * @see $oldid
+	 * @see getparentRevId()
+	 */
 	public $parentRevId = 0;
 
 	/** @var string */
@@ -2021,7 +2039,10 @@ ERROR;
 
 			wfDebug( "timestamp: {$timestamp}, edittime: {$this->edittime}\n" );
 
-			// Check editRevId if set, which handles same-second timestamp collisions
+			// An edit conflict is detected if the current revision is different from the
+			// revision that was current when editing was initiated on the client.
+			// This is checked based on the timestamp and revision ID.
+			// TODO: the timestamp based check can probably go away now.
 			if ( $timestamp != $this->edittime
 				|| ( $this->editRevId !== null && $this->editRevId != $latest )
 			) {
@@ -2301,7 +2322,8 @@ ERROR;
 	private function mergeChangesIntoContent( &$editContent ) {
 		$db = wfGetDB( DB_MASTER );
 
-		// This is the revision the editor started from
+		// This is the revision that was current at the time editing was initiated on the client,
+		// even if the edit was based on an old revision.
 		$baseRevision = $this->getBaseRevision();
 		$baseContent = $baseRevision ? $baseRevision->getContent() : null;
 
@@ -2332,9 +2354,16 @@ ERROR;
 	}
 
 	/**
-	 * @note: this method is very poorly named. If the user opened the form with ?oldid=X,
-	 *        one might think of X as the "base revision", which is NOT what this returns.
-	 * @return Revision|null Current version when the edit was started
+	 * Returns the revision that was current at the time editing was initiated on the client,
+	 * even if the edit was based on an old revision.
+	 *
+	 * @warning: this method is very poorly named. If the user opened the form with ?oldid=X,
+	 *        one might think of X as the "base revision", which is NOT what this returns,
+	 *        see oldid for that. One might further assume that this corresponds to the $baseRevId
+	 *        parameter of WikiPage::doEditContent, which is not the case either.
+	 *        getExpectedParentRevision() would perhaps be a better name.
+	 *
+	 * @return Revision|null Current version when editing was initiated on the client
 	 */
 	public function getBaseRevision() {
 		if ( !$this->mBaseRevision ) {
-- 
2.20.1