From 70ae75674dcbe028d79d0622bfddb94c73abdb6b Mon Sep 17 00:00:00 2001
From: "Eloy Lafuente (stronk7)" <stronk7@moodle.org>
Date: Mon, 26 Oct 2015 17:59:40 +0100
Subject: [PATCH] MDL-51863 packer: ensure empty zip files behavior remains
 consistent

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 | 67 +++++++++++++++++++++++
 lib/filestorage/zip_archive.php           |  4 +-
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/lib/filestorage/tests/zip_packer_test.php b/lib/filestorage/tests/zip_packer_test.php
index b39bcdfa3e5..8da89dd3d80 100644
--- a/lib/filestorage/tests/zip_packer_test.php
+++ b/lib/filestorage/tests/zip_packer_test.php
@@ -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
      */
diff --git a/lib/filestorage/zip_archive.php b/lib/filestorage/zip_archive.php
index f51facd2b3c..70b9d336f5d 100644
--- a/lib/filestorage/zip_archive.php
+++ b/lib/filestorage/zip_archive.php
@@ -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;