MDL-37429 zipping improvements
authorPetr Škoda <commits@skodak.org>
Wed, 9 Jan 2013 14:54:11 +0000 (15:54 +0100)
committerPetr Škoda <commits@skodak.org>
Sat, 12 Jan 2013 13:58:26 +0000 (14:58 +0100)
Changes:
* zip_packer can create empty zip archives
* new option to ignore problematic files when creating archive
* detection of non-existent files
* debugging messages for opening of faulty zip archives
* coding style improvements
* no PHP 5.2 hacks
* more unit tests

lib/filestorage/file_archive.php
lib/filestorage/file_exceptions.php
lib/filestorage/file_packer.php
lib/filestorage/tests/fixtures/empty.zip [new file with mode: 0644]
lib/filestorage/tests/fixtures/zip_info.php
lib/filestorage/tests/zip_packer_test.php
lib/filestorage/zip_archive.php
lib/filestorage/zip_packer.php

index d27d2c6..1aa688d 100644 (file)
@@ -47,7 +47,7 @@ abstract class file_archive implements Iterator {
     protected $encoding = 'utf-8';
 
     /**
     protected $encoding = 'utf-8';
 
     /**
-     * Open or create archive (depending on $mode)
+     * Open or create archive (depending on $mode).
      *
      * @param string $archivepathname archive path name
      * @param int $mode OPEN, CREATE or OVERWRITE constant
      *
      * @param string $archivepathname archive path name
      * @param int $mode OPEN, CREATE or OVERWRITE constant
@@ -57,14 +57,14 @@ abstract class file_archive implements Iterator {
     public abstract function open($archivepathname, $mode=file_archive::CREATE, $encoding='utf-8');
 
     /**
     public abstract function open($archivepathname, $mode=file_archive::CREATE, $encoding='utf-8');
 
     /**
-     * Close archive
+     * Close archive.
      *
      * @return bool success
      */
     public abstract function close();
 
     /**
      *
      * @return bool success
      */
     public abstract function close();
 
     /**
-     * Returns file stream for reading of content
+     * Returns file stream for reading of content.
      *
      * @param int $index index of file
      * @return stream|bool stream or false if error
      *
      * @param int $index index of file
      * @return stream|bool stream or false if error
@@ -72,7 +72,7 @@ abstract class file_archive implements Iterator {
     public abstract function get_stream($index);
 
     /**
     public abstract function get_stream($index);
 
     /**
-     * Returns file information
+     * Returns file information.
      *
      * @param int $index index of file
      * @return stdClass|bool object or false if error
      *
      * @param int $index index of file
      * @return stdClass|bool object or false if error
@@ -80,21 +80,21 @@ abstract class file_archive implements Iterator {
     public abstract function get_info($index);
 
     /**
     public abstract function get_info($index);
 
     /**
-     * Returns array of info about all files in archive
+     * Returns array of info about all files in archive.
      *
      * @return array of file infos
      */
     public abstract function list_files();
 
     /**
      *
      * @return array of file infos
      */
     public abstract function list_files();
 
     /**
-     * Returns number of files in archive
+     * Returns number of files in archive.
      *
      * @return int number of files
      */
     public abstract function count();
 
     /**
      *
      * @return int number of files
      */
     public abstract function count();
 
     /**
-     * Add file into archive
+     * Add file into archive.
      *
      * @param string $localname name of file in archive
      * @param string $pathname location of file
      *
      * @param string $localname name of file in archive
      * @param string $pathname location of file
@@ -103,7 +103,7 @@ abstract class file_archive implements Iterator {
     public abstract function add_file_from_pathname($localname, $pathname);
 
     /**
     public abstract function add_file_from_pathname($localname, $pathname);
 
     /**
-     * Add content of string into archive
+     * Add content of string into archive.
      *
      * @param string $localname name of file in archive
      * @param string $contents contents
      *
      * @param string $localname name of file in archive
      * @param string $contents contents
@@ -112,7 +112,7 @@ abstract class file_archive implements Iterator {
     public abstract function add_file_from_string($localname, $contents);
 
     /**
     public abstract function add_file_from_string($localname, $contents);
 
     /**
-     * Add empty directory into archive
+     * Add empty directory into archive.
      *
      * @param string $localname name of file in archive
      * @return bool success
      *
      * @param string $localname name of file in archive
      * @return bool success
@@ -182,25 +182,25 @@ abstract class file_archive implements Iterator {
     }
 
     /**
     }
 
     /**
-     * Returns current file info
+     * Returns current file info.
      * @return object
      */
     //public abstract function current();
 
     /**
      * @return object
      */
     //public abstract function current();
 
     /**
-     * Returns the index of current file
+     * Returns the index of current file.
      * @return int current file index
      */
     //public abstract function key();
 
     /**
      * @return int current file index
      */
     //public abstract function key();
 
     /**
-     * Moves forward to next file
+     * Moves forward to next file.
      * @return void
      */
     //public abstract function next();
 
     /**
      * @return void
      */
     //public abstract function next();
 
     /**
-     * Rewinds back to the first file
+     * Rewinds back to the first file.
      * @return void
      */
     //public abstract function rewind();
      * @return void
      */
     //public abstract function rewind();
index 425e304..92e39e5 100644 (file)
@@ -25,7 +25,7 @@
 defined('MOODLE_INTERNAL') || die();
 
 /**
 defined('MOODLE_INTERNAL') || die();
 
 /**
- * Basic file related exception class
+ * Basic file related exception class.
  *
  * @package   core_files
  * @category  files
  *
  * @package   core_files
  * @category  files
@@ -46,7 +46,7 @@ class file_exception extends moodle_exception {
 }
 
 /**
 }
 
 /**
- * Can not create file exception
+ * Can not create file exception.
  *
  * @package   core_files
  * @category  files
  *
  * @package   core_files
  * @category  files
@@ -55,7 +55,7 @@ class file_exception extends moodle_exception {
  */
 class stored_file_creation_exception extends file_exception {
     /**
  */
 class stored_file_creation_exception extends file_exception {
     /**
-     * Constructor
+     * Constructor.
      *
      * @param int $contextid context ID
      * @param string $component component
      *
      * @param int $contextid context ID
      * @param string $component component
@@ -87,7 +87,7 @@ class stored_file_creation_exception extends file_exception {
  */
 class file_access_exception extends file_exception {
     /**
  */
 class file_access_exception extends file_exception {
     /**
-     * Constructor
+     * Constructor.
      *
      * @param string $debuginfo extra debug info
      */
      *
      * @param string $debuginfo extra debug info
      */
@@ -106,7 +106,7 @@ class file_access_exception extends file_exception {
  */
 class file_pool_content_exception extends file_exception {
     /**
  */
 class file_pool_content_exception extends file_exception {
     /**
-     * Constructor
+     * Constructor.
      *
      * @param string $contenthash content hash
      * @param string $debuginfo extra debug info
      *
      * @param string $contenthash content hash
      * @param string $debuginfo extra debug info
@@ -118,7 +118,7 @@ class file_pool_content_exception extends file_exception {
 
 
 /**
 
 
 /**
- * Problem with records in the {files_reference} table
+ * Problem with records in the {files_reference} table.
  *
  * @package   core_files
  * @catehory  files
  *
  * @package   core_files
  * @catehory  files
@@ -127,7 +127,7 @@ class file_pool_content_exception extends file_exception {
  */
 class file_reference_exception extends file_exception {
     /**
  */
 class file_reference_exception extends file_exception {
     /**
-     * Constructor
+     * Constructor.
      *
      * @param int $repositoryid the id of the repository that provides the referenced file
      * @param string $reference the information for the repository to locate the file
      *
      * @param int $repositoryid the id of the repository that provides the referenced file
      * @param string $reference the information for the repository to locate the file
index 76aeead..3fbfb62 100644 (file)
@@ -14,7 +14,6 @@
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
-
 /**
  * Abstraction of general file packer.
  *
 /**
  * Abstraction of general file packer.
  *
@@ -35,7 +34,7 @@ defined('MOODLE_INTERNAL') || die();
 abstract class file_packer {
 
     /**
 abstract class file_packer {
 
     /**
-     * Archive files and store the result in file storage
+     * Archive files and store the result in file storage.
      *
      * @param array $files array with zip paths as keys (archivepath=>ospathname or archivepath=>stored_file)
      * @param int $contextid context ID
      *
      * @param array $files array with zip paths as keys (archivepath=>ospathname or archivepath=>stored_file)
      * @param int $contextid context ID
@@ -45,21 +44,23 @@ abstract class file_packer {
      * @param string $filepath file path
      * @param string $filename file name
      * @param int $userid user ID
      * @param string $filepath file path
      * @param string $filename file name
      * @param int $userid user ID
-     * @return stored_file|bool false if error stored file instance if ok
+     * @param bool $ignoreinvalidfiles true means ignore missing or invalid files, false means abort on any error
+     * @return stored_file|bool false if error stored_file instance if ok
      */
      */
-    public abstract function archive_to_storage($files, $contextid, $component, $filearea, $itemid, $filepath, $filename, $userid = NULL);
+    public abstract function archive_to_storage(array $files, $contextid, $component, $filearea, $itemid, $filepath, $filename, $userid = NULL, $ignoreinvalidfiles=true);
 
     /**
 
     /**
-     * Archive files and store the result in os file
+     * Archive files and store the result in os file.
      *
      * @param array $files array with zip paths as keys (archivepath=>ospathname or archivepath=>stored_file)
      * @param string $archivefile path to target zip file
      *
      * @param array $files array with zip paths as keys (archivepath=>ospathname or archivepath=>stored_file)
      * @param string $archivefile path to target zip file
-     * @return bool success
+     * @param bool $ignoreinvalidfiles true means ignore missing or invalid files, false means abort on any error
+     * @return bool true if file created, false if not
      */
      */
-    public abstract function archive_to_pathname($files, $archivefile);
+    public abstract function archive_to_pathname(array $files, $archivefile, $ignoreinvalidfiles=true);
 
     /**
 
     /**
-     * Extract file to given file path (real OS filesystem), existing files are overwrited
+     * Extract file to given file path (real OS filesystem), existing files are overwritten.
      *
      * @param stored_file|string $archivefile full pathname of zip file or stored_file instance
      * @param string $pathname target directory
      *
      * @param stored_file|string $archivefile full pathname of zip file or stored_file instance
      * @param string $pathname target directory
@@ -69,7 +70,7 @@ abstract class file_packer {
     public abstract function extract_to_pathname($archivefile, $pathname, array $onlyfiles = NULL);
 
     /**
     public abstract function extract_to_pathname($archivefile, $pathname, array $onlyfiles = NULL);
 
     /**
-     * Extract file to given file path (real OS filesystem), existing files are overwrited
+     * Extract file to given file path (real OS filesystem), existing files are overwritten.
      *
      * @param string|stored_file $archivefile full pathname of zip file or stored_file instance
      * @param int $contextid context ID
      *
      * @param string|stored_file $archivefile full pathname of zip file or stored_file instance
      * @param int $contextid context ID
@@ -83,7 +84,7 @@ abstract class file_packer {
     public abstract function extract_to_storage($archivefile, $contextid, $component, $filearea, $itemid, $pathbase, $userid = NULL);
 
     /**
     public abstract function extract_to_storage($archivefile, $contextid, $component, $filearea, $itemid, $pathbase, $userid = NULL);
 
     /**
-     * Returns array of info about all files in archive
+     * Returns array of info about all files in archive.
      *
      * @param string|file_archive $archivefile
      * @return array of file infos
      *
      * @param string|file_archive $archivefile
      * @return array of file infos
diff --git a/lib/filestorage/tests/fixtures/empty.zip b/lib/filestorage/tests/fixtures/empty.zip
new file mode 100644 (file)
index 0000000..15cb0ec
Binary files /dev/null and b/lib/filestorage/tests/fixtures/empty.zip differ
index f04dbe3..098223f 100644 (file)
@@ -36,14 +36,24 @@ $archive = $_SERVER['argv'][1];
 
 // Note: the ZIP structure is described at http://www.pkware.com/documents/casestudies/APPNOTE.TXT
 if (!$filesize = filesize($archive) or !$fp = fopen($archive, 'rb+')) {
 
 // Note: the ZIP structure is described at http://www.pkware.com/documents/casestudies/APPNOTE.TXT
 if (!$filesize = filesize($archive) or !$fp = fopen($archive, 'rb+')) {
-    cli_error("Can not open file file $archive");
+    cli_error("Can not open ZIP archive: $archive");
+}
+
+if ($filesize == 22) {
+    $info = unpack('Vsig', fread($fp, 4));
+    fclose($fp);
+    if ($info['sig'] == 0x6054b50) {
+        cli_error("This ZIP archive is empty: $archive");
+    } else {
+        cli_error("This is not a ZIP archive: $archive");
+    }
 }
 
 fseek($fp, 0);
 $info = unpack('Vsig', fread($fp, 4));
 if ($info['sig'] !== 0x04034b50) {
     fclose($fp);
 }
 
 fseek($fp, 0);
 $info = unpack('Vsig', fread($fp, 4));
 if ($info['sig'] !== 0x04034b50) {
     fclose($fp);
-    cli_error("This is not a zip file: $archive");
+    cli_error("This is not a ZIP archive: $archive");
 }
 
 // Find end of central directory record.
 }
 
 // Find end of central directory record.
index d98183f..d71b798 100644 (file)
@@ -116,6 +116,11 @@ class zip_packer_testcase extends advanced_testcase {
             $this->assertTrue($file->pathname === 'Žluťoučký/Koníček.txt' or $file->pathname === 'testíček.txt' or $file->pathname === 'test.test');
         }
         $zip_archive->close();
             $this->assertTrue($file->pathname === 'Žluťoučký/Koníček.txt' or $file->pathname === 'testíček.txt' or $file->pathname === 'test.test');
         }
         $zip_archive->close();
+
+        // Empty archive extraction.
+        $archive = __DIR__.'/fixtures/empty.zip';
+        $archivefiles = $packer->list_files($archive);
+        $this->assertSame(array(), $archivefiles);
     }
 
     /**
     }
 
     /**
@@ -129,10 +134,10 @@ class zip_packer_testcase extends advanced_testcase {
         $packer = get_file_packer('application/zip');
         $archive = "$CFG->tempdir/archive.zip";
 
         $packer = get_file_packer('application/zip');
         $archive = "$CFG->tempdir/archive.zip";
 
-        $this->assertFalse(file_exists($archive));
+        $this->assertFileNotExists($archive);
         $result = $packer->archive_to_pathname($this->files, $archive);
         $this->assertTrue($result);
         $result = $packer->archive_to_pathname($this->files, $archive);
         $this->assertTrue($result);
-        $this->assertTrue(file_exists($archive));
+        $this->assertFileExists($archive);
 
         $archivefiles = $packer->list_files($archive);
         $this->assertTrue(is_array($archivefiles));
 
         $archivefiles = $packer->list_files($archive);
         $this->assertTrue(is_array($archivefiles));
@@ -143,31 +148,37 @@ class zip_packer_testcase extends advanced_testcase {
 
         // Test invalid files parameter.
         $archive = "$CFG->tempdir/archive2.zip";
 
         // Test invalid files parameter.
         $archive = "$CFG->tempdir/archive2.zip";
-        $this->assertFalse(file_exists($archive));
+        $this->assertFileNotExists($archive);
 
 
-        $this->assertFalse(file_exists(__DIR__.'/xx/yy/ee.txt'));
+        $this->assertFileNotExists(__DIR__.'/xx/yy/ee.txt');
         $files = array('xtest.txt'=>__DIR__.'/xx/yy/ee.txt');
 
         $files = array('xtest.txt'=>__DIR__.'/xx/yy/ee.txt');
 
-        // TODO MDL-37429 Ugly hack to handle multiple debugging message. To be
-        // removed once this issue has been integrated.
-        ob_start();
-        $result = $packer->archive_to_pathname($files, $archive);
-        $d = ob_end_clean();
-        $this->assertTrue($d !== '');
+        $result = $packer->archive_to_pathname($files, $archive, false);
         $this->assertFalse($result);
         $this->assertFalse($result);
-        // TODO MDL-37429 Uncomment the following line and remove the ugly hack above once this
-        // issue has been integrated. In a nutshell, we need support for multiple debug messages.
-        // $this->assertDebuggingCalled();
-
-        $this->assertTrue(file_exists(__DIR__.'/fixtures/test.txt'));
-        $files = array();
-        $files['""""'] = null; // Invalid directory name.
-        $files['test.txt'] = __DIR__.'/fixtures/test.txt';
+        $this->assertDebuggingCalled();
+        $this->assertFileNotExists($archive);
+
         $result = $packer->archive_to_pathname($files, $archive);
         $this->assertTrue($result);
         $result = $packer->archive_to_pathname($files, $archive);
         $this->assertTrue($result);
-        $this->resetDebugging();
+        $this->assertFileExists($archive);
+        $this->assertDebuggingCalled();
+        $archivefiles = $packer->list_files($archive);
+        $this->assertSame(array(), $archivefiles);
+        unlink($archive);
 
 
-        @unlink($archive);
+        $this->assertFileNotExists(__DIR__.'/xx/yy/ee.txt');
+        $this->assertFileExists(__DIR__.'/fixtures/test.txt');
+        $files = array('xtest.txt'=>__DIR__.'/xx/yy/ee.txt', 'test.txt'=>__DIR__.'/fixtures/test.txt', 'ytest.txt'=>__DIR__.'/xx/yy/yy.txt');
+        $result = $packer->archive_to_pathname($files, $archive);
+        $this->assertTrue($result);
+        $this->assertFileExists($archive);
+        $archivefiles = $packer->list_files($archive);
+        $this->assertCount(1, $archivefiles);
+        $this->assertEquals('test.txt', $archivefiles[0]->pathname);
+        $dms = $this->getDebuggingMessages();
+        $this->assertCount(2, $dms);
+        $this->resetDebugging();
+        unlink($archive);
     }
 
     /**
     }
 
     /**
@@ -212,13 +223,13 @@ class zip_packer_testcase extends advanced_testcase {
         $this->assertTrue(is_dir($target));
 
         $archive = "$CFG->tempdir/archive.zip";
         $this->assertTrue(is_dir($target));
 
         $archive = "$CFG->tempdir/archive.zip";
-        $this->assertTrue(file_exists($archive));
+        $this->assertFileExists($archive);
         $result = $packer->extract_to_pathname($archive, $target);
         $this->assertTrue(is_array($result));
         $this->assertEquals(count($this->files), count($result));
         foreach($this->files as $file=>$unused) {
             $this->assertTrue($result[$file]);
         $result = $packer->extract_to_pathname($archive, $target);
         $this->assertTrue(is_array($result));
         $this->assertEquals(count($this->files), count($result));
         foreach($this->files as $file=>$unused) {
             $this->assertTrue($result[$file]);
-            $this->assertTrue(file_exists($target.$file));
+            $this->assertFileExists($target.$file);
             $this->assertSame($testcontent, file_get_contents($target.$file));
         }
 
             $this->assertSame($testcontent, file_get_contents($target.$file));
         }
 
@@ -229,7 +240,7 @@ class zip_packer_testcase extends advanced_testcase {
         $this->assertEquals(count($this->files), count($result));
         foreach($this->files as $file=>$unused) {
             $this->assertTrue($result[$file]);
         $this->assertEquals(count($this->files), count($result));
         foreach($this->files as $file=>$unused) {
             $this->assertTrue($result[$file]);
-            $this->assertTrue(file_exists($target.$file));
+            $this->assertFileExists($target.$file);
             $this->assertSame($testcontent, file_get_contents($target.$file));
         }
     }
             $this->assertSame($testcontent, file_get_contents($target.$file));
         }
     }
@@ -257,19 +268,19 @@ class zip_packer_testcase extends advanced_testcase {
         $donotextract = array_diff(array_keys($this->files), $onlyfiles);
 
         $archive = "$CFG->tempdir/archive.zip";
         $donotextract = array_diff(array_keys($this->files), $onlyfiles);
 
         $archive = "$CFG->tempdir/archive.zip";
-        $this->assertTrue(file_exists($archive));
+        $this->assertFileExists($archive);
         $result = $packer->extract_to_pathname($archive, $target, $onlyfiles);
         $this->assertTrue(is_array($result));
         $this->assertEquals(count($willbeextracted), count($result));
 
         foreach($willbeextracted as $file) {
             $this->assertTrue($result[$file]);
         $result = $packer->extract_to_pathname($archive, $target, $onlyfiles);
         $this->assertTrue(is_array($result));
         $this->assertEquals(count($willbeextracted), count($result));
 
         foreach($willbeextracted as $file) {
             $this->assertTrue($result[$file]);
-            $this->assertTrue(file_exists($target.$file));
+            $this->assertFileExists($target.$file);
             $this->assertSame($testcontent, file_get_contents($target.$file));
         }
         foreach($donotextract as $file) {
             $this->assertFalse(isset($result[$file]));
             $this->assertSame($testcontent, file_get_contents($target.$file));
         }
         foreach($donotextract as $file) {
             $this->assertFalse(isset($result[$file]));
-            $this->assertFalse(file_exists($target.$file));
+            $this->assertFileNotExists($target.$file);
         }
 
     }
         }
 
     }
@@ -280,7 +291,7 @@ class zip_packer_testcase extends advanced_testcase {
     public function test_extract_to_storage() {
         global $CFG;
 
     public function test_extract_to_storage() {
         global $CFG;
 
-        $this->resetAfterTest(true);
+        $this->resetAfterTest(false);
 
         $packer = get_file_packer('application/zip');
         $fs = get_file_storage();
 
         $packer = get_file_packer('application/zip');
         $fs = get_file_storage();
@@ -301,7 +312,7 @@ class zip_packer_testcase extends advanced_testcase {
         }
 
         $archive = "$CFG->tempdir/archive.zip";
         }
 
         $archive = "$CFG->tempdir/archive.zip";
-        $this->assertTrue(file_exists($archive));
+        $this->assertFileExists($archive);
         $result = $packer->extract_to_storage($archive, $context->id, 'phpunit', 'target', 0, '/');
         $this->assertTrue(is_array($result));
         $this->assertEquals(count($this->files), count($result));
         $result = $packer->extract_to_storage($archive, $context->id, 'phpunit', 'target', 0, '/');
         $this->assertTrue(is_array($result));
         $this->assertEquals(count($this->files), count($result));
@@ -311,5 +322,90 @@ class zip_packer_testcase extends advanced_testcase {
             $this->assertInstanceOf('stored_file', $stored_file);
             $this->assertSame($testcontent, $stored_file->get_content());
         }
             $this->assertInstanceOf('stored_file', $stored_file);
             $this->assertSame($testcontent, $stored_file->get_content());
         }
+        unlink($archive);
+    }
+
+    /**
+     * @depends test_extract_to_storage
+     */
+    public function test_add_files() {
+        global $CFG;
+
+        $this->resetAfterTest(false);
+
+        $packer = get_file_packer('application/zip');
+        $archive = "$CFG->tempdir/archive.zip";
+
+        $this->assertFileNotExists($archive);
+        $packer->archive_to_pathname(array(), $archive);
+        $this->assertFileExists($archive);
+
+        $zip_archive = new zip_archive();
+        $zip_archive->open($archive, file_archive::OPEN);
+        $this->assertEquals(0, $zip_archive->count());
+
+        $zip_archive->add_file_from_string('test.txt', 'test');
+        $zip_archive->close();
+        $zip_archive->open($archive, file_archive::OPEN);
+        $this->assertEquals(1, $zip_archive->count());
+
+        $zip_archive->add_directory('test2');
+        $zip_archive->close();
+        $zip_archive->open($archive, file_archive::OPEN);
+        $files = $zip_archive->list_files();
+        $this->assertCount(2, $files);
+        $this->assertEquals('test.txt', $files[0]->pathname);
+        $this->assertEquals('test2/', $files[1]->pathname);
+
+        $result = $zip_archive->add_file_from_pathname('test.txt', __DIR__.'/nonexistent/file.txt');
+        $this->assertFalse($result);
+        $zip_archive->close();
+        $zip_archive->open($archive, file_archive::OPEN);
+        $this->assertEquals(2, $zip_archive->count());
+
+        unlink($archive);
+    }
+
+    /**
+     * @depends test_add_files
+     */
+    public function test_open_archive() {
+        global $CFG;
+
+        $this->resetAfterTest(true);
+
+        $archive = "$CFG->tempdir/archive.zip";
+
+        $this->assertFileNotExists($archive);
+
+        $zip_archive = new zip_archive();
+        $result = $zip_archive->open($archive, file_archive::OPEN);
+        $this->assertFalse($result);
+        $this->assertDebuggingCalled();
+
+        $zip_archive = new zip_archive();
+        $result = $zip_archive->open($archive, file_archive::CREATE);
+        $this->assertTrue($result);
+        $zip_archive->add_file_from_string('test.txt', 'test');
+        $zip_archive->close();
+        $zip_archive->open($archive, file_archive::OPEN);
+        $this->assertEquals(1, $zip_archive->count());
+
+        $zip_archive = new zip_archive();
+        $result = $zip_archive->open($archive, file_archive::OVERWRITE);
+        $this->assertTrue($result);
+        $zip_archive->add_file_from_string('test2.txt', 'test');
+        $zip_archive->close();
+        $zip_archive->open($archive, file_archive::OPEN);
+        $this->assertEquals(1, $zip_archive->count());
+
+        unlink($archive);
+        $zip_archive = new zip_archive();
+        $result = $zip_archive->open($archive, file_archive::OVERWRITE);
+        $this->assertTrue($result);
+        $zip_archive->add_file_from_string('test2.txt', 'test');
+        $zip_archive->close();
+        $zip_archive->open($archive, file_archive::OPEN);
+        $this->assertEquals(1, $zip_archive->count());
     }
 }
     }
 }
index 435660e..7f140da 100644 (file)
@@ -14,7 +14,6 @@
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
-
 /**
  * Implementation of zip file archive.
  *
 /**
  * Implementation of zip file archive.
  *
@@ -28,7 +27,7 @@ defined('MOODLE_INTERNAL') || die();
 require_once("$CFG->libdir/filestorage/file_archive.php");
 
 /**
 require_once("$CFG->libdir/filestorage/file_archive.php");
 
 /**
- * zip file archive class.
+ * Zip file archive class.
  *
  * @package   core_files
  * @category  files
  *
  * @package   core_files
  * @category  files
@@ -55,7 +54,7 @@ class zip_archive extends file_archive {
     /** @var bool was this archive modified? */
     protected $modified = false;
 
     /** @var bool was this archive modified? */
     protected $modified = false;
 
-    /** @var array unicode decoding array, created by decoding zip file*/
+    /** @var array unicode decoding array, created by decoding zip file */
     protected $namelookup = null;
 
     /**
     protected $namelookup = null;
 
     /**
@@ -66,7 +65,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Open or create archive (depending on $mode)
+     * Open or create archive (depending on $mode).
      *
      * @todo MDL-31048 return error message
      * @param string $archivepathname
      *
      * @todo MDL-31048 return error message
      * @param string $archivepathname
@@ -102,9 +101,21 @@ class zip_archive extends file_archive {
             return true;
 
         } else {
             return true;
 
         } else {
+            $message = 'Unknown error.';
+            switch ($result) {
+                case ZIPARCHIVE::ER_EXISTS: $message = 'File already exists.'; break;
+                case ZIPARCHIVE::ER_INCONS: $message = 'Zip archive inconsistent.'; break;
+                case ZIPARCHIVE::ER_INVAL: $message = 'Invalid argument.'; break;
+                case ZIPARCHIVE::ER_MEMORY: $message = 'Malloc failure.'; break;
+                case ZIPARCHIVE::ER_NOENT: $message = 'No such file.'; break;
+                case ZIPARCHIVE::ER_NOZIP: $message = 'Not a zip archive.'; break;
+                case ZIPARCHIVE::ER_OPEN: $message = 'Can\'t open file.'; break;
+                case ZIPARCHIVE::ER_READ: $message = 'Read error.'; break;
+                case ZIPARCHIVE::ER_SEEK: $message = 'Seek error.'; break;
+            }
+            debugging($message.': '.$archivepathname, DEBUG_DEVELOPER);
             $this->za = null;
             $this->archivepathname = null;
             $this->za = null;
             $this->archivepathname = null;
-            // TODO: maybe we should return some error info
             return false;
         }
     }
             return false;
         }
     }
@@ -140,7 +151,7 @@ class zip_archive extends file_archive {
 
         if (!isset($this->namelookup[$localname])) {
             $name = $localname;
 
         if (!isset($this->namelookup[$localname])) {
             $name = $localname;
-            // This should not happen
+            // This should not happen.
             if (!empty($this->encoding) and $this->encoding !== 'utf-8') {
                 $name = @textlib::convert($name, $this->encoding, 'utf-8');
             }
             if (!empty($this->encoding) and $this->encoding !== 'utf-8') {
                 $name = @textlib::convert($name, $this->encoding, 'utf-8');
             }
@@ -153,7 +164,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Close archive
+     * Close archive, write changes to disk.
      *
      * @return bool success
      */
      *
      * @return bool success
      */
@@ -162,6 +173,21 @@ class zip_archive extends file_archive {
             return false;
         }
 
             return false;
         }
 
+        if ($this->za->numFiles == 0) {
+            // PHP can not create empty archives, so let's fake it.
+            $this->za->close();
+            $this->za = null;
+            $this->mode = null;
+            $this->namelookup = null;
+            $this->modified = false;
+            @unlink($this->archivepathname);
+            $data = base64_decode('UEsFBgAAAAAAAAAAAAAAAAAAAAAAAA==');
+            if (!file_put_contents($this->archivepathname, $data)) {
+                return false;
+            }
+            return true;
+        }
+
         $res = $this->za->close();
         $this->za = null;
         $this->mode = null;
         $res = $this->za->close();
         $this->za = null;
         $this->mode = null;
@@ -176,7 +202,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Returns file stream for reading of content
+     * Returns file stream for reading of content.
      *
      * @param int $index index of file
      * @return resource|bool file handle or false if error
      *
      * @param int $index index of file
      * @return resource|bool file handle or false if error
@@ -195,7 +221,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Returns file information
+     * Returns file information.
      *
      * @param int $index index of file
      * @return stdClass|bool info object or false if error
      *
      * @param int $index index of file
      * @return stdClass|bool info object or false if error
@@ -233,7 +259,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Returns array of info about all files in archive
+     * Returns array of info about all files in archive.
      *
      * @return array of file infos
      */
      *
      * @return array of file infos
      */
@@ -256,7 +282,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Returns number of files in archive
+     * Returns number of files in archive.
      *
      * @return int number of files
      */
      *
      * @return int number of files
      */
@@ -269,7 +295,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Add file into archive
+     * Add file into archive.
      *
      * @param string $localname name of file in archive
      * @param string $pathname location of file
      *
      * @param string $localname name of file in archive
      * @param string $pathname location of file
@@ -281,32 +307,25 @@ class zip_archive extends file_archive {
         }
 
         if ($this->archivepathname === realpath($pathname)) {
         }
 
         if ($this->archivepathname === realpath($pathname)) {
-            // do not add self into archive
+            // Do not add self into archive.
+            return false;
+        }
+
+        if (!is_readable($pathname) or is_dir($pathname)) {
             return false;
         }
 
         if (is_null($localname)) {
             $localname = clean_param($pathname, PARAM_PATH);
         }
             return false;
         }
 
         if (is_null($localname)) {
             $localname = clean_param($pathname, PARAM_PATH);
         }
-        $localname = trim($localname, '/'); // no leading slashes in archives
+        $localname = trim($localname, '/'); // No leading slashes in archives!
         $localname = $this->mangle_pathname($localname);
 
         if ($localname === '') {
         $localname = $this->mangle_pathname($localname);
 
         if ($localname === '') {
-            //sorry - conversion failed badly
+            // Sorry - conversion failed badly.
             return false;
         }
 
             return false;
         }
 
-        if (!check_php_version('5.2.8')) {
-            // workaround for open file handles problem, ZipArchive uses file locking in order to prevent file modifications before the close() (strange, eh?)
-            if ($this->count() > 0 and $this->count() % 500 === 0) {
-                $this->close();
-                $res = $this->open($this->archivepathname, file_archive::OPEN, $this->encoding);
-                if ($res !== true) {
-                    print_error('cannotopenzip');
-                }
-            }
-        }
-
         if (!$this->za->addFile($pathname, $localname)) {
             return false;
         }
         if (!$this->za->addFile($pathname, $localname)) {
             return false;
         }
@@ -315,7 +334,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Add content of string into archive
+     * Add content of string into archive.
      *
      * @param string $localname name of file in archive
      * @param string $contents contents
      *
      * @param string $localname name of file in archive
      * @param string $contents contents
@@ -326,16 +345,16 @@ class zip_archive extends file_archive {
             return false;
         }
 
             return false;
         }
 
-        $localname = trim($localname, '/'); // no leading slashes in archives
+        $localname = trim($localname, '/'); // No leading slashes in archives!
         $localname = $this->mangle_pathname($localname);
 
         if ($localname === '') {
         $localname = $this->mangle_pathname($localname);
 
         if ($localname === '') {
-            //sorry - conversion failed badly
+            // Sorry - conversion failed badly.
             return false;
         }
 
         if ($this->usedmem > 2097151) {
             return false;
         }
 
         if ($this->usedmem > 2097151) {
-            // this prevents running out of memory when adding many large files using strings
+            // This prevents running out of memory when adding many large files using strings.
             $this->close();
             $res = $this->open($this->archivepathname, file_archive::OPEN, $this->encoding);
             if ($res !== true) {
             $this->close();
             $res = $this->open($this->archivepathname, file_archive::OPEN, $this->encoding);
             if ($res !== true) {
@@ -352,7 +371,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Add empty directory into archive
+     * Add empty directory into archive.
      *
      * @param string $localname name of file in archive
      * @return bool success
      *
      * @param string $localname name of file in archive
      * @return bool success
@@ -365,7 +384,7 @@ class zip_archive extends file_archive {
         $localname = $this->mangle_pathname($localname);
 
         if ($localname === '/') {
         $localname = $this->mangle_pathname($localname);
 
         if ($localname === '/') {
-            //sorry - conversion failed badly
+            // Sorry - conversion failed badly.
             return false;
         }
 
             return false;
         }
 
@@ -379,7 +398,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Returns current file info
+     * Returns current file info.
      *
      * @return stdClass
      */
      *
      * @return stdClass
      */
@@ -392,7 +411,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Returns the index of current file
+     * Returns the index of current file.
      *
      * @return int current file index
      */
      *
      * @return int current file index
      */
@@ -401,14 +420,14 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Moves forward to next file
+     * Moves forward to next file.
      */
     public function next() {
         $this->pos++;
     }
 
     /**
      */
     public function next() {
         $this->pos++;
     }
 
     /**
-     * Rewinds back to the first file
+     * Rewinds back to the first file.
      */
     public function rewind() {
         $this->pos = 0;
      */
     public function rewind() {
         $this->pos = 0;
@@ -696,7 +715,7 @@ class zip_archive extends file_archive {
     }
 
     /**
     }
 
     /**
-     * Parse file header
+     * Parse file header.
      * @internal
      * @param string $data
      * @param array $centralend
      * @internal
      * @param string $data
      * @param array $centralend
index c30b479..2f5bfc3 100644 (file)
@@ -14,7 +14,6 @@
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
-
 /**
  * Implementation of zip packer.
  *
 /**
  * Implementation of zip packer.
  *
@@ -39,7 +38,7 @@ require_once("$CFG->libdir/filestorage/zip_archive.php");
 class zip_packer extends file_packer {
 
     /**
 class zip_packer extends file_packer {
 
     /**
-     * Zip files and store the result in file storage
+     * Zip files and store the result in file storage.
      *
      * @param array $files array with full zip paths (including directory information)
      *              as keys (archivepath=>ospathname or archivepath/subdir=>stored_file or archivepath=>array('content_as_string'))
      *
      * @param array $files array with full zip paths (including directory information)
      *              as keys (archivepath=>ospathname or archivepath/subdir=>stored_file or archivepath=>array('content_as_string'))
@@ -50,9 +49,10 @@ class zip_packer extends file_packer {
      * @param string $filepath file path
      * @param string $filename file name
      * @param int $userid user ID
      * @param string $filepath file path
      * @param string $filename file name
      * @param int $userid user ID
-     * @return stored_file|bool false if error stored file instance if ok
+     * @param bool $ignoreinvalidfiles true means ignore missing or invalid files, false means abort on any error
+     * @return stored_file|bool false if error stored_file instance if ok
      */
      */
-    public function archive_to_storage($files, $contextid, $component, $filearea, $itemid, $filepath, $filename, $userid = NULL) {
+    public function archive_to_storage(array $files, $contextid, $component, $filearea, $itemid, $filepath, $filename, $userid = NULL, $ignoreinvalidfiles=true) {
         global $CFG;
 
         $fs = get_file_storage();
         global $CFG;
 
         $fs = get_file_storage();
@@ -60,7 +60,7 @@ class zip_packer extends file_packer {
         check_dir_exists($CFG->tempdir.'/zip');
         $tmpfile = tempnam($CFG->tempdir.'/zip', 'zipstor');
 
         check_dir_exists($CFG->tempdir.'/zip');
         $tmpfile = tempnam($CFG->tempdir.'/zip', 'zipstor');
 
-        if ($result = $this->archive_to_pathname($files, $tmpfile)) {
+        if ($result = $this->archive_to_pathname($files, $tmpfile, $ignoreinvalidfiles)) {
             if ($file = $fs->get_file($contextid, $component, $filearea, $itemid, $filepath, $filename)) {
                 if (!$file->delete()) {
                     @unlink($tmpfile);
             if ($file = $fs->get_file($contextid, $component, $filearea, $itemid, $filepath, $filename)) {
                 if (!$file->delete()) {
                     @unlink($tmpfile);
@@ -84,70 +84,78 @@ class zip_packer extends file_packer {
     }
 
     /**
     }
 
     /**
-     * Zip files and store the result in os file
+     * Zip files and store the result in os file.
      *
      * @param array $files array with zip paths as keys (archivepath=>ospathname or archivepath=>stored_file or archivepath=>array('content_as_string'))
      * @param string $archivefile path to target zip file
      *
      * @param array $files array with zip paths as keys (archivepath=>ospathname or archivepath=>stored_file or archivepath=>array('content_as_string'))
      * @param string $archivefile path to target zip file
-     * @return bool success
+     * @param bool $ignoreinvalidfiles true means ignore missing or invalid files, false means abort on any error
+     * @return bool true if file created, false if not
      */
      */
-    public function archive_to_pathname($files, $archivefile) {
-        if (!is_array($files)) {
-            return false;
-        }
-
+    public function archive_to_pathname(array $files, $archivefile, $ignoreinvalidfiles=true) {
         $ziparch = new zip_archive();
         if (!$ziparch->open($archivefile, file_archive::OVERWRITE)) {
             return false;
         }
 
         $ziparch = new zip_archive();
         if (!$ziparch->open($archivefile, file_archive::OVERWRITE)) {
             return false;
         }
 
-        $result = false; // One processed file or dir means success here.
-
+        $abort = false;
         foreach ($files as $archivepath => $file) {
             $archivepath = trim($archivepath, '/');
 
             if (is_null($file)) {
         foreach ($files as $archivepath => $file) {
             $archivepath = trim($archivepath, '/');
 
             if (is_null($file)) {
-                // empty directories have null as content
-                if ($ziparch->add_directory($archivepath.'/')) {
-                    $result = true;
-                } else {
+                // Directories have null as content.
+                if (!$ziparch->add_directory($archivepath.'/')) {
                     debugging("Can not zip '$archivepath' directory", DEBUG_DEVELOPER);
                     debugging("Can not zip '$archivepath' directory", DEBUG_DEVELOPER);
+                    if (!$ignoreinvalidfiles) {
+                        $abort = true;
+                        break;
+                    }
                 }
 
             } else if (is_string($file)) {
                 }
 
             } else if (is_string($file)) {
-                if ($this->archive_pathname($ziparch, $archivepath, $file)) {
-                    $result = true;
-                } else {
+                if (!$this->archive_pathname($ziparch, $archivepath, $file)) {
                     debugging("Can not zip '$archivepath' file", DEBUG_DEVELOPER);
                     debugging("Can not zip '$archivepath' file", DEBUG_DEVELOPER);
+                    if (!$ignoreinvalidfiles) {
+                        $abort = true;
+                        break;
+                    }
                 }
 
             } else if (is_array($file)) {
                 $content = reset($file);
                 }
 
             } else if (is_array($file)) {
                 $content = reset($file);
-                if ($ziparch->add_file_from_string($archivepath, $content)) {
-                    $result = true;
-                } else {
+                if (!$ziparch->add_file_from_string($archivepath, $content)) {
                     debugging("Can not zip '$archivepath' file", DEBUG_DEVELOPER);
                     debugging("Can not zip '$archivepath' file", DEBUG_DEVELOPER);
+                    if (!$ignoreinvalidfiles) {
+                        $abort = true;
+                        break;
+                    }
                 }
 
             } else {
                 }
 
             } else {
-                if ($this->archive_stored($ziparch, $archivepath, $file)) {
-                    $result = true;
-                } else {
+                if (!$this->archive_stored($ziparch, $archivepath, $file)) {
                     debugging("Can not zip '$archivepath' file", DEBUG_DEVELOPER);
                     debugging("Can not zip '$archivepath' file", DEBUG_DEVELOPER);
+                    if (!$ignoreinvalidfiles) {
+                        $abort = true;
+                        break;
+                    }
                 }
             }
         }
 
                 }
             }
         }
 
-        // We can consider that there was an error if the file generated does not contain anything.
-        if ($ziparch->count() == 0) {
-            $result = false;
-            debugging("Nothing was added to the zip file", DEBUG_DEVELOPER);
+        if (!$ziparch->close()) {
+            @unlink($archivefile);
+            return false;
         }
 
         }
 
-        return ($ziparch->close() && $result);
+        if ($abort) {
+            @unlink($archivefile);
+            return false;
+        }
+
+        return true;
     }
 
     /**
     }
 
     /**
-     * Perform archiving file from stored file
+     * Perform archiving file from stored file.
      *
      * @param zip_archive $ziparch zip archive instance
      * @param string $archivepath file path to archive
      *
      * @param zip_archive $ziparch zip archive instance
      * @param string $archivepath file path to archive
@@ -183,7 +191,7 @@ class zip_packer extends file_packer {
     }
 
     /**
     }
 
     /**
-     * Perform archiving file from file path
+     * Perform archiving file from file path.
      *
      * @param zip_archive $ziparch zip archive instance
      * @param string $archivepath file path to archive
      *
      * @param zip_archive $ziparch zip archive instance
      * @param string $archivepath file path to archive
@@ -213,13 +221,13 @@ class zip_packer extends file_packer {
                 $newpath = $archivepath.'/'.$file->getFilename();
                 $this->archive_pathname($ziparch, $newpath, $file->getPathname());
             }
                 $newpath = $archivepath.'/'.$file->getFilename();
                 $this->archive_pathname($ziparch, $newpath, $file->getPathname());
             }
-            unset($files); //release file handles
+            unset($files); // Release file handles.
             return true;
         }
     }
 
     /**
             return true;
         }
     }
 
     /**
-     * Unzip file to given file path (real OS filesystem), existing files are overwrited
+     * Unzip file to given file path (real OS filesystem), existing files are overwritten.
      *
      * @todo MDL-31048 localise messages
      * @param string|stored_file $archivefile full pathname of zip file or stored_file instance
      *
      * @todo MDL-31048 localise messages
      * @param string|stored_file $archivefile full pathname of zip file or stored_file instance
@@ -319,7 +327,7 @@ class zip_packer extends file_packer {
     }
 
     /**
     }
 
     /**
-     * Unzip file to given file path (real OS filesystem), existing files are overwrited
+     * Unzip file to given file path (real OS filesystem), existing files are overwritten.
      *
      * @todo MDL-31048 localise messages
      * @param string|stored_file $archivefile full pathname of zip file or stored_file instance
      *
      * @todo MDL-31048 localise messages
      * @param string|stored_file $archivefile full pathname of zip file or stored_file instance
@@ -375,7 +383,7 @@ class zip_packer extends file_packer {
             }
 
             if ($size < 2097151) {
             }
 
             if ($size < 2097151) {
-                // small file
+                // Small file.
                 if (!$fz = $ziparch->get_stream($info->index)) {
                     $processed[$name] = 'Can not read file from zip archive'; // TODO: localise
                     continue;
                 if (!$fz = $ziparch->get_stream($info->index)) {
                     $processed[$name] = 'Can not read file from zip archive'; // TODO: localise
                     continue;
@@ -469,7 +477,7 @@ class zip_packer extends file_packer {
     }
 
     /**
     }
 
     /**
-     * Returns array of info about all files in archive
+     * Returns array of info about all files in archive.
      *
      * @param string|file_archive $archivefile
      * @return array of file infos
      *
      * @param string|file_archive $archivefile
      * @return array of file infos