From e280521f26ab4ba83fe4a3b3e5d04475ead91b16 Mon Sep 17 00:00:00 2001 From: Andreas Berqvist Date: Fri, 3 Jul 2026 00:13:34 +0200 Subject: [PATCH 1/6] Error handling and performance optimization --- src/Backuper.php | 53 ++++++++++++++++++++ src/Support/Zipper.php | 100 ++++++++++++++++++++++++++++++++++---- tests/Unit/ZipperTest.php | 55 +++++++++++++++++++++ 3 files changed, 199 insertions(+), 9 deletions(-) diff --git a/src/Backuper.php b/src/Backuper.php index aed6006..5a2d439 100644 --- a/src/Backuper.php +++ b/src/Backuper.php @@ -7,6 +7,7 @@ use Illuminate\Contracts\Auth\Authenticatable; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\File; +use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Pipeline; use Itiden\Backup\Contracts\Repositories\BackupRepository; use Itiden\Backup\DataTransferObjects\BackupDto; @@ -15,6 +16,7 @@ use Itiden\Backup\Events\BackupFailed; use Itiden\Backup\Models\Metadata; use Itiden\Backup\Support\Zipper; +use RuntimeException; use Throwable; use function Illuminate\Filesystem\join_paths; @@ -33,12 +35,41 @@ public function __construct( */ public function backup(?Authenticatable $user = null): BackupDto { + if (function_exists('set_time_limit')) { + set_time_limit(0); + } + + ignore_user_abort(true); + $lock = $this->stateManager->getLock(); + $temp_zip_path = null; + try { $this->stateManager->setState(State::BackupInProgress); $temp_zip_path = join_paths(Config::string('backup.temp_path'), 'temp.zip'); + $completed = false; + + register_shutdown_function(static function () use (&$completed, $temp_zip_path): void { + if ($completed) { + return; + } + + Log::error('backup: process killed mid-backup', [ + 'temp_zip_exists' => File::exists($temp_zip_path), + ]); + + if (File::exists($temp_zip_path)) { + File::delete($temp_zip_path); + } + + app(StateManager::class)->setState(State::BackupFailed); + }); + + Log::info('backup: started', [ + 'user' => $user?->getAuthIdentifier(), + ]); $zipper = Zipper::write($temp_zip_path); @@ -57,6 +88,18 @@ public function backup(?Authenticatable $user = null): BackupDto $zipper->close(); + Log::info('backup: zip closed', [ + 'size' => File::size($temp_zip_path), + ]); + + if (!Zipper::verify($temp_zip_path)) { + File::delete($temp_zip_path); + + throw new RuntimeException('Zip verification failed — the backup archive is invalid.'); + } + + Log::info('backup: zip verified'); + $backup = $this->repository->add($temp_zip_path); $metadata = static::addMetaFromZipToBackupMeta($temp_zip_path, $backup); @@ -73,8 +116,18 @@ public function backup(?Authenticatable $user = null): BackupDto $this->stateManager->setState(State::BackupCompleted); + Log::info('backup: completed', ['path' => $backup->path]); + + $completed = true; + return $backup; } catch (Throwable $e) { + if ($temp_zip_path !== null && File::exists($temp_zip_path)) { + File::delete($temp_zip_path); + } + + Log::error('backup: failed', ['error' => $e->getMessage()]); + $exception = new Exceptions\BackupFailed(previous: $e); event(new BackupFailed($exception)); diff --git a/src/Support/Zipper.php b/src/Support/Zipper.php index f4bfd86..7dea381 100644 --- a/src/Support/Zipper.php +++ b/src/Support/Zipper.php @@ -6,23 +6,43 @@ use Illuminate\Support\Collection; use Illuminate\Support\Facades\File; +use Illuminate\Support\Facades\Log; +use RuntimeException; use SensitiveParameter; -use Symfony\Component\Finder\SplFileInfo; +use Symfony\Component\Finder\Finder; use ZipArchive; // @mago-expect lint:too-many-methods final class Zipper { + /** + * File extensions that are already compressed and should be stored + * without re-compression to save CPU cycles and I/O bandwidth. + */ + private const STORED_EXTENSIONS = [ + 'zip', 'mp4', 'webm', 'png', 'jpg', 'jpeg', 'webp', 'gif', 'pdf', + 'mp3', 'wav', 'mov', 'avi', 'ogg', 'gz', 'tar', 'tgz', + 'woff', 'woff2', 'ttf', 'otf', + 'ico', 'avif', 'heic', + 'bz2', 'xz', '7z', 'rar', + ]; + private ZipArchive $zip; private array $meta = []; + private string $path; public function __construct(string $path, int $flags = ZipArchive::CREATE | ZipArchive::OVERWRITE) { File::ensureDirectoryExists(dirname($path)); + $this->path = $path; $this->zip = new ZipArchive(); - $this->zip->open($path, $flags); + $result = $this->zip->open($path, $flags); + + if ($result !== true) { + throw new RuntimeException("Failed to open zip [{$path}] (error code: {$result})"); + } } /** @@ -38,12 +58,36 @@ public static function read(string $path): self return new static($path, ZipArchive::RDONLY); } + /** + * Verify that a zip file at the given path is a valid archive. + */ + public static function verify(string $path): bool + { + $zip = new ZipArchive(); + + if ($zip->open($path, ZipArchive::RDONLY) !== true) { + return false; + } + + $valid = $zip->numFiles > 0; + + $zip->close(); + + return $valid; + } + /** * Close the Zipper and write the archive to the filesystem. */ public function close(): void { - $this->zip->close(); + if (!$this->zip->close()) { + Log::error('zipper: close failed', ['path' => $this->path]); + + throw new RuntimeException( + "Failed to write zip archive [{$this->path}] — check disk space and memory limits.", + ); + } } /** @@ -53,18 +97,36 @@ public function encrypt(#[SensitiveParameter] string $password): self { $this->zip->setPassword($password); - collect(range(0, $this->zip->numFiles - 1)) - ->each(fn(int $file): bool => $this->zip->setEncryptionIndex($file, ZipArchive::EM_AES_256)); + for ($i = 0; $i < $this->zip->numFiles; $i++) { + if (!$this->zip->setEncryptionIndex($i, ZipArchive::EM_AES_256)) { + throw new RuntimeException("Failed to set encryption for file at index {$i}"); + } + } return $this; } /** * Add a file to the archive. + * + * Pre-compressed file types (images, videos, archives) are stored + * without re-compression using CM_STORE for maximum I/O performance. + * Text-based files use CM_DEFLATE for size reduction. */ public function addFile(string $path, ?string $name = null): self { - $this->zip->addFile($path, $name ?? basename($path)); + $entryName = $name ?? basename($path); + + if (!$this->zip->addFile($path, $entryName)) { + throw new RuntimeException("Failed to add file to zip: {$path}"); + } + + $extension = strtolower(pathinfo($entryName, PATHINFO_EXTENSION)); + $method = in_array($extension, self::STORED_EXTENSIONS, true) + ? ZipArchive::CM_STORE + : ZipArchive::CM_DEFLATE; + + $this->zip->setCompressionName($entryName, $method); return $this; } @@ -74,7 +136,9 @@ public function addFile(string $path, ?string $name = null): self */ public function addFromString(string $name, string $content): self { - $this->zip->addFromString($name, $content); + if (!$this->zip->addFromString($name, $content)) { + throw new RuntimeException("Failed to add content to zip: {$name}"); + } return $this; } @@ -84,9 +148,27 @@ public function addFromString(string $name, string $content): self */ public function addDirectory(string $path, ?string $prefix = null): self { - collect(File::allFiles($path))->each(function (SplFileInfo $file) use ($prefix): void { + $finder = (new Finder())->files()->ignoreDotFiles(false)->in($path); + + $count = 0; + + foreach ($finder as $file) { $this->addFile($file->getPathname(), $prefix . '/' . $file->getRelativePathname()); - }); + + $count++; + + if ($count % 500 === 0) { + Log::info('zipper: addDirectory progress', [ + 'directory' => $path, + 'files_added' => $count, + ]); + } + } + + Log::info('zipper: addDirectory complete', [ + 'directory' => $path, + 'total_files' => $count, + ]); return $this; } diff --git a/tests/Unit/ZipperTest.php b/tests/Unit/ZipperTest.php index f69fa27..3e923bb 100644 --- a/tests/Unit/ZipperTest.php +++ b/tests/Unit/ZipperTest.php @@ -124,4 +124,59 @@ $zip->close(); }); + + it('uses CM_STORE for pre-compressed media extensions', function (): void { + $target = storage_path('test.zip'); + $source = storage_path('test_media.png'); + + File::put($source, str_repeat('a', 10_000)); + + Zipper::write($target)->addFile($source, 'test.png')->close(); + + $archive = new ZipArchive(); + $archive->open($target); + $stat = $archive->statName('test.png'); + + expect($stat['comp_size'])->toBe($stat['size']); + + $archive->close(); + }); + + it('uses CM_DEFLATE for text files', function (): void { + $target = storage_path('test.zip'); + $source = storage_path('test_text.txt'); + + File::put($source, str_repeat('a', 10_000)); + + Zipper::write($target)->addFile($source, 'test.txt')->close(); + + $archive = new ZipArchive(); + $archive->open($target); + $stat = $archive->statName('test.txt'); + + expect($stat['comp_size'])->toBeLessThan($stat['size']); + + $archive->close(); + }); + + it('can verify a valid zip', function (): void { + $target = storage_path('test.zip'); + + Zipper::write($target)->addFromString('test.txt', 'test')->close(); + + expect(Zipper::verify($target))->toBeTrue(); + }); + + it('returns false when verifying an invalid zip', function (): void { + $target = storage_path('invalid.zip'); + + File::put($target, 'not a zip file'); + + expect(Zipper::verify($target))->toBeFalse(); + }); + + it('throws when opening a non-existent zip for reading', function (): void { + expect(fn() => Zipper::read(storage_path('nonexistent.zip'))) + ->toThrow(RuntimeException::class); + }); })->group('zipper'); From 968720adf77ebe7487e6dc4fc49e6496030f94d7 Mon Sep 17 00:00:00 2001 From: Andreas Berqvist Date: Fri, 3 Jul 2026 00:19:05 +0200 Subject: [PATCH 2/6] Fix format --- src/Exceptions/BackupFailed.php | 6 +-- src/Http/Controllers/Api/BackupController.php | 42 +++++++++--------- src/Support/Zipper.php | 44 ++++++++++++++----- tests/Feature/RestoreCommandTest.php | 6 +-- tests/Feature/ViewBackupsTest.php | 38 +++++++++------- tests/Unit/ZipperTest.php | 3 +- 6 files changed, 84 insertions(+), 55 deletions(-) diff --git a/src/Exceptions/BackupFailed.php b/src/Exceptions/BackupFailed.php index e3534a9..13aea58 100644 --- a/src/Exceptions/BackupFailed.php +++ b/src/Exceptions/BackupFailed.php @@ -12,8 +12,8 @@ final class BackupFailed extends Exception { public function __construct(Throwable $previous) { - parent::__construct(__('statamic-backup::backup.failed', ['date' => Carbon::now()->format( - 'Ymd', - )]), previous: $previous); + parent::__construct(__('statamic-backup::backup.failed', [ + 'date' => Carbon::now()->format('Ymd'), + ]), previous: $previous); } } diff --git a/src/Http/Controllers/Api/BackupController.php b/src/Http/Controllers/Api/BackupController.php index b508671..71eea56 100644 --- a/src/Http/Controllers/Api/BackupController.php +++ b/src/Http/Controllers/Api/BackupController.php @@ -14,27 +14,29 @@ public function __invoke(BackupRepository $repo): AnonymousResourceCollection { $backups = $repo->all(); - return BackupResource::collection($backups)->additional(['meta' => [ - // Required by statamic to render the table - 'columns' => [ - [ - 'label' => 'Name', - 'field' => 'name', - 'visible' => true, - ], - [ - 'label' => 'Created at', - 'field' => 'created_at', - 'visible' => true, - 'sortable' => true, - ], - [ - 'label' => 'Size', - 'field' => 'size', - 'visible' => true, - 'sortable' => true, + return BackupResource::collection($backups)->additional([ + 'meta' => [ + // Required by statamic to render the table + 'columns' => [ + [ + 'label' => 'Name', + 'field' => 'name', + 'visible' => true, + ], + [ + 'label' => 'Created at', + 'field' => 'created_at', + 'visible' => true, + 'sortable' => true, + ], + [ + 'label' => 'Size', + 'field' => 'size', + 'visible' => true, + 'sortable' => true, + ], ], ], - ]]); + ]); } } diff --git a/src/Support/Zipper.php b/src/Support/Zipper.php index 7dea381..c0e44e5 100644 --- a/src/Support/Zipper.php +++ b/src/Support/Zipper.php @@ -20,11 +20,34 @@ final class Zipper * without re-compression to save CPU cycles and I/O bandwidth. */ private const STORED_EXTENSIONS = [ - 'zip', 'mp4', 'webm', 'png', 'jpg', 'jpeg', 'webp', 'gif', 'pdf', - 'mp3', 'wav', 'mov', 'avi', 'ogg', 'gz', 'tar', 'tgz', - 'woff', 'woff2', 'ttf', 'otf', - 'ico', 'avif', 'heic', - 'bz2', 'xz', '7z', 'rar', + 'zip', + 'mp4', + 'webm', + 'png', + 'jpg', + 'jpeg', + 'webp', + 'gif', + 'pdf', + 'mp3', + 'wav', + 'mov', + 'avi', + 'ogg', + 'gz', + 'tar', + 'tgz', + 'woff', + 'woff2', + 'ttf', + 'otf', + 'ico', + 'avif', + 'heic', + 'bz2', + 'xz', + '7z', + 'rar', ]; private ZipArchive $zip; @@ -122,9 +145,7 @@ public function addFile(string $path, ?string $name = null): self } $extension = strtolower(pathinfo($entryName, PATHINFO_EXTENSION)); - $method = in_array($extension, self::STORED_EXTENSIONS, true) - ? ZipArchive::CM_STORE - : ZipArchive::CM_DEFLATE; + $method = in_array($extension, self::STORED_EXTENSIONS, true) ? ZipArchive::CM_STORE : ZipArchive::CM_DEFLATE; $this->zip->setCompressionName($entryName, $method); @@ -148,7 +169,10 @@ public function addFromString(string $name, string $content): self */ public function addDirectory(string $path, ?string $prefix = null): self { - $finder = (new Finder())->files()->ignoreDotFiles(false)->in($path); + $finder = new Finder() + ->files() + ->ignoreDotFiles(false) + ->in($path); $count = 0; @@ -157,7 +181,7 @@ public function addDirectory(string $path, ?string $prefix = null): self $count++; - if ($count % 500 === 0) { + if (($count % 500) === 0) { Log::info('zipper: addDirectory progress', [ 'directory' => $path, 'files_added' => $count, diff --git a/tests/Feature/RestoreCommandTest.php b/tests/Feature/RestoreCommandTest.php index 6fd2976..5d434ac 100644 --- a/tests/Feature/RestoreCommandTest.php +++ b/tests/Feature/RestoreCommandTest.php @@ -27,9 +27,9 @@ $backup = Backuper::backup(); - artisan('statamic:backup:restore', ['--path' => Storage::disk(config( - 'backup.destination.disk', - ))->path($backup->path)]) + artisan('statamic:backup:restore', [ + '--path' => Storage::disk(config('backup.destination.disk'))->path($backup->path), + ]) ->expectsConfirmation('Are you sure you want to restore your content?') ->assertFailed(); }); diff --git a/tests/Feature/ViewBackupsTest.php b/tests/Feature/ViewBackupsTest.php index fb184e4..e55ec9d 100644 --- a/tests/Feature/ViewBackupsTest.php +++ b/tests/Feature/ViewBackupsTest.php @@ -70,24 +70,28 @@ getJson(cp_route('api.itiden.backup.index')) ->assertOk() ->assertJsonStructure([ - 'data' => ['*' => [ - 'name', - 'size', - 'path', - 'created_at', - 'id', - 'metadata' => [ - 'created_by', - 'downloads', - 'restores', - 'skipped_pipes', + 'data' => [ + '*' => [ + 'name', + 'size', + 'path', + 'created_at', + 'id', + 'metadata' => [ + 'created_by', + 'downloads', + 'restores', + 'skipped_pipes', + ], ], - ]], - 'meta' => ['columns' => ['*' => [ - 'label', - 'field', - 'visible', - ]]], + ], + 'meta' => [ + 'columns' => ['*' => [ + 'label', + 'field', + 'visible', + ]], + ], ]); }); })->group('view'); diff --git a/tests/Unit/ZipperTest.php b/tests/Unit/ZipperTest.php index 3e923bb..f425d36 100644 --- a/tests/Unit/ZipperTest.php +++ b/tests/Unit/ZipperTest.php @@ -176,7 +176,6 @@ }); it('throws when opening a non-existent zip for reading', function (): void { - expect(fn() => Zipper::read(storage_path('nonexistent.zip'))) - ->toThrow(RuntimeException::class); + expect(fn() => Zipper::read(storage_path('nonexistent.zip')))->toThrow(RuntimeException::class); }); })->group('zipper'); From 4c61509ddadda78c70345a7f43377b5e0d0b6819 Mon Sep 17 00:00:00 2001 From: Andreas Bergqvist Date: Fri, 3 Jul 2026 00:22:58 +0200 Subject: [PATCH 3/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/Backuper.php | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/src/Backuper.php b/src/Backuper.php index 5a2d439..23a6093 100644 --- a/src/Backuper.php +++ b/src/Backuper.php @@ -51,21 +51,32 @@ public function backup(?Authenticatable $user = null): BackupDto $temp_zip_path = join_paths(Config::string('backup.temp_path'), 'temp.zip'); $completed = false; - register_shutdown_function(static function () use (&$completed, $temp_zip_path): void { - if ($completed) { - return; - } +register_shutdown_function(static function () use (&$completed, $temp_zip_path): void { + if ($completed) { + return; + } + + $error = error_get_last(); + + // Only treat true fatal errors as a "killed mid-backup" scenario. + if ($error === null || !in_array($error['type'], [E_ERROR, E_PARSE, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR], true)) { + return; + } + + Log::error('backup: fatal error mid-backup', [ + 'error' => $error, + 'temp_zip_exists' => File::exists($temp_zip_path), + ]); - Log::error('backup: process killed mid-backup', [ - 'temp_zip_exists' => File::exists($temp_zip_path), - ]); + if (File::exists($temp_zip_path)) { + File::delete($temp_zip_path); + } - if (File::exists($temp_zip_path)) { - File::delete($temp_zip_path); - } + // Ensure the lock doesn't remain held indefinitely after a fatal error. + \Illuminate\Support\Facades\Cache::lock(StateManager::LOCK)->forceRelease(); - app(StateManager::class)->setState(State::BackupFailed); - }); + app(StateManager::class)->setState(State::BackupFailed); +}); Log::info('backup: started', [ 'user' => $user?->getAuthIdentifier(), From b0c83eecf89207b3bdd77cadfe9ff6eb9216d77f Mon Sep 17 00:00:00 2001 From: Andreas Berqvist Date: Fri, 3 Jul 2026 00:24:31 +0200 Subject: [PATCH 4/6] Fix format --- src/Backuper.php | 55 +++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 26 deletions(-) diff --git a/src/Backuper.php b/src/Backuper.php index 23a6093..acb9829 100644 --- a/src/Backuper.php +++ b/src/Backuper.php @@ -51,32 +51,35 @@ public function backup(?Authenticatable $user = null): BackupDto $temp_zip_path = join_paths(Config::string('backup.temp_path'), 'temp.zip'); $completed = false; -register_shutdown_function(static function () use (&$completed, $temp_zip_path): void { - if ($completed) { - return; - } - - $error = error_get_last(); - - // Only treat true fatal errors as a "killed mid-backup" scenario. - if ($error === null || !in_array($error['type'], [E_ERROR, E_PARSE, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR], true)) { - return; - } - - Log::error('backup: fatal error mid-backup', [ - 'error' => $error, - 'temp_zip_exists' => File::exists($temp_zip_path), - ]); - - if (File::exists($temp_zip_path)) { - File::delete($temp_zip_path); - } - - // Ensure the lock doesn't remain held indefinitely after a fatal error. - \Illuminate\Support\Facades\Cache::lock(StateManager::LOCK)->forceRelease(); - - app(StateManager::class)->setState(State::BackupFailed); -}); + register_shutdown_function(static function () use (&$completed, $temp_zip_path): void { + if ($completed) { + return; + } + + $error = error_get_last(); + + // Only treat true fatal errors as a "killed mid-backup" scenario. + if ( + $error === null + || !in_array($error['type'], [E_ERROR, E_PARSE, E_CORE_ERROR, E_COMPILE_ERROR, E_USER_ERROR], true) + ) { + return; + } + + Log::error('backup: fatal error mid-backup', [ + 'error' => $error, + 'temp_zip_exists' => File::exists($temp_zip_path), + ]); + + if (File::exists($temp_zip_path)) { + File::delete($temp_zip_path); + } + + // Ensure the lock doesn't remain held indefinitely after a fatal error. + \Illuminate\Support\Facades\Cache::lock(StateManager::LOCK)->forceRelease(); + + app(StateManager::class)->setState(State::BackupFailed); + }); Log::info('backup: started', [ 'user' => $user?->getAuthIdentifier(), From f89220c85d4118b9ce22fc41321589936c4bafbb Mon Sep 17 00:00:00 2001 From: Andreas Berqvist Date: Fri, 3 Jul 2026 00:31:43 +0200 Subject: [PATCH 5/6] Fix parse error --- src/Support/Zipper.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Support/Zipper.php b/src/Support/Zipper.php index c0e44e5..c2cf03a 100644 --- a/src/Support/Zipper.php +++ b/src/Support/Zipper.php @@ -169,10 +169,8 @@ public function addFromString(string $name, string $content): self */ public function addDirectory(string $path, ?string $prefix = null): self { - $finder = new Finder() - ->files() - ->ignoreDotFiles(false) - ->in($path); + $finder = new Finder(); + $finder->files()->ignoreDotFiles(false)->in($path); $count = 0; From c5e73835fabf19e6f189d88fb869b7f6eaf2710c Mon Sep 17 00:00:00 2001 From: Andreas Berqvist Date: Fri, 3 Jul 2026 20:24:00 +0200 Subject: [PATCH 6/6] Fix download --- .../Controllers/DownloadBackupController.php | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/Http/Controllers/DownloadBackupController.php b/src/Http/Controllers/DownloadBackupController.php index ba719f0..97d31ba 100644 --- a/src/Http/Controllers/DownloadBackupController.php +++ b/src/Http/Controllers/DownloadBackupController.php @@ -9,11 +9,11 @@ use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Storage; use Itiden\Backup\Contracts\Repositories\BackupRepository; -use Symfony\Component\HttpFoundation\StreamedResponse; +use Symfony\Component\HttpFoundation\Response; final readonly class DownloadBackupController { - public function __invoke(Request $request, string $id, BackupRepository $repo): StreamedResponse + public function __invoke(Request $request, string $id, BackupRepository $repo): Response { $backup = $repo->find($id); @@ -26,6 +26,22 @@ public function __invoke(Request $request, string $id, BackupRepository $repo): $backup->getMetadata()->addDownload($user); - return Storage::disk(Config::string('backup.destination.disk'))->download($backup->path); + if (function_exists('set_time_limit')) { + set_time_limit(0); + } + + // Clean and close all active output buffers to allow streaming without running out of memory + while (ob_get_level() > 0) { + ob_end_clean(); + } + + $disk = Storage::disk(Config::string('backup.destination.disk')); + + try { + $path = $disk->path($backup->path); + return response()->download($path); + } catch (\Throwable) { + return $disk->download($backup->path); + } } }