Unable to open Zip archive using ArchiverZip

Rerolled the patch, please review.

Confirmed that the documentation states the file will be created as needed, which does not happen without this patch. (See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Archiver%...)

Tested that this patch fixes the issue with:

$zip_path = file_create_filename($zip_name . '.zip', 'temporary://');
$zip = \Drupal::service('plugin.manager.archiver')->getInstance(['filepath' => $zip_path]);

Before patch, ArchiverException it thrown.
After patch, $zip is set to the Drupal/Core/Archiver/Zip object as expected.

This could use some test coverage.

Added test coverage.

This testcoverage looks to be sufficient, that looks really good. It does bring some new coding standards violations though.

1. +++ b/core/tests/Drupal/KernelTests/Core/Archiver/ZipTest.php
   @@ -0,0 +1,48 @@
   +/**
   + * @file
   + * Contains \Drupal\Tests\Core\Archiver\ZipTest.
   + */
   

   Should be removed.

2. +++ b/core/tests/Drupal/KernelTests/Core/Archiver/ZipTest.php
   @@ -0,0 +1,48 @@
   +  }
   

   There should be a newline following the method.

3. +++ b/core/tests/Drupal/KernelTests/Core/Archiver/ZipTest.php
   @@ -0,0 +1,48 @@
   +}
   \ No newline at end of file
   

   There should be a newline at the end of the file.

Fixed my own nitpicks.

All I did was fix nitpicks, I feel it's ok for me to set this to RTBC?

1) Drupal\Tests\ComposerIntegrationTest::testComposerLockHash
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-0a3f405c686e98658196e44ca56ed3b0
+9d37d607bfdb3f5127d18051be44fca0

That was the failure in #12 I retested this because that doesn't seem to be related.

Back to rtbc, because the test is green again.

+++ b/core/tests/Drupal/KernelTests/Core/Archiver/ZipTest.php
@@ -0,0 +1,44 @@
+class ZipTest extends KernelTestBase {
...
+    // ZipArchive::open() does not support the temporary stream wrapper, so we
+    // have to get the path to the temporary folder.
+    $filename = file_directory_temp() . '/' . $this->randomMachineName() . '.zip';
...
+      file_unmanaged_save_data('llama', file_directory_temp() . '/llama.txt', FILE_EXISTS_REPLACE);
+      $zip->add(file_directory_temp() . '/llama.txt');

Let's use BrowserTestBase here. Using the file_directory_temp() in a test like this is super risky and has lead to hard to spot random fails in the past. That way we get a real directory. Yes it is a shame we have to install Drupal but the predictability and clean up is important.

Fixed #17.

That test was green locally, I have no idea why it's not on the testbot.

1. +++ b/core/lib/Drupal/Core/Archiver/Zip.php
   @@ -23,12 +23,15 @@ class Zip implements ArchiverInterface {
   -  public function __construct($file_path) {
   +  public function __construct($file_path, $flags = \ZipArchive::CREATE) {
        $this->zip = new \ZipArchive();
   -    if ($this->zip->open($file_path) !== TRUE) {
   +    if ($this->zip->open($file_path, $flags) !== TRUE) {
   

   Imo we shouldn't change the default from \ZipArchive::open()... i.e our default should be NULL too. But we should allow passing in flags.

   Ah I see. So this change makes us comply with the docs and also probably behave the same as \Drupal\Core\Archiver\Tar

   That seems like a good idea.

2. I guess we need to work out why this fails on DrupalCI.

Rerolled patch against 9.1.x. The exception message needed to be updated.

Hey, I've been using this patch for a good three years now, it's just that failing test that's blocking this from being committed to master eh?

As this is now targeted for Drupal 9, the test will have to use its file management methods.

• Make it a Kernel test. There isn't a justification for it to be a BrowserTest. However, I just read #17 above so I'm not sure about my position on it.
• Move the test to the correct namespace.
• Use D9 file functions in the test.
• Reorder variable declarations in the test to be closer to their usages.
• Remove what I feel is an unnecessary try/catch in the test.

Here it is, again as a BrowserTest.

Fix the coding standards.

The 8.9.x failures are unrelated.

+++ b/core/tests/Drupal/FunctionalTests/Core/Archiver/ZipTest.php
@@ -0,0 +1,39 @@
+/**
+ * @coversDefaultClass \Drupal\Core\Archiver\Zip
+ * @group zip
+ */
+class ZipTest extends BrowserTestBase {

Looking back at my comments over 2 years ago - I realise we can use KernelTestBase we just should not use the temporary directory - we can use the sites directory - the test system will clear this up for us.

+++ b/core/tests/Drupal/FunctionalTests/Core/Archiver/ZipTest.php
@@ -0,0 +1,39 @@
+    // Create a temporary file that can be added to the archive.
+    $filename = \Drupal::service('file_system')->getTempDirectory() . '/' . $this->randomMachineName();
+    \Drupal::service('file_system')->saveData('llama', $filename, FileSystemInterface::EXISTS_REPLACE);

Can use TestFileCreationTrait (even in a Kernel test) to generate a test file.

I have updated the patch as per @alexpott comments.
Please review.

1. +++ b/core/tests/Drupal/KernelTests/Core/Archiver/ZipTest.php
   @@ -0,0 +1,66 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  public static $modules = ['system'];
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $defaultTheme = 'stark';
   

   These are not required for this test.

2. +++ b/core/tests/Drupal/KernelTests/Core/Archiver/ZipTest.php
   @@ -0,0 +1,66 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUpFilesystem() {
   +    $public_file_directory = $this->siteDirectory . '/files';
   +
   +    require_once 'core/includes/file.inc';
   +
   +    mkdir($this->siteDirectory, 0775);
   +    mkdir($this->siteDirectory . '/files', 0775);
   +
   +    $this->setSetting('file_public_path', $public_file_directory);
   +  }
   

   This is not necessary if you change the test to:

     public function testCreateArchive() {
       /** @var \Drupal\Core\File\FileSystemInterface $file_system */
       $file_system = \Drupal::service('file_system');
       $textfile = current($this->getTestFiles('text'));
       $archiveFilename = 'public://' . $this->randomMachineName() . '.zip';
       $zip = new Zip($file_system->realPath($archiveFilename));
       $zip->add($file_system->realPath($textfile->uri));
       // Close the archive and make sure it is written to disk.
       $this->assertTrue($zip->getArchive()->close(), 'Successfully closed archive.');
       $this->assertFileExists($archiveFilename, 'Archive is automatically created if the file does not exist.');
     }
   

   Add extend from Drupal\KernelTests\Core\File\FileTestBase instead of KernelTestBase.

   We need to file a follow-up issue about Drupal\KernelTests\Core\File\FileTestBase not cleaning up after itself.

@alexpott, Thank you for reviewing the patch.
I have updated the patch please.

Fixed PHPLint Errors in #36.

+++ b/core/lib/Drupal/Core/Archiver/Zip.php
@@ -23,12 +23,15 @@ class Zip implements ArchiverInterface {
+   * @param int $flags
+   *   (optional) The mode to use to open the archive. Defaults to create the
+   *   archive if it does not exist.
...
+  public function __construct($file_path, $flags = \ZipArchive::CREATE) {

Adding a new paramater to the constructor suggests this is something you can overwrite. However, since the plugin manager \Drupal\Core\Archiver\ArchiverManager::createInstance() only sends the $file_path this is not something you can change when creating a new plugin instance.

I suggest either:

1. Pass a the full configuration object to the constructor, so the zip plugin can be instantiated with a different flag
2. OR: Check the error code when calling $this->zip->open() https://www.php.net/manual/en/ziparchive.open.php and create a new zip on ZipArchive::ER_NOENT so the code matches what it says on the tin.

There's 2 different bugs

1) no api to create new archive, which could use to add createlist(array $files) as feature parity to \Archive_Tar, it looks better then exposing default value when extension could be missing

2) ability to exclude zip plugin if no zip extension installed in PHP or fallback somehow, better keeps in #3163123: Error: Class 'ZipArchive' not found in Drupal\Core\Archiver\Zip->__construct() (line 30 of core/lib/Drupal/Core/Archiver/Zip.php).

So the issue title could be fixed like "ArchiverZip has no method to create new archive"

There's different approach, so test could be adopted from #37

Just always pass configuration to plugin

It seems the test coverage in #37 was lost in #40.

@idebr test from #37 is useless for this change, it needs extra test coverage for both plugins

My intent is start discussion of plugin config way to solve the issue
The changes in plugins is unnecessary and just shows example of benefits of this approach

I've been having to use touch($file_path); prior to running new Zip($file_path); because of not being able to pass flags. PHP 8.0 has deprecated opening an empty zip file:

Deprecated function: ZipArchive::open(): Using empty file as ZipArchive is deprecated

Continuing on #40's usage of having configuration via the plugin be each archiver constructor's second parameter, I've adjusted the $buffer in Tar to default to 512 to match the default value and to prevent a fread(): Length parameter must be greater than 0 error being thrown.

Left the fallback for $configuration['flags'] as 0 in Zip. PHP Documentation has it at zero although my intellisense has it as NULL. ZipArchive's constants appear to start at one so the chances of ZipArchive having a zero constant added in the future seems slim to none.

Continuing on #37's test coverage, I've added an overwrite test for the Zip archiver and a basic creation test for the Tar archiver, each extending a new ArchiverTestBase (that extends the previous FileTestBase). The tests themselves use direct Tar/Zip class constructions; however, the assertions go through the plugin - in an attempt to cover both ends.

@collinhaines great job! please fix documentation nitpicks, ++ rtbc

1. +++ b/core/lib/Drupal/Core/Archiver/Tar.php
   @@ -21,11 +21,15 @@ class Tar implements ArchiverInterface {
   +   * @param array $configuration
   +   *   (Optional) settings to open archive.
   ...
   +  public function __construct($file_path, array $configuration = []) {
   +    $compress = $configuration['compress'] ?? NULL;
   +    $buffer = $configuration['buffer_length'] ?? 512;
   

   would be great to elaborate available for tar

2. +++ b/core/lib/Drupal/Core/Archiver/Zip.php
   @@ -23,12 +23,14 @@ class Zip implements ArchiverInterface {
   +   * @param array $configuration
   +   *   (Optional) settings to open archive.
   ...
   -  public function __construct($file_path) {
   +  public function __construct($file_path, array $configuration = []) {
   ...
   +    if ($this->zip->open($file_path, $configuration['flags'] ?? 0) !== TRUE) {
   

   at least "flags" should be documented

Added in documentation for the compress, buffer_length, and flags keys.

Thank you @collinhaines - fixing cspell

RTBC per #49 doc issues were addressed

The current solution looks really nice. Adding more configuration options to the plugins is a great idea. But we need a change record to tell devs about this.

Added CR https://www.drupal.org/node/3322990

Related issue #3163123: Error: Class 'ZipArchive' not found in Drupal\Core\Archiver\Zip->__construct() (line 30 of core/lib/Drupal/Core/Archiver/Zip.php). landed but it still a bug

Now that we have the CR this can go back to RTBC.

Committed 70700fc and pushed to 10.1.x. Thanks!