Temp files should be deleted when an exception is thrown (#327)

If an exception is thrown while writing data, instead of letting the developer handle this situation gracefully, Spout can attempt to delete all the temporary files that were created so far, as well as the output file as it won't be completed and therefore corrupted.

src/Spout/Common/Helper/GlobalFunctionsHelper.php

@@ -203,6 +203,18 @@ class GlobalFunctionsHelper
         return (strpos($path, 'zip://') === 0);
     }
 
+    /**
+     * Wrapper around global function dirname()
+     * @see dirname()
+     *
+     * @param string $filePath
+     * @return string
+     */
+    public function dirname($filePath)
+    {
+        return dirname($filePath);
+    }
+
     /**
      * Wrapper around global function feof()
      * @see feof()

src/Spout/Writer/AbstractWriter.php

@@ -4,6 +4,8 @@ namespace Box\Spout\Writer;
 
 use Box\Spout\Common\Exception\IOException;
 use Box\Spout\Common\Exception\InvalidArgumentException;
+use Box\Spout\Common\Exception\SpoutException;
+use Box\Spout\Common\Helper\FileSystemHelper;
 use Box\Spout\Writer\Exception\WriterAlreadyOpenedException;
 use Box\Spout\Writer\Exception\WriterNotOpenedException;
 use Box\Spout\Writer\Style\StyleBuilder;
@@ -199,13 +201,23 @@ abstract class AbstractWriter implements WriterInterface
      * @return AbstractWriter
      * @throws \Box\Spout\Writer\Exception\WriterNotOpenedException If this function is called before opening the writer
      * @throws \Box\Spout\Common\Exception\IOException If unable to write data
+     * @throws \Box\Spout\Common\Exception\SpoutException If anything else goes wrong while writing data
      */
     public function addRow(array $dataRow)
     {
         if ($this->isWriterOpened) {
             // empty $dataRow should not add an empty line
             if (!empty($dataRow)) {
-                $this->addRowToWriter($dataRow, $this->rowStyle);
+                try {
+                    $this->addRowToWriter($dataRow, $this->rowStyle);
+                } catch (SpoutException $e) {
+                    // if an exception occurs while writing data,
+                    // close the writer and remove all files created so far.
+                    $this->closeAndAttemptToCleanupAllFiles();
+
+                    // re-throw the exception to alert developers of the error
+                    throw $e;
+                }
             }
         } else {
             throw new WriterNotOpenedException('The writer needs to be opened before adding row.');
@@ -346,5 +358,24 @@ abstract class AbstractWriter implements WriterInterface
 
         $this->isWriterOpened = false;
     }
+
+    /**
+     * Closes the writer and attempts to cleanup all files that were
+     * created during the writing process (temp files & final file).
+     *
+     * @return void
+     */
+    private function closeAndAttemptToCleanupAllFiles()
+    {
+        // close the writer, which should remove all temp files
+        $this->close();
+
+        // remove output file if it was created
+        if ($this->globalFunctionsHelper->file_exists($this->outputFilePath)) {
+            $outputFolder = $this->globalFunctionsHelper->dirname($this->outputFilePath);
+            $fileSystemHelper = new FileSystemHelper($outputFolder);
+            $fileSystemHelper->deleteFile($this->outputFilePath);
+        }
+    }
 }
 

tests/Spout/TestUsingResource.php

@@ -18,6 +18,9 @@ trait TestUsingResource
     /** @var string Path to the test resources folder, that does not have writing permissions */
     private $generatedUnwritableResourcesPath = 'tests/resources/generated/unwritable';
 
+    /** @var string Path to the test temp folder */
+    private $tempFolderPath = 'tests/resources/generated/temp';
+
     /**
      * @param string $resourceName
      * @return string|null Path of the resource who matches the given name or null if resource not found
@@ -86,6 +89,48 @@ trait TestUsingResource
         }
     }
 
+    /**
+     * @return string Path of the temp folder
+     */
+    protected function getTempFolderPath()
+    {
+        return realpath($this->tempFolderPath);
+    }
+
+    /**
+     * @return void
+     */
+    protected function recreateTempFolder()
+    {
+        if (file_exists($this->tempFolderPath)) {
+            $this->deleteFolderRecursively($this->tempFolderPath);
+        }
+
+        mkdir($this->tempFolderPath, 0777, true);
+    }
+
+    /**
+     * @param string $folderPath
+     * @return void
+     */
+    private function deleteFolderRecursively($folderPath)
+    {
+        $itemIterator = new \RecursiveIteratorIterator(
+            new \RecursiveDirectoryIterator($folderPath, \RecursiveDirectoryIterator::SKIP_DOTS),
+            \RecursiveIteratorIterator::CHILD_FIRST
+        );
+
+        foreach ($itemIterator as $item) {
+            if ($item->isDir()) {
+                rmdir($item->getPathname());
+            } else {
+                unlink($item->getPathname());
+            }
+        }
+
+        rmdir($folderPath);
+    }
+
     /**
      * @return bool Whether the OS on which PHP is installed is Windows
      */

tests/Spout/Writer/ODS/WriterTest.php

@@ -2,6 +2,7 @@
 
 namespace Box\Spout\Writer\ODS;
 
+use Box\Spout\Common\Exception\SpoutException;
 use Box\Spout\Common\Type;
 use Box\Spout\Reader\Wrapper\XMLReader;
 use Box\Spout\TestUsingResource;
@@ -91,6 +92,39 @@ class WriterTest extends \PHPUnit_Framework_TestCase
         $this->writeToODSFile($dataRows, $fileName);
     }
 
+    /**
+     * @return void
+     */
+    public function testAddRowShouldCleanupAllFilesIfExceptionIsThrown()
+    {
+        $fileName = 'test_add_row_should_cleanup_all_files_if_exception_thrown.ods';
+        $dataRows = [
+            ['wrong'],
+            [new \stdClass()],
+        ];
+
+        $this->createGeneratedFolderIfNeeded($fileName);
+        $resourcePath = $this->getGeneratedResourcePath($fileName);
+
+        $this->recreateTempFolder();
+        $tempFolderPath = $this->getTempFolderPath();
+
+        /** @var \Box\Spout\Writer\ODS\Writer $writer */
+        $writer = WriterFactory::create(Type::ODS);
+        $writer->setTempFolder($tempFolderPath);
+        $writer->openToFile($resourcePath);
+
+        try {
+            $writer->addRows($dataRows);
+            $this->fail('Exception should have been thrown');
+        } catch (SpoutException $e) {
+            $this->assertFalse(file_exists($fileName), 'Output file should have been deleted');
+
+            $numFiles = iterator_count(new \FilesystemIterator($tempFolderPath, \FilesystemIterator::SKIP_DOTS));
+            $this->assertEquals(0, $numFiles, 'All temp files should have been deleted');
+        }
+    }
+
     /**
      * @return void
      */

tests/Spout/Writer/XLSX/WriterTest.php

@@ -2,6 +2,7 @@
 
 namespace Box\Spout\Writer\XLSX;
 
+use Box\Spout\Common\Exception\SpoutException;
 use Box\Spout\Common\Type;
 use Box\Spout\TestUsingResource;
 use Box\Spout\Writer\WriterFactory;
@@ -104,6 +105,39 @@ class WriterTest extends \PHPUnit_Framework_TestCase
         $this->writeToXLSXFile($dataRows, $fileName);
     }
 
+    /**
+     * @return void
+     */
+    public function testAddRowShouldCleanupAllFilesIfExceptionIsThrown2()
+    {
+        $fileName = 'test_add_row_should_cleanup_all_files_if_exception_thrown.xlsx';
+        $dataRows = [
+            ['wrong'],
+            [new \stdClass()],
+        ];
+
+        $this->createGeneratedFolderIfNeeded($fileName);
+        $resourcePath = $this->getGeneratedResourcePath($fileName);
+
+        $this->recreateTempFolder();
+        $tempFolderPath = $this->getTempFolderPath();
+
+        /** @var \Box\Spout\Writer\XLSX\Writer $writer */
+        $writer = WriterFactory::create(Type::XLSX);
+        $writer->setTempFolder($tempFolderPath);
+        $writer->openToFile($resourcePath);
+
+        try {
+            $writer->addRows($dataRows);
+            $this->fail('Exception should have been thrown');
+        } catch (SpoutException $e) {
+            $this->assertFalse(file_exists($fileName), 'Output file should have been deleted');
+
+            $numFiles = iterator_count(new \FilesystemIterator($tempFolderPath, \FilesystemIterator::SKIP_DOTS));
+            $this->assertEquals(0, $numFiles, 'All temp files should have been deleted');
+        }
+    }
+
     /**
      * @return void
      */