From 2c2f4f17c3318133ec9450a3db85610ad3981491 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Mon, 11 Jul 2016 14:05:47 +0800 Subject: [PATCH] MDL-24343 core: Deprecate unzip_file --- lib/componentlib.class.php | 35 +++++++++------- lib/deprecatedlib.php | 6 ++- lib/filestorage/file_packer.php | 4 +- lib/filestorage/mbz_packer.php | 6 ++- lib/filestorage/tests/mbz_packer_test.php | 51 +++++++++++++++++++++++ lib/filestorage/tests/tgz_packer_test.php | 36 ++++++++++++++++ lib/filestorage/tests/zip_packer_test.php | 35 ++++++++++++++++ lib/filestorage/tgz_packer.php | 29 +++++++++++-- lib/filestorage/zip_packer.php | 18 +++++++- lib/upgrade.txt | 5 ++- question/format/webct/format.php | 8 ++-- 11 files changed, 202 insertions(+), 31 deletions(-) diff --git a/lib/componentlib.class.php b/lib/componentlib.class.php index 0d07797c368..858038d7e2a 100644 --- a/lib/componentlib.class.php +++ b/lib/componentlib.class.php @@ -268,15 +268,13 @@ class component_installer { * compare md5 values, download, unzip, install and regenerate * local md5 file * - * @global object * @uses COMPONENT_ERROR * @uses COMPONENT_UPTODATE * @uses COMPONENT_ERROR * @uses COMPONENT_INSTALLED * @return int COMPONENT_(ERROR | UPTODATE | INSTALLED) */ - function install() { - + public function install() { global $CFG; /// Check requisites are passed @@ -330,25 +328,30 @@ class component_installer { $this->errorstring='downloadedfilecheckfailed'; return COMPONENT_ERROR; } - /// Move current revision to a safe place - $destinationdir = $CFG->dataroot.'/'.$this->destpath; - $destinationcomponent = $destinationdir.'/'.$this->componentname; - @remove_dir($destinationcomponent.'_old'); // Deleting a possible old version. + + // Move current revision to a safe place. + $destinationdir = $CFG->dataroot . '/' . $this->destpath; + $destinationcomponent = $destinationdir . '/' . $this->componentname; + $destinationcomponentold = $destinationcomponent . '_old'; + @remove_dir($destinationcomponentold); // Deleting a possible old version. // Moving to a safe place. - @rename($destinationcomponent, $destinationcomponent.'_old'); + @rename($destinationcomponent, $destinationcomponentold); - /// Unzip new version - if (!unzip_file($zipfile, $destinationdir, false)) { - /// Error so, go back to the older + // Unzip new version. + $packer = get_file_packer('application/zip'); + $unzipsuccess = $packer->extract_to_pathname($zipfile, $destinationdir, null, null, true); + if (!$unzipsuccess) { @remove_dir($destinationcomponent); - @rename ($destinationcomponent.'_old', $destinationcomponent); - $this->errorstring='cannotunzipfile'; + @rename($destinationcomponentold, $destinationcomponent); + $this->errorstring = 'cannotunzipfile'; return COMPONENT_ERROR; } - /// Delete old component version - @remove_dir($destinationcomponent.'_old'); - /// Create local md5 + + // Delete old component version. + @remove_dir($destinationcomponentold); + + // Create local md5. if ($file = fopen($destinationcomponent.'/'.$this->componentname.'.md5', 'w')) { if (!fwrite($file, $new_md5)) { fclose($file); diff --git a/lib/deprecatedlib.php b/lib/deprecatedlib.php index 96c70b3107b..fcc6cfba0be 100644 --- a/lib/deprecatedlib.php +++ b/lib/deprecatedlib.php @@ -608,11 +608,13 @@ function detect_munged_arguments($string, $allowdots=1) { * @param string $zipfile The zip file to unzip * @param string $destination The location to unzip to * @param bool $showstatus_ignored Unused + * @deprecated since 2.0 MDL-15919 */ function unzip_file($zipfile, $destination = '', $showstatus_ignored = true) { - global $CFG; + debugging(__FUNCTION__ . '() is deprecated. ' + . 'Please use the application/zip file_packer implementation instead.', DEBUG_DEVELOPER); - //Extract everything from zipfile + // Extract everything from zipfile. $path_parts = pathinfo(cleardoubleslashes($zipfile)); $zippath = $path_parts["dirname"]; //The path of the zip file $zipfilename = $path_parts["basename"]; //The name of the zip file diff --git a/lib/filestorage/file_packer.php b/lib/filestorage/file_packer.php index 5e6ec4f6386..468fed46a55 100644 --- a/lib/filestorage/file_packer.php +++ b/lib/filestorage/file_packer.php @@ -94,10 +94,12 @@ abstract class file_packer { * @param string $pathname target directory * @param array $onlyfiles only extract files present in the array * @param file_progress $progress Progress indicator callback or null if not required + * @param bool $returnbool Whether to return a basic true/false indicating error state, or full per-file error + * details. * @return array|bool list of processed files; false if error */ public abstract function extract_to_pathname($archivefile, $pathname, - array $onlyfiles = NULL, file_progress $progress = null); + array $onlyfiles = NULL, file_progress $progress = null, $returnbool = false); /** * Extract file to given file path (real OS filesystem), existing files are overwritten. diff --git a/lib/filestorage/mbz_packer.php b/lib/filestorage/mbz_packer.php index 749c622b12a..3fde394c0f0 100644 --- a/lib/filestorage/mbz_packer.php +++ b/lib/filestorage/mbz_packer.php @@ -95,13 +95,15 @@ class mbz_packer extends file_packer { * @param string $pathname target directory * @param array $onlyfiles only extract files present in the array * @param file_progress $progress Progress indicator callback or null if not required + * @param bool $returnbool Whether to return a basic true/false indicating error state, or full per-file error + * details. * @return array list of processed files (name=>true) * @throws moodle_exception If error */ public function extract_to_pathname($archivefile, $pathname, - array $onlyfiles = null, file_progress $progress = null) { + array $onlyfiles = null, file_progress $progress = null, $returnbool = false) { return $this->get_packer_for_read_operation($archivefile)->extract_to_pathname( - $archivefile, $pathname, $onlyfiles, $progress); + $archivefile, $pathname, $onlyfiles, $progress, $returnbool); } /** diff --git a/lib/filestorage/tests/mbz_packer_test.php b/lib/filestorage/tests/mbz_packer_test.php index 5882218379a..b64c6939f03 100644 --- a/lib/filestorage/tests/mbz_packer_test.php +++ b/lib/filestorage/tests/mbz_packer_test.php @@ -87,4 +87,55 @@ class core_files_mbz_packer_testcase extends advanced_testcase { $this->assertNotEmpty($out); $this->assertEquals('frog', $out->get_content()); } + + public function usezipbackups_provider() { + return [ + 'Use zips' => [true], + 'Use tgz' => [false], + ]; + } + + /** + * @dataProvider usezipbackups_provider + */ + public function test_extract_to_pathname_returnvalue_successful($usezipbackups) { + global $CFG; + $this->resetAfterTest(); + + $packer = get_file_packer('application/vnd.moodle.backup'); + + // Set up basic archive contents. + $files = array('1.txt' => array('frog')); + + // Create 2 archives (each with one file in) in zip mode. + $CFG->usezipbackups = $usezipbackups; + + $mbzfile = make_request_directory() . '/file.mbz'; + $packer->archive_to_pathname($files, $mbzfile); + + $target = make_request_directory(); + $result = $packer->extract_to_pathname($mbzfile, $target, null, null, true); + $this->assertTrue($result); + } + + /** + * @dataProvider usezipbackups_provider + */ + public function test_extract_to_pathname_returnvalue_failure($usezipbackups) { + global $CFG; + $this->resetAfterTest(); + + $packer = get_file_packer('application/vnd.moodle.backup'); + + // Create 2 archives (each with one file in) in zip mode. + $CFG->usezipbackups = $usezipbackups; + + $mbzfile = make_request_directory() . '/file.mbz'; + file_put_contents($mbzfile, 'Content'); + + $target = make_request_directory(); + $result = $packer->extract_to_pathname($mbzfile, $target, null, null, true); + $this->assertDebuggingCalledCount(1); + $this->assertFalse($result); + } } diff --git a/lib/filestorage/tests/tgz_packer_test.php b/lib/filestorage/tests/tgz_packer_test.php index d61d264d2d2..74dba64f91d 100644 --- a/lib/filestorage/tests/tgz_packer_test.php +++ b/lib/filestorage/tests/tgz_packer_test.php @@ -247,6 +247,42 @@ class core_files_tgz_packer_testcase extends advanced_testcase implements file_p $this->assertTrue(is_dir($outdir . '/out6')); } + /** + * Tests extracting files returning only a boolean state with success. + */ + public function test_extract_to_pathname_returnvalue_successful() { + $packer = get_file_packer('application/x-gzip'); + + // Prepare files. + $files = $this->prepare_file_list(); + $archivefile = make_request_directory() . DIRECTORY_SEPARATOR . 'test.tgz'; + $packer->archive_to_pathname($files, $archivefile); + + // Extract same files. + $outdir = make_request_directory(); + $result = $packer->extract_to_pathname($archivefile, $outdir, null, null, true); + + $this->assertTrue($result); + } + + /** + * Tests extracting files returning only a boolean state with failure. + */ + public function test_extract_to_pathname_returnvalue_failure() { + $packer = get_file_packer('application/x-gzip'); + + // Create sample files. + $archivefile = make_request_directory() . DIRECTORY_SEPARATOR . 'test.tgz'; + file_put_contents($archivefile, ''); + + // Extract same files. + $outdir = make_request_directory(); + + $result = $packer->extract_to_pathname($archivefile, $outdir, null, null, true); + + $this->assertFalse($result); + } + /** * Tests the progress reporting. */ diff --git a/lib/filestorage/tests/zip_packer_test.php b/lib/filestorage/tests/zip_packer_test.php index 8da89dd3d80..d7f25004866 100644 --- a/lib/filestorage/tests/zip_packer_test.php +++ b/lib/filestorage/tests/zip_packer_test.php @@ -294,6 +294,41 @@ class core_files_zip_packer_testcase extends advanced_testcase implements file_p } + /** + * @depends test_archive_to_storage + */ + public function test_extract_to_pathname_returnvalue_successful() { + global $CFG; + + $this->resetAfterTest(false); + + $packer = get_file_packer('application/zip'); + + $target = make_request_directory(); + + $archive = "$CFG->tempdir/archive.zip"; + $this->assertFileExists($archive); + $result = $packer->extract_to_pathname($archive, $target, null, null, true); + $this->assertTrue($result); + } + + /** + * @depends test_archive_to_storage + */ + public function test_extract_to_pathname_returnvalue_failure() { + global $CFG; + + $this->resetAfterTest(false); + + $packer = get_file_packer('application/zip'); + + $target = make_request_directory(); + + $archive = "$CFG->tempdir/noarchive.zip"; + $result = $packer->extract_to_pathname($archive, $target, null, null, true); + $this->assertFalse($result); + } + /** * @depends test_archive_to_storage */ diff --git a/lib/filestorage/tgz_packer.php b/lib/filestorage/tgz_packer.php index 5aea5844ed3..23a8cd3c64e 100644 --- a/lib/filestorage/tgz_packer.php +++ b/lib/filestorage/tgz_packer.php @@ -635,14 +635,37 @@ class tgz_packer extends file_packer { * @param string $pathname target directory * @param array $onlyfiles only extract files present in the array * @param file_progress $progress Progress indicator callback or null if not required + * @param bool $returnbool Whether to return a basic true/false indicating error state, or full per-file error + * details. * @return array list of processed files (name=>true) * @throws moodle_exception If error */ public function extract_to_pathname($archivefile, $pathname, - array $onlyfiles = null, file_progress $progress = null) { + array $onlyfiles = null, file_progress $progress = null, $returnbool = false) { $extractor = new tgz_extractor($archivefile); - return $extractor->extract( - new tgz_packer_extract_to_pathname($pathname, $onlyfiles), $progress); + try { + $result = $extractor->extract( + new tgz_packer_extract_to_pathname($pathname, $onlyfiles), $progress); + if ($returnbool) { + if (!is_array($result)) { + return false; + } + foreach ($result as $status) { + if ($status !== true) { + return false; + } + } + return true; + } else { + return $result; + } + } catch (moodle_exception $e) { + if ($returnbool) { + return false; + } else { + throw $e; + } + } } /** diff --git a/lib/filestorage/zip_packer.php b/lib/filestorage/zip_packer.php index 175a55217a1..47395f73b24 100644 --- a/lib/filestorage/zip_packer.php +++ b/lib/filestorage/zip_packer.php @@ -258,10 +258,12 @@ class zip_packer extends file_packer { * @param array $onlyfiles only extract files present in the array. The path to files MUST NOT * start with a /. Example: array('myfile.txt', 'directory/anotherfile.txt') * @param file_progress $progress Progress indicator callback or null if not required + * @param bool $returnbool Whether to return a basic true/false indicating error state, or full per-file error + * details. * @return bool|array list of processed files; false if error */ public function extract_to_pathname($archivefile, $pathname, - array $onlyfiles = null, file_progress $progress = null) { + array $onlyfiles = null, file_progress $progress = null, $returnbool = false) { global $CFG; if (!is_string($archivefile)) { @@ -269,6 +271,7 @@ class zip_packer extends file_packer { } $processed = array(); + $success = true; $pathname = rtrim($pathname, '/'); if (!is_readable($archivefile)) { @@ -308,6 +311,7 @@ class zip_packer extends file_packer { // directory if (is_file($newdir) and !unlink($newdir)) { $processed[$name] = 'Can not create directory, file already exists'; // TODO: localise + $success = false; continue; } if (is_dir($newdir)) { @@ -318,6 +322,7 @@ class zip_packer extends file_packer { $processed[$name] = true; } else { $processed[$name] = 'Can not create directory'; // TODO: localise + $success = false; } } continue; @@ -330,6 +335,7 @@ class zip_packer extends file_packer { if (!is_dir($newdir)) { if (!mkdir($newdir, $CFG->directorypermissions, true)) { $processed[$name] = 'Can not create directory'; // TODO: localise + $success = false; continue; } } @@ -337,10 +343,12 @@ class zip_packer extends file_packer { $newfile = "$newdir/$filename"; if (!$fp = fopen($newfile, 'wb')) { $processed[$name] = 'Can not write target file'; // TODO: localise + $success = false; continue; } if (!$fz = $ziparch->get_stream($info->index)) { $processed[$name] = 'Can not read file from zip archive'; // TODO: localise + $success = false; fclose($fp); continue; } @@ -353,6 +361,7 @@ class zip_packer extends file_packer { fclose($fp); if (filesize($newfile) !== $size) { $processed[$name] = 'Unknown error during zip extraction'; // TODO: localise + $success = false; // something went wrong :-( @unlink($newfile); continue; @@ -360,7 +369,12 @@ class zip_packer extends file_packer { $processed[$name] = true; } $ziparch->close(); - return $processed; + + if ($returnbool) { + return $success; + } else { + return $processed; + } } /** diff --git a/lib/upgrade.txt b/lib/upgrade.txt index ce7be67911a..1686473972d 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -25,7 +25,8 @@ information provided here is intended especially for developers. * The following functions have been deprecated and are not used any more: - get_records_csv() Please use csv_import_reader::load_csv_content() instead. - put_records_csv() Please use download_as_dataformat (lib/dataformatlib.php) instead. - - zip_files() - See MDL-24343 for more information. + - zip_files() - See MDL-24343 for more information. + - unzip_file() - See MDL-24343 for more information. * The password_compat library was removed as it is no longer required. * Phpunit has been upgraded to 5.4.x and following has been deprecated and is not used any more: - setExpectedException(), use @expectedException or $this->expectException() and $this->expectExceptionMessage() @@ -47,6 +48,8 @@ information provided here is intended especially for developers. Calling them through the magic method __call() will throw a coding exception. * The alfresco library has been removed from core. It was an old version of the library which was not compatible with newer versions of Alfresco. +* All file_packer implementations now accept an additional parameter to allow a simple boolean return value instead of + an array of individual file statuses. === 3.1 === diff --git a/question/format/webct/format.php b/question/format/webct/format.php index 83c800e1268..9d1c8155535 100644 --- a/question/format/webct/format.php +++ b/question/format/webct/format.php @@ -255,7 +255,7 @@ class qformat_webct extends qformat_default { * @return bool success */ public function importpostprocess() { - if ($this->tempdir != '') { + if (!empty($this->tempdir)) { fulldelete($this->tempdir); } return true; @@ -280,15 +280,15 @@ class qformat_webct extends qformat_default { } // We are importing a zip file. // Create name for temporary directory. - $uniquecode = time(); - $this->tempdir = make_temp_directory('webct_import/' . $uniquecode); + $this->tempdir = make_request_directory(); if (is_readable($filename)) { if (!copy($filename, $this->tempdir . '/webct.zip')) { $this->error(get_string('cannotcopybackup', 'question')); fulldelete($this->tempdir); return false; } - if (unzip_file($this->tempdir . '/webct.zip', '', false)) { + $packer = get_file_packer('application/zip'); + if ($packer->extract_to_pathname($this->tempdir . '/webct.zip', $this->tempdir, null, null, true)) { $dir = $this->tempdir; if ((($handle = opendir($dir))) == false) { // The directory could not be opened.