Fix and make file moving better resilient against errors by first copying the files...

* FsRepo::cleanupBatch now accepts either pairs, virtual urls and real paths
* Add missing begin() and commit() to LocalFileMoveBatch
* Actually set an error after incrementing failCount because otherwise the status is still good
* Changed some isOk() to isGood() and set ->ok explicitly to false after rollback

diff --git a/includes/filerepo/FSRepo.php b/includes/filerepo/FSRepo.php
index d992986..b7b066e 100644 (file)
--- a/includes/filerepo/FSRepo.php
+++ b/includes/filerepo/FSRepo.php
@@ -262,16 +262,28 @@ class FSRepo extends FileRepo {
        }
        
        /**
-        * Deletes a batch of (zone, rel) pairs. It will try to delete each pair,
-        * but ignores any errors doing so.
+        * Deletes a batch of files. Each file can be a (zone, rel) pairs, a
+        * virtual url or a real path. It will try to delete each file, but 
+        * ignores any errors that may occur
         * 
-        * @param $pairs array Pair of (zone, rel) pairs to delete
+        * @param $pairs array List of files to delete
         */
-       function cleanupBatch( $pairs ) {
-               foreach ( $pairs as $pair ) {
-                       list( $zone, $rel ) = $pair;
-                       $root = $this->getZonePath( $zone );
-                       $path = "$root/$rel";
+       function cleanupBatch( $files ) {
+               foreach ( $files as $file ) {
+                       if ( is_array( $file ) ) {
+                               // This is a pair, extract it
+                               list( $zone, $rel ) = $file;
+                               $root = $this->getZonePath( $zone );
+                               $path = "$root/$rel";
+                       } else {
+                               if ( self::isVirtualUrl( $file ) ) {
+                                       // This is a virtual url, resolve it 
+                                       $path = $this->resolveVirtualUrl( $file );
+                               } else {
+                                       // This is a full file name
+                                       $path = $file;
+                               }
+                       }
                        
                        wfSuppressWarnings();
                        unlink( $path );

diff --git a/includes/filerepo/LocalFile.php b/includes/filerepo/LocalFile.php
index ebc0efe..6e57ca0 100644 (file)
--- a/includes/filerepo/LocalFile.php
+++ b/includes/filerepo/LocalFile.php
@@ -2043,16 +2043,32 @@ class LocalFileMoveBatch {
                $triplets = $this->getMoveTriplets();
 
                $triplets = $this->removeNonexistentFiles( $triplets );
-               $statusDb = $this->doDBUpdates();
-               wfDebugLog( 'imagemove', "Renamed {$this->file->name} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
-               $statusMove = $repo->storeBatch( $triplets, FSRepo::DELETE_SOURCE );
+               
+               // Copy the files into their new location
+               $statusMove = $repo->storeBatch( $triplets );
                wfDebugLog( 'imagemove', "Moved files for {$this->file->name}: {$statusMove->successCount} successes, {$statusMove->failCount} failures" );
-
-               if ( !$statusMove->isOk() ) {
+               if ( !$statusMove->isGood() ) {
                        wfDebugLog( 'imagemove', "Error in moving files: " . $statusMove->getWikiText() );
-                       $this->db->rollback();
+                       $this->cleanupTarget( $triplets );
+                       $statusMove->ok = false;
+                       return $statusMove;
                }
 
+               $this->db->begin();
+               $statusDb = $this->doDBUpdates();
+               wfDebugLog( 'imagemove', "Renamed {$this->file->name} in database: {$statusDb->successCount} successes, {$statusDb->failCount} failures" );
+               if ( !$statusDb->isGood() ) {
+                       $this->db->rollback();
+                       // Something went wrong with the DB updates, so remove the target files
+                       $this->cleanupTarget( $triplets );
+                       $statusDb->ok = false;
+                       return $statusDb;
+               }
+               $this->db->commit();
+               
+               // Everything went ok, remove the source files
+               $this->cleanupSource( $triplets );
+               
                $status->merge( $statusDb );
                $status->merge( $statusMove );
 
@@ -2082,6 +2098,8 @@ class LocalFileMoveBatch {
                        $status->successCount++;
                } else {
                        $status->failCount++;
+                       $status->fatal( 'imageinvalidfilename' );
+                       return $status;
                }
 
                // Update old images
@@ -2099,6 +2117,9 @@ class LocalFileMoveBatch {
                $total = $this->oldCount;
                $status->successCount += $affected;
                $status->failCount += $total - $affected;
+               if ( $status->failCount ) {
+                       $status->error( 'imageinvalidfilename' );
+               }
 
                return $status;
        }
@@ -2143,4 +2164,32 @@ class LocalFileMoveBatch {
 
                return $filteredTriplets;
        }
+       
+       /**
+        * Cleanup a partially moved array of triplets by deleting the target 
+        * files. Called if something went wrong half way.
+        */
+       function cleanupTarget( $triplets ) {
+               // Create dest pairs from the triplets
+               $pairs = array();
+               foreach ( $triplets as $triplet ) {
+                       $pairs[] = array( $triplet[1], $triplet[2] );
+               }
+               
+               $this->file->repo->cleanupBatch( $pairs );
+       }
+       
+       /**
+        * Cleanup a fully moved array of triplets by deleting the source files.
+        * Called at the end of the move process if everything else went ok. 
+        */
+       function cleanupSource( $triplets ) {
+               // Create source file names from the triplets
+               $files = array();
+               foreach ( $triplets as $triplet ) {
+                       $files[] = $triplet[0];
+               }
+               
+               $this->file->repo->cleanupBatch( $files );
+       }
 }