diff --git a/CHANGELOG.md b/CHANGELOG.md index ae2230d..aa62bc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ Latest ### Fixes * [#15](https://github.com/cleverage/archive-process-bundle/issues/15) Fix ZipTask and UnzipTask: resolve the options on every execution, so that each input is used (the options of the first input were reused for the following ones). Update documentation, add tests. * [#16](https://github.com/cleverage/archive-process-bundle/issues/16) Fix ZipTask: only remove the leading `files_base_path` from the file paths (every occurrence was removed), and read the files at the given path when `files_base_path` is empty (relative paths were resolved from the filesystem root). Update documentation, add tests. +* [#17](https://github.com/cleverage/archive-process-bundle/issues/17) Fix ZipTask and UnzipTask: throw a `\RuntimeException` when the archive cannot be written (ZipTask) or extracted (UnzipTask), instead of outputting the path anyway; add the `ZipArchive` error code to the UnzipTask open failure message. Update documentation, add tests. v2.1 ------ diff --git a/docs/reference/tasks/unzip_task.md b/docs/reference/tasks/unzip_task.md index 213e1a2..af4e46f 100644 --- a/docs/reference/tasks/unzip_task.md +++ b/docs/reference/tasks/unzip_task.md @@ -77,7 +77,9 @@ Notes * Underlying method is [ZipArchive::extractTo()](https://www.php.net/manual/en/ziparchive.extractto.php): the whole archive is extracted, existing files with the same name are overwritten and other files already present in the destination directory are kept. -* A `\RuntimeException` is thrown if the file cannot be opened as a zip archive. +* A `\RuntimeException` is thrown if the file cannot be opened as a zip archive (with the `ZipArchive` error code, + e.g. `19` for `ZipArchive::ER_NOZIP`), or cannot be extracted (e.g. `destination` is not writable or is a file). In + the latter case, the files extracted before the failure are kept. * Options are resolved on each execution of the task: when the task receives several inputs (e.g. after an iterable task), each input is extracted with its own `filename` and `destination`. * See the [Import the CSV files of an uploaded archive](../../cookbooks/import_archive.md) cookbook. diff --git a/docs/reference/tasks/zip_task.md b/docs/reference/tasks/zip_task.md index 6171a05..018f52e 100644 --- a/docs/reference/tasks/zip_task.md +++ b/docs/reference/tasks/zip_task.md @@ -86,7 +86,9 @@ Notes ----- * Underlying class is [ZipArchive](https://www.php.net/manual/en/class.ziparchive.php), the archive is opened with the - `ZipArchive::CREATE | ZipArchive::OVERWRITE` flags. A `\RuntimeException` is thrown if it cannot be opened. + `ZipArchive::CREATE | ZipArchive::OVERWRITE` flags. A `\RuntimeException` is thrown if it cannot be opened (with the + `ZipArchive` error code), or written once all files are added (e.g. when the parent directory of `filename` does not + exist, the directory is not created). * For each file, the entry name is the file path with the leading `files_base_path` removed (only when the path starts with it, followed by a directory separator) and leading directory separators trimmed; the file actually read is `files_base_path` + directory separator + entry name. Hence all files must be located under `files_base_path`. diff --git a/src/Task/UnzipTask.php b/src/Task/UnzipTask.php index 4a48ca3..ea87e72 100644 --- a/src/Task/UnzipTask.php +++ b/src/Task/UnzipTask.php @@ -45,11 +45,17 @@ public function execute(ProcessState $state): void $zipArchive = new \ZipArchive(); $res = $zipArchive->open($filename); - if (true === $res) { - $zipArchive->extractTo($dest); - $zipArchive->close(); - } else { - throw new \RuntimeException("Unable to open file {$filename}"); + if (true !== $res) { + throw new \RuntimeException("Unable to open file {$filename} with code {$res}"); + } + + // The PHP warning is replaced by the exception below + error_clear_last(); + $extracted = @$zipArchive->extractTo($dest); + $error = error_get_last()['message'] ?? $zipArchive->getStatusString(); + $zipArchive->close(); + if (!$extracted) { + throw new \RuntimeException("Unable to extract file {$filename} to {$dest}: {$error}"); } $state->setOutput($dest); diff --git a/src/Task/ZipTask.php b/src/Task/ZipTask.php index 1971498..69e41b2 100644 --- a/src/Task/ZipTask.php +++ b/src/Task/ZipTask.php @@ -67,7 +67,10 @@ public function execute(ProcessState $state): void } } - $zip->close(); + // The archive is written on close: the PHP warning is replaced by the exception below + if (!@$zip->close()) { + throw new \RuntimeException("Unable to write zip file {$options['filename']}: {$zip->getStatusString()}"); + } $state->setOutput($options['filename']); } diff --git a/tests/Task/UnzipTaskTest.php b/tests/Task/UnzipTaskTest.php index 590ddea..4aa49df 100644 --- a/tests/Task/UnzipTaskTest.php +++ b/tests/Task/UnzipTaskTest.php @@ -66,6 +66,39 @@ public function testEachInputIsUsed(): void self::assertFileDoesNotExist($this->dir.'/destination1/file2.txt'); } + public function testExtractFailure(): void + { + file_put_contents($this->dir.'/file.txt', 'not a directory'); + [$task, $state] = $this->createTask([ + 'filename' => $this->dir.'/archive1.zip', + 'destination' => $this->dir.'/file.txt', + ]); + + try { + $this->execute($task, $state, null); + self::fail('A \RuntimeException should have been thrown'); + } catch (\RuntimeException $e) { + self::assertStringStartsWith("Unable to extract file {$this->dir}/archive1.zip to {$this->dir}/file.txt: ZipArchive::extractTo(", $e->getMessage()); + } + self::assertNull($state->getOutput()); + } + + public function testOpenFailureGivesTheErrorCode(): void + { + file_put_contents($this->dir.'/file.txt', 'not a zip archive'); + [$task, $state] = $this->createTask([ + 'filename' => $this->dir.'/file.txt', + 'destination' => $this->dir.'/destination', + ]); + + try { + $this->execute($task, $state, null); + self::fail('A \RuntimeException should have been thrown'); + } catch (\RuntimeException $e) { + self::assertSame("Unable to open file {$this->dir}/file.txt with code ".\ZipArchive::ER_NOZIP, $e->getMessage()); + } + } + /** * @param array $options * diff --git a/tests/Task/ZipTaskTest.php b/tests/Task/ZipTaskTest.php index 519d84c..40ea9df 100644 --- a/tests/Task/ZipTaskTest.php +++ b/tests/Task/ZipTaskTest.php @@ -99,6 +99,23 @@ public function testRelativePathWithoutBasePath(): void self::assertSame(['file1.txt', ltrim($this->dir, '/').'/file2.txt'], $this->getEntries($this->dir.'/archive.zip')); } + public function testWriteFailure(): void + { + [$task, $state] = $this->createTask([ + 'filename' => $this->dir.'/missing_directory/archive.zip', + 'files' => 'file1.txt', + 'files_base_path' => $this->dir, + ]); + + try { + $this->execute($task, $state, null); + self::fail('A \RuntimeException should have been thrown'); + } catch (\RuntimeException $e) { + self::assertStringStartsWith("Unable to write zip file {$this->dir}/missing_directory/archive.zip: Failure to create temporary file", $e->getMessage()); + } + self::assertNull($state->getOutput()); + } + /** * @param array $options *