fix(task) #17 ZipTask and UnzipTask no longer ignore write failures

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
 ------

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.

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`.

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);

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']);
     }

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<string, mixed> $options
      *

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<string, mixed> $options
      *