From ea1781053eb3842dc149aa7065edb8c028ff88de Mon Sep 17 00:00:00 2001
From: "Alexia E. Smith" <washuu@gmail.com>
Date: Fri, 7 Dec 2018 12:26:47 -0600
Subject: [PATCH] Cancel the transaction if the file fails to move.  This
 prevents losing files when there is a database error.

Bug: T211442
Change-Id: I4aaa8af50d684de9d72224d43dfe5209b930810f
---
 includes/MovePage.php | 53 ++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/includes/MovePage.php b/includes/MovePage.php
index bb76395dca..ffe4faba40 100644
--- a/includes/MovePage.php
+++ b/includes/MovePage.php
@@ -249,23 +249,7 @@ class MovePage {
 			return $status;
 		}
 
-		// If it is a file, move it first.
-		// It is done before all other moving stuff is done because it's hard to revert.
 		$dbw = wfGetDB( DB_MASTER );
-		if ( $this->oldTitle->getNamespace() == NS_FILE ) {
-			$file = wfLocalFile( $this->oldTitle );
-			$file->load( File::READ_LATEST );
-			if ( $file->exists() ) {
-				$status = $file->move( $this->newTitle );
-				if ( !$status->isOK() ) {
-					return $status;
-				}
-			}
-			// Clear RepoGroup process cache
-			RepoGroup::singleton()->clearCache( $this->oldTitle );
-			RepoGroup::singleton()->clearCache( $this->newTitle ); # clear false negative cache
-		}
-
 		$dbw->startAtomic( __METHOD__ );
 
 		Hooks::run( 'TitleMoveStarting', [ $this->oldTitle, $this->newTitle, $user ] );
@@ -394,6 +378,16 @@ class MovePage {
 			$store->duplicateAllAssociatedEntries( $this->oldTitle, $this->newTitle );
 		}
 
+		// If it is a file then move it last.
+		// This is done after all database changes so that file system errors cancel the transaction.
+		if ( $this->oldTitle->getNamespace() == NS_FILE ) {
+			$status = $this->moveFile( $this->oldTitle, $this->newTitle );
+			if ( !$status->isOK() ) {
+				$dbw->cancelAtomic( __METHOD__ );
+				return $status;
+			}
+		}
+
 		Hooks::run(
 			'TitleMoveCompleting',
 			[ $this->oldTitle, $this->newTitle,
@@ -427,6 +421,33 @@ class MovePage {
 		return Status::newGood();
 	}
 
+	/**
+	 * Move a file associated with a page to a new location.
+	 * Can also be used to revert after a DB failure.
+	 *
+	 * @access private
+	 * @param Title Old location to move the file from.
+	 * @param Title New location to move the file to.
+	 * @return Status
+	 */
+	private function moveFile( $oldTitle, $newTitle ) {
+		$status = Status::newFatal(
+			'cannotdelete',
+			$oldTitle->getPrefixedText()->escaped()
+		);
+
+		$file = wfLocalFile( $oldTitle );
+		$file->load( File::READ_LATEST );
+		if ( $file->exists() ) {
+			$status = $file->move( $newTitle );
+		}
+
+		// Clear RepoGroup process cache
+		RepoGroup::singleton()->clearCache( $oldTitle );
+		RepoGroup::singleton()->clearCache( $newTitle ); # clear false negative cache
+		return $status;
+	}
+
 	/**
 	 * Move page to a title which is either a redirect to the
 	 * source page or nonexistent
-- 
2.20.1