From b684acb14b1092affd6d99beef30683c8e9f1799 Mon Sep 17 00:00:00 2001 From: Adrien Loison Date: Tue, 11 Oct 2016 14:55:41 -0700 Subject: [PATCH] Temp files should be deleted when an exception is thrown 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. --- .../Common/Helper/GlobalFunctionsHelper.php | 12 +++++ src/Spout/Writer/AbstractWriter.php | 33 +++++++++++++- tests/Spout/TestUsingResource.php | 45 +++++++++++++++++++ tests/Spout/Writer/ODS/WriterTest.php | 34 ++++++++++++++ tests/Spout/Writer/XLSX/WriterTest.php | 34 ++++++++++++++ 5 files changed, 157 insertions(+), 1 deletion(-) diff --git a/src/Spout/Common/Helper/GlobalFunctionsHelper.php b/src/Spout/Common/Helper/GlobalFunctionsHelper.php index c5d6e31..7eb66b4 100644 --- a/src/Spout/Common/Helper/GlobalFunctionsHelper.php +++ b/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() diff --git a/src/Spout/Writer/AbstractWriter.php b/src/Spout/Writer/AbstractWriter.php index 49939ee..af97ead 100644 --- a/src/Spout/Writer/AbstractWriter.php +++ b/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); + } + } } diff --git a/tests/Spout/TestUsingResource.php b/tests/Spout/TestUsingResource.php index 17c37b5..2b200d9 100644 --- a/tests/Spout/TestUsingResource.php +++ b/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 */ diff --git a/tests/Spout/Writer/ODS/WriterTest.php b/tests/Spout/Writer/ODS/WriterTest.php index 43383e4..7c4b396 100644 --- a/tests/Spout/Writer/ODS/WriterTest.php +++ b/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 */ diff --git a/tests/Spout/Writer/XLSX/WriterTest.php b/tests/Spout/Writer/XLSX/WriterTest.php index e74cd9c..cc8daca 100644 --- a/tests/Spout/Writer/XLSX/WriterTest.php +++ b/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 */