From 89ef786a9141beb980250c7e21208a527ebe8e00 Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Wed, 1 Nov 2023 12:36:55 +0100 Subject: [PATCH 01/10] fix: unlink zip file in case of exceptions --- model/ZipExporter.php | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/model/ZipExporter.php b/model/ZipExporter.php index bed44454a..91eb71c4b 100644 --- a/model/ZipExporter.php +++ b/model/ZipExporter.php @@ -31,6 +31,7 @@ use core_kernel_classes_Literal; use core_kernel_classes_Property; use core_kernel_classes_Resource; +use Exception; use oat\oatbox\service\ServiceManager; use oat\taoMediaManager\model\export\service\MediaResourcePreparerInterface; use oat\taoMediaManager\model\export\service\SharedStimulusCSSExporter; @@ -138,17 +139,27 @@ private function getSavePath(string $unsafePath): string return $safePath; } + /** + * @throws common_Exception + * @throws Exception + */ protected function createZipFile($filename, array $exportClasses = [], array $exportFiles = []): string { - $zip = new ZipArchive(); $baseDir = tao_helpers_Export::getExportPath(); $path = $baseDir . '/' . $filename . '.zip'; + $zip = new ZipArchive(); if ($zip->open($path, ZipArchive::CREATE) !== true) { throw new common_Exception('Unable to create zipfile ' . $path); } - if ($zip->numFiles === 0) { + if ($zip->numFiles !== 0) { + $zip->close(); + + return $path; + } + + try { $errors = []; foreach ($exportFiles as $label => $files) { $archivePath = ''; @@ -183,11 +194,17 @@ protected function createZipFile($filename, array $exportClasses = [], array $ex if (!empty($errors)) { throw new ZipExporterFileErrorList($errors); } - } - $zip->close(); + $zip->close(); - return $path; + return $path; + } catch (Exception $e) { + $zip->close(); + + unlink($path); + + throw $e; + } } public function getServiceManager() From f1847b158b6853b340c9c57f49a47d40b8ae41fb Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Wed, 1 Nov 2023 13:41:17 +0100 Subject: [PATCH 02/10] fix: unlink zip file (if exists) in case of exceptions --- model/ZipExporter.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/model/ZipExporter.php b/model/ZipExporter.php index 91eb71c4b..8ed99c124 100644 --- a/model/ZipExporter.php +++ b/model/ZipExporter.php @@ -201,7 +201,9 @@ protected function createZipFile($filename, array $exportClasses = [], array $ex } catch (Exception $e) { $zip->close(); - unlink($path); + if (is_file($path)) { + unlink($path); + } throw $e; } From f42f14746ee2e25937093305059104f553e4403a Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Wed, 1 Nov 2023 20:36:36 +0100 Subject: [PATCH 03/10] fix: add test coverage --- test/unit/model/ZipExporterTest.php | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/unit/model/ZipExporterTest.php b/test/unit/model/ZipExporterTest.php index 2d8828a76..f1da79dc5 100644 --- a/test/unit/model/ZipExporterTest.php +++ b/test/unit/model/ZipExporterTest.php @@ -13,6 +13,7 @@ use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\TestCase; use Psr\Http\Message\StreamInterface; +use ZipArchive; class ZipExporterTest extends TestCase { @@ -51,6 +52,28 @@ public function setUp(): void ->getMock(); } + public function testCreateZipFile() + { + $exportClasses = [ + 'foo' + ]; + + $exportFiles = [ + 'foo' => [ + $this->resourceMock + ] + ]; + + $zip = new ZipArchive(); + $zip->open(self::TMP_TAO_EXPORT_TEST_ZIP, ZipArchive::CREATE); + $zip->addFile(__FILE__); + $zip->close(); + + $this->sut->createZipFile(self::FILENAME, $exportClasses, $exportFiles); + + $this->addToAssertionCount(1); + } + /** * @throws common_Exception */ From 95b3269d515a5689b83f42add114c1fb0d6071b4 Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Wed, 1 Nov 2023 20:50:37 +0100 Subject: [PATCH 04/10] revert: add test coverage --- test/unit/model/ZipExporterTest.php | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/test/unit/model/ZipExporterTest.php b/test/unit/model/ZipExporterTest.php index f1da79dc5..bb3aa61b1 100644 --- a/test/unit/model/ZipExporterTest.php +++ b/test/unit/model/ZipExporterTest.php @@ -52,28 +52,6 @@ public function setUp(): void ->getMock(); } - public function testCreateZipFile() - { - $exportClasses = [ - 'foo' - ]; - - $exportFiles = [ - 'foo' => [ - $this->resourceMock - ] - ]; - - $zip = new ZipArchive(); - $zip->open(self::TMP_TAO_EXPORT_TEST_ZIP, ZipArchive::CREATE); - $zip->addFile(__FILE__); - $zip->close(); - - $this->sut->createZipFile(self::FILENAME, $exportClasses, $exportFiles); - - $this->addToAssertionCount(1); - } - /** * @throws common_Exception */ From 93f740ed108efd2283129d38c0653196edc443b1 Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Thu, 2 Nov 2023 08:11:40 +0100 Subject: [PATCH 05/10] revert: add test coverage --- test/resources/empty.zip | Bin 0 -> 22 bytes test/resources/test.zip | Bin 0 -> 167 bytes test/unit/model/ZipExporterTest.php | 25 +++++++++++++++++++++++++ 3 files changed, 25 insertions(+) create mode 100644 test/resources/empty.zip create mode 100644 test/resources/test.zip diff --git a/test/resources/empty.zip b/test/resources/empty.zip new file mode 100644 index 0000000000000000000000000000000000000000..15cb0ecb3e219d1701294bfdf0fe3f5cb5d208e7 GIT binary patch literal 22 NcmWIWW@Tf*000g10H*)| literal 0 HcmV?d00001 diff --git a/test/resources/test.zip b/test/resources/test.zip new file mode 100644 index 0000000000000000000000000000000000000000..4ab5580af98d1e561885e15588d92b8163758869 GIT binary patch literal 167 zcmWIWW@h1H0D*UQN#T>(t{X7|*&xipAj43ST3n)6Qc)5b!pXpV-`zR&DG--da5FHn zykKTv025pR-i%Cg%(x7Z0GSOoa7iPGg)ov8VkDYz0p6@^5FLyRen5sZh{FH??CBm= literal 0 HcmV?d00001 diff --git a/test/unit/model/ZipExporterTest.php b/test/unit/model/ZipExporterTest.php index bb3aa61b1..4dee56dda 100644 --- a/test/unit/model/ZipExporterTest.php +++ b/test/unit/model/ZipExporterTest.php @@ -22,6 +22,7 @@ class ZipExporterTest extends TestCase private const FILENAME = 'test'; private const TMP_TAO_EXPORT_TEST_ZIP = '/tmp/tao_export/test.zip'; + const RESOURCES_DIR = __DIR__ . '/../../resources/'; private StreamInterface $streamMock; @@ -52,6 +53,28 @@ public function setUp(): void ->getMock(); } + /** + * @throws common_Exception + */ + public function testCreateZipFile() + { + $exportClasses = [ + 'foo' + ]; + + $exportFiles = [ + 'foo' => [ + $this->resourceMock + ] + ]; + + copy(self::RESOURCES_DIR . 'test.zip', self::TMP_TAO_EXPORT_TEST_ZIP); + + $this->sut->createZipFile(self::FILENAME, $exportClasses, $exportFiles); + + $this->addToAssertionCount(1); + } + /** * @throws common_Exception */ @@ -101,6 +124,8 @@ public function testCreateZipFileThrowsMediaReferencesNotFoundException(): void 'Error in Asset class "foo": Media references to Image: foo.jpg FilePath: foo.jpg could not be found.' ); + copy(self::RESOURCES_DIR . 'empty.zip', self::TMP_TAO_EXPORT_TEST_ZIP); + $this->sut->createZipFile(self::FILENAME, $exportClasses, $exportFiles); } From 9ecdd8ce0f021d754f3bdf0053c69f510c812719 Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Thu, 2 Nov 2023 08:20:27 +0100 Subject: [PATCH 06/10] fix: setup and teardown test dir --- test/unit/model/ZipExporterTest.php | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/test/unit/model/ZipExporterTest.php b/test/unit/model/ZipExporterTest.php index 4dee56dda..664b3c8c8 100644 --- a/test/unit/model/ZipExporterTest.php +++ b/test/unit/model/ZipExporterTest.php @@ -21,8 +21,13 @@ class ZipExporterTest extends TestCase private const FILENAME = 'test'; - private const TMP_TAO_EXPORT_TEST_ZIP = '/tmp/tao_export/test.zip'; - const RESOURCES_DIR = __DIR__ . '/../../resources/'; + private const EXPORT_DIR = '/tmp/tao_export/'; + + private const RESOURCES_DIR = __DIR__ . '/../../resources/'; + + private const TEST_ZIP = 'test.zip'; + + private const EMPTY_ZIP = 'empty.zip'; private StreamInterface $streamMock; @@ -41,6 +46,10 @@ class ZipExporterTest extends TestCase */ public function setUp(): void { + if (!is_dir(self::EXPORT_DIR)) { + mkdir(self::EXPORT_DIR); + } + $this->streamMock = $this->createMock(StreamInterface::class); $this->resourceMock = $this->createMock(core_kernel_classes_Resource::class); $this->fileManagementMock = $this->createMock(FileManagement::class); @@ -68,7 +77,7 @@ public function testCreateZipFile() ] ]; - copy(self::RESOURCES_DIR . 'test.zip', self::TMP_TAO_EXPORT_TEST_ZIP); + copy(self::RESOURCES_DIR . self::TEST_ZIP, self::EXPORT_DIR . self::TEST_ZIP); $this->sut->createZipFile(self::FILENAME, $exportClasses, $exportFiles); @@ -124,15 +133,15 @@ public function testCreateZipFileThrowsMediaReferencesNotFoundException(): void 'Error in Asset class "foo": Media references to Image: foo.jpg FilePath: foo.jpg could not be found.' ); - copy(self::RESOURCES_DIR . 'empty.zip', self::TMP_TAO_EXPORT_TEST_ZIP); + copy(self::RESOURCES_DIR . self::EMPTY_ZIP, self::EXPORT_DIR . self::TEST_ZIP); $this->sut->createZipFile(self::FILENAME, $exportClasses, $exportFiles); } public function tearDown(): void { - if (is_file(self::TMP_TAO_EXPORT_TEST_ZIP)) { - unlink(self::TMP_TAO_EXPORT_TEST_ZIP); + if (is_dir(self::EXPORT_DIR)) { + rmdir(self::EXPORT_DIR); } } } From 3384aa504ade6b97092fcb648e27cb9f9bc494a4 Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Thu, 2 Nov 2023 08:37:21 +0100 Subject: [PATCH 07/10] fix: teardown test files --- test/unit/model/ZipExporterTest.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/unit/model/ZipExporterTest.php b/test/unit/model/ZipExporterTest.php index 664b3c8c8..528906175 100644 --- a/test/unit/model/ZipExporterTest.php +++ b/test/unit/model/ZipExporterTest.php @@ -13,7 +13,6 @@ use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\TestCase; use Psr\Http\Message\StreamInterface; -use ZipArchive; class ZipExporterTest extends TestCase { @@ -140,6 +139,14 @@ public function testCreateZipFileThrowsMediaReferencesNotFoundException(): void public function tearDown(): void { + if (is_file(self::EXPORT_DIR . self::TEST_ZIP)) { + unlink(self::EXPORT_DIR . self::TEST_ZIP); + } + + if (is_file(self::EXPORT_DIR . self::EMPTY_ZIP)) { + unlink(self::EXPORT_DIR . self::EMPTY_ZIP); + } + if (is_dir(self::EXPORT_DIR)) { rmdir(self::EXPORT_DIR); } From 1781821d4b5dc91693dd6b3d523b3c48a5af4ff5 Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Sun, 5 Nov 2023 10:33:51 +0100 Subject: [PATCH 08/10] feature: update composer dependencies, add logging of zip export errors. --- composer.json | 2 +- model/ZipExporter.php | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index a125dc2e2..2a62e1aaa 100755 --- a/composer.json +++ b/composer.json @@ -27,7 +27,7 @@ "oat-sa/oatbox-extension-installer": "~1.1||dev-master", "oat-sa/lib-generis-search": "2.1.2", "oat-sa/generis": ">=15.24", - "oat-sa/tao-core": ">=52.1", + "oat-sa/tao-core": ">=53.12", "oat-sa/extension-tao-item": ">=v12.1.0", "oat-sa/extension-tao-itemqti": ">=29.14.5", "oat-sa/extension-tao-test": ">=15.16", diff --git a/model/ZipExporter.php b/model/ZipExporter.php index 8ed99c124..2b53fc8c3 100644 --- a/model/ZipExporter.php +++ b/model/ZipExporter.php @@ -32,6 +32,7 @@ use core_kernel_classes_Property; use core_kernel_classes_Resource; use Exception; +use oat\oatbox\log\LoggerAwareTrait; use oat\oatbox\service\ServiceManager; use oat\taoMediaManager\model\export\service\MediaResourcePreparerInterface; use oat\taoMediaManager\model\export\service\SharedStimulusCSSExporter; @@ -49,6 +50,8 @@ */ class ZipExporter implements tao_models_classes_export_ExportHandler { + use LoggerAwareTrait; + /** * @inheritDoc */ @@ -199,6 +202,8 @@ protected function createZipFile($filename, array $exportClasses = [], array $ex return $path; } catch (Exception $e) { + $this->getLogger()->error($e->getMessage() . $e->getTraceAsString()); + $zip->close(); if (is_file($path)) { From 385df631cdca77adece14d0c7e14bb50db124a46 Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Sun, 5 Nov 2023 10:50:14 +0100 Subject: [PATCH 09/10] feature: Added logging of zip export errors with tests. --- test/unit/model/ZipExporterTest.php | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/unit/model/ZipExporterTest.php b/test/unit/model/ZipExporterTest.php index 528906175..08fbaa95c 100644 --- a/test/unit/model/ZipExporterTest.php +++ b/test/unit/model/ZipExporterTest.php @@ -13,6 +13,7 @@ use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\TestCase; use Psr\Http\Message\StreamInterface; +use Psr\Log\LoggerInterface; class ZipExporterTest extends TestCase { @@ -38,6 +39,8 @@ class ZipExporterTest extends TestCase private ServiceManager $serviceManagerMock; + private LoggerInterface $loggerMock; + private ZipExporter $sut; /** @@ -54,10 +57,11 @@ public function setUp(): void $this->fileManagementMock = $this->createMock(FileManagement::class); $this->mediaResourcePreparerMock = $this->createMock(MediaResourcePreparerInterface::class); $this->serviceManagerMock = $this->createMock(ServiceManager::class); + $this->loggerMock = $this->createMock(LoggerInterface::class); $this->sut = $this ->getMockBuilder(ZipExporterTester::class) - ->onlyMethods(['getServiceManager']) + ->onlyMethods(['getServiceManager', 'getLogger']) ->getMock(); } @@ -108,6 +112,12 @@ public function testCreateZipFileThrowsMediaReferencesNotFoundException(): void ->method('getServiceManager') ->willReturn($this->serviceManagerMock); + $this + ->sut + ->expects(self::once()) + ->method('getLogger') + ->willReturn($this->loggerMock); + $this ->fileManagementMock ->expects(self::once()) @@ -126,6 +136,11 @@ public function testCreateZipFileThrowsMediaReferencesNotFoundException(): void ->method('get') ->willReturnOnConsecutiveCalls($this->fileManagementMock, $this->mediaResourcePreparerMock); + $this + ->loggerMock + ->expects(self::once()) + ->method('error'); + $this->expectException(ZipExporterFileErrorList::class); $this->expectExceptionMessage( 'Errors in zip file:
' . From e7ea1b99db9d112c464d046d8ebdadd4d3535405 Mon Sep 17 00:00:00 2001 From: Janos Pribelszki Date: Mon, 6 Nov 2023 18:23:58 +0100 Subject: [PATCH 10/10] feature: Added logging of zip export create errors. --- model/ZipExporter.php | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/model/ZipExporter.php b/model/ZipExporter.php index 2b53fc8c3..7f9a5cbeb 100644 --- a/model/ZipExporter.php +++ b/model/ZipExporter.php @@ -148,22 +148,23 @@ private function getSavePath(string $unsafePath): string */ protected function createZipFile($filename, array $exportClasses = [], array $exportFiles = []): string { - $baseDir = tao_helpers_Export::getExportPath(); - $path = $baseDir . '/' . $filename . '.zip'; + try { + $errors = []; - $zip = new ZipArchive(); - if ($zip->open($path, ZipArchive::CREATE) !== true) { - throw new common_Exception('Unable to create zipfile ' . $path); - } + $baseDir = tao_helpers_Export::getExportPath(); + $path = $baseDir . '/' . $filename . '.zip'; - if ($zip->numFiles !== 0) { - $zip->close(); + $zip = new ZipArchive(); + if ($zip->open($path, ZipArchive::CREATE) !== true) { + throw new common_Exception('Unable to create zipfile ' . $path); + } - return $path; - } + if ($zip->numFiles !== 0) { + $zip->close(); + + return $path; + } - try { - $errors = []; foreach ($exportFiles as $label => $files) { $archivePath = '';