From: Victor Vasiliev <vasilievvv@users.mediawiki.org>
Date: Sun, 29 Mar 2009 13:42:29 +0000 (+0000)
Subject: Improve image moving error handling:
X-Git-Tag: 1.31.0-rc.0~42290
X-Git-Url: http://git.cyclocoop.org/%24action?a=commitdiff_plain;h=3eccb0169a535a2b487d3694007c6473357242a7;p=lhc%2Fweb%2Fwiklou.git

Improve image moving error handling:
* Do image moving before page moving. That will allow to avoid situations when image page is moved, while image itself isn't.
* Add safeguard code that checks all files before moving them
* More debug logging
---

diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index 02a0fe1216..f8a9bea0ae 100644
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -308,6 +308,7 @@ it from source control: http://www.mediawiki.org/wiki/Download_from_SVN
 * (bug 18190) Proper parsing in MediaWiki:Sharedupload message
 * (bug 17617) HTML cleanup for ImagePage
 * (bug 17964) namespaceDupes.php no longer fails on an empty interwiki table
+* Improved error handling for image moving
 
 == API changes in 1.15 ==
 * (bug 16858) Revamped list=deletedrevs to make listing deleted contributions
diff --git a/includes/Status.php b/includes/Status.php
index 185ea6e5d7..1eb2b66a45 100644
--- a/includes/Status.php
+++ b/includes/Status.php
@@ -175,7 +175,10 @@ class Status {
 		$result = array();
 		foreach ( $this->errors as $error ) {
 			if ( $error['type'] == 'error' )
-				$result[] = $error['message'];
+				if( $error['params'] )
+					$result[] = array_merge( array( $error['message'] ), $error['params'] );
+				else
+					$result[] = $error['message'];
 		}
 		return $result;
 	}
diff --git a/includes/Title.php b/includes/Title.php
index 3ce99d80cc..782169cbf9 100644
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -2664,6 +2664,18 @@ class Title {
 			return $err;
 		}
 
+		// If it is a file, more it first. It is done before all other moving stuff is done because it's hard to revert
+		$dbw = wfGetDB( DB_MASTER );
+		if( $this->getNamespace() == NS_FILE ) {
+			$file = wfLocalFile( $this );
+			if( $file->exists() ) {
+				$status = $file->move( $nt );
+				if( !$status->isOk() ) {
+					return $status->getErrorsArray();
+				}
+			}
+		}
+
 		$pageid = $this->getArticleID();
 		$protected = $this->isProtected();
 		if( $nt->exists() ) {
@@ -2690,7 +2702,6 @@ class Title {
 		// we can't actually distinguish it from a default here, and it'll
 		// be set to the new title even though it really shouldn't.
 		// It'll get corrected on the next edit, but resetting cl_timestamp.
-		$dbw = wfGetDB( DB_MASTER );
 		$dbw->update( 'categorylinks',
 			array(
 				'cl_sortkey' => $nt->getPrefixedText(),
@@ -2873,18 +2884,6 @@ class Title {
 			$redirectSuppressed = true;
 		}
 
-		# Move an image if this is a file
-		if( $this->getNamespace() == NS_FILE ) {
-			$file = wfLocalFile( $this );
-			if( $file->exists() ) {
-				$status = $file->move( $nt );
-				if( !$status->isOk() ) {
-					$dbw->rollback();
-					return $status->getErrorsArray();
-				}
-			}
-		}
-
 		# Log the move
 		$log = new LogPage( 'move' );
 		$log->addEntry( 'move_redir', $this, $reason, array( 1 => $nt->getPrefixedText(), 2 => $redirectSuppressed ) );
@@ -2970,18 +2969,6 @@ class Title {
 			$redirectSuppressed = true;
 		}
 
-		# Move an image if this is a file
-		if( $this->getNamespace() == NS_FILE ) {
-			$file = wfLocalFile( $this );
-			if( $file->exists() ) {
-				$status = $file->move( $nt );
-				if( !$status->isOk() ) {
-					$dbw->rollback();
-					return $status->getErrorsArray();
-				}
-			}
-		}
-
 		# Log the move
 		$log = new LogPage( 'move' );
 		$log->addEntry( 'move', $this, $reason, array( 1 => $nt->getPrefixedText(), 2 => $redirectSuppressed ) );
diff --git a/includes/filerepo/FSRepo.php b/includes/filerepo/FSRepo.php
index d561e61b88..5a868e20f2 100644
--- a/includes/filerepo/FSRepo.php
+++ b/includes/filerepo/FSRepo.php
@@ -212,6 +212,33 @@ class FSRepo extends FileRepo {
 		return $status;
 	}
 
+	/**
+	 * Checks existance of specified array of files.
+	 *
+	 * @param array $files URLs of files to check
+	 * @param integer $flags Bitwise combination of the following flags:
+	 *     self::FILES_ONLY     Mark file as existing only if it is a file (not directory)
+	 * @return Either array of files and existance flags, or false
+	 */
+	function fileExistsBatch( $files, $flags = 0 ) {
+		if ( !file_exists( $this->directory ) || !is_readable( $this->directory ) ) {
+			return false;
+		}
+		$result = array();
+		foreach ( $files as $key => $file ) {
+			if ( self::isVirtualUrl( $file ) ) {
+				$file = $this->resolveVirtualUrl( $file );
+			}
+			if( $flags & self::FILES_ONLY ) {
+				$result[$key] = is_file( $file );
+			} else {
+				$result[$key] = file_exists( $file );
+			}
+		}
+
+		return $result;
+	}
+
 	/**
 	 * Take all available measures to prevent web accessibility of new deleted
 	 * directories, in case the user has not configured offline storage
diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php
index face161487..22e41d1a33 100644
--- a/includes/filerepo/FileRepo.php
+++ b/includes/filerepo/FileRepo.php
@@ -6,6 +6,7 @@
  * @ingroup FileRepo
  */
 abstract class FileRepo {
+	const FILES_ONLY = 1;
 	const DELETE_SOURCE = 1;
 	const FIND_PRIVATE = 1;
 	const FIND_IGNORE_REDIRECT = 2;
diff --git a/includes/filerepo/LocalFile.php b/includes/filerepo/LocalFile.php
index b997d75f36..710a605cad 100644
--- a/includes/filerepo/LocalFile.php
+++ b/includes/filerepo/LocalFile.php
@@ -1789,6 +1789,11 @@ class LocalFileMoveBatch {
 		$status = $repo->newGood();
 		$triplets = $this->getMoveTriplets();
 
+		$statusPreCheck = $this->checkFileExistance( 0 );
+		if( !$statusPreCheck->isOk() ) {
+			wfDebugLog( 'imagemove', "Move of {$this->file->name} aborted due to pre-move file existance check failure" );
+			return $statusPreCheck;
+		}
 		$statusDb = $this->doDBUpdates();
 		wfDebugLog( 'imagemove', "Renamed {$this->file->name} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
 		$statusMove = $repo->storeBatch( $triplets, FSRepo::DELETE_SOURCE );
@@ -1796,7 +1801,14 @@ class LocalFileMoveBatch {
 		if( !$statusMove->isOk() ) {
 			wfDebugLog( 'imagemove', "Error in moving files: " . $statusMove->getWikiText() );
 			$this->db->rollback();
+		} else {
+			$statusPostCheck = $this->checkFileExistance( 1 );
+			if( !$statusPostCheck->isOk() ) {
+				// This clearly mustn't have happend. FSRepo::storeBatch should have given out an error in that case.
+				wfDebugLog( 'imagemove', "ATTENTION! Move of {$this->file->name} has some files missing, while storeBatch() reported success" );
+			}
 		}
+
 		$status->merge( $statusDb );
 		$status->merge( $statusMove );
 		return $status;
@@ -1856,4 +1868,23 @@ class LocalFileMoveBatch {
 		}
 		return $triplets;
 	}
+
+	/*
+	 * Checks file existance.
+	 * Set $key = 0 for source files check
+	 * and $key = 1 for destination files check.
+	 */ 
+	function checkFileExistance( $key = 0 ) {
+		$files = array();
+		foreach( array_merge( array( $this->cur ), $this->olds ) as $file )
+			$files[$file[$key]] = $this->file->repo->getVirtualUrl() . '/public/' . rawurlencode( $file[$key] );
+		$result = $this->file->repo->fileExistsBatch( $files, FSRepo::FILES_ONLY );
+		$status = $this->file->repo->newGood();
+		foreach( $result as $filename => $exists )
+			if( !$exists ) {
+				wfDebugLog( 'imagemove', "File {$filename} does not exist" );
+				$status->fatal( 'filenotfound', $filename );
+			}
+		return $status;
+	}
 }