MDL-51863 packer: ensure empty zip files behavior remains consistent
authorEloy Lafuente (stronk7) <stronk7@moodle.org>
Mon, 26 Oct 2015 16:59:40 +0000 (17:59 +0100)
committerEloy Lafuente (stronk7) <stronk7@moodle.org>
Mon, 26 Oct 2015 20:16:54 +0000 (21:16 +0100)
With PHP bug #70322 fixed, ZipArchive::close() did start returning false
and throwing PHP Warnings with recent PHP versions (5.6.14 and up).
Previously (5.6.13 verified) it was returning true, and false in older
versions (5.4.x verified).

This change does silent the 2 "hacky" calls to close() that we perform
in core leaving the 3rd one (used for files having files) unmodified.

A new unit test has been created to cover the close() behavior, ideally
supporting both old and new PHP versions without harcoding any PHP
version.

Note that we don't use to rely much on results coming from close(), and
that's a good thing given the buggy behavior commented above. This just
keeps empty zips working like they were before.

lib/filestorage/tests/zip_packer_test.php
lib/filestorage/zip_archive.php

index b39bcdf..8da89dd 100644 (file)
@@ -376,6 +376,73 @@ class core_files_zip_packer_testcase extends advanced_testcase implements file_p
         unlink($archive);
     }
 
+    public function test_close_archive() {
+        global $CFG;
+
+        $this->resetAfterTest(true);
+
+        $archive = "$CFG->tempdir/archive.zip";
+        $textfile = "$CFG->tempdir/textfile.txt";
+        touch($textfile);
+
+        $this->assertFileNotExists($archive);
+        $this->assertFileExists($textfile);
+
+        // Create archive and close it without files.
+        // (returns true, without any warning).
+        $zip_archive = new zip_archive();
+        $result = $zip_archive->open($archive, file_archive::CREATE);
+        $this->assertTrue($result);
+        $result = $zip_archive->close();
+        $this->assertTrue($result);
+        unlink($archive);
+
+        // Create archive and close it with files.
+        // (returns true, without any warning).
+        $zip_archive = new zip_archive();
+        $result = $zip_archive->open($archive, file_archive::CREATE);
+        $this->assertTrue($result);
+        $result = $zip_archive->add_file_from_string('test.txt', 'test');
+        $this->assertTrue($result);
+        $result = $zip_archive->add_file_from_pathname('test2.txt', $textfile);
+        $result = $zip_archive->close();
+        $this->assertTrue($result);
+        unlink($archive);
+
+        // Create archive and close if forcing error.
+        // (returns true for old PHP versions and
+        // false with warnings for new PHP versions). MDL-51863.
+        $zip_archive = new zip_archive();
+        $result = $zip_archive->open($archive, file_archive::CREATE);
+        $this->assertTrue($result);
+        $result = $zip_archive->add_file_from_string('test.txt', 'test');
+        $this->assertTrue($result);
+        $result = $zip_archive->add_file_from_pathname('test2.txt', $textfile);
+        $this->assertTrue($result);
+        // Delete the file before closing does force close() to fail.
+        unlink($textfile);
+        // Behavior is different between old PHP versions and new ones. Let's detect it.
+        $result = false;
+        try {
+            // Old PHP versions were not printing any warning.
+            $result = $zip_archive->close();
+        } catch (Exception $e) {
+            // New PHP versions print PHP Warning.
+            $this->assertInstanceOf('PHPUnit_Framework_Error_Warning', $e);
+            $this->assertContains('ZipArchive::close', $e->getMessage());
+        }
+        // This is crazy, but it shows how some PHP versions do return true.
+        try {
+            // And some PHP versions do return correctly false (5.4.25, 5.6.14...)
+            $this->assertFalse($result);
+        } catch (Exception $e) {
+            // But others do insist into returning true (5.6.13...). Only can accept them.
+            $this->assertInstanceOf('PHPUnit_Framework_ExpectationFailedException', $e);
+            $this->assertTrue($result);
+        }
+        $this->assertFileNotExists($archive);
+    }
+
     /**
      * @depends test_add_files
      */
index f51facd..70b9d33 100644 (file)
@@ -191,7 +191,7 @@ class zip_archive extends file_archive {
         }
 
         if ($this->emptyziphack) {
-            $this->za->close();
+            @$this->za->close();
             $this->za = null;
             $this->mode = null;
             $this->namelookup = null;
@@ -202,7 +202,7 @@ class zip_archive extends file_archive {
 
         } else if ($this->za->numFiles == 0) {
             // PHP can not create empty archives, so let's fake it.
-            $this->za->close();
+            @$this->za->close();
             $this->za = null;
             $this->mode = null;
             $this->namelookup = null;