From 7e15e926626d2f2a579b1fe9fb7cec969c9d9e48 Mon Sep 17 00:00:00 2001 From: Daniel Jakob Date: Wed, 17 Apr 2024 03:31:24 +0200 Subject: [PATCH] Re-arrange some code in the ZipHandler and Batch action (#3931) Also correct some type issues --- phpstan-baseline.neon | 20 ---- src/Module/Api/Xml_Data.php | 5 +- .../Application/Batch/DefaultAction.php | 108 ++++++++---------- src/Module/Util/ZipHandler.php | 62 ++++++---- src/Module/Util/ZipHandlerInterface.php | 4 +- src/Repository/Model/Playlist.php | 7 +- src/Repository/Model/Search.php | 7 +- src/Repository/Model/playlist_object.php | 4 +- 8 files changed, 103 insertions(+), 114 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index c2d3d074fb..e971e80080 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -4845,16 +4845,6 @@ parameters: count: 1 path: src/Module/Application/Batch/DefaultAction.php - - - message: "#^Method Ampache\\\\Module\\\\Application\\\\Batch\\\\DefaultAction\\:\\:getMediaFiles\\(\\) has parameter \\$media_ids with no value type specified in iterable type array\\.$#" - count: 1 - path: src/Module/Application/Batch/DefaultAction.php - - - - message: "#^Method Ampache\\\\Module\\\\Application\\\\Batch\\\\DefaultAction\\:\\:getMediaFiles\\(\\) return type has no value type specified in iterable type array\\.$#" - count: 1 - path: src/Module/Application/Batch/DefaultAction.php - - message: "#^Comparison operation \"\\=\\=\" between \\(array\\\\|string\\) and int\\|string results in an error\\.$#" count: 1 @@ -11425,16 +11415,6 @@ parameters: count: 1 path: src/Module/Util/Waveform.php - - - message: "#^Method Ampache\\\\Module\\\\Util\\\\ZipHandler\\:\\:zip\\(\\) has parameter \\$media_files with no value type specified in iterable type array\\.$#" - count: 1 - path: src/Module/Util/ZipHandler.php - - - - message: "#^Method Ampache\\\\Module\\\\Util\\\\ZipHandlerInterface\\:\\:zip\\(\\) has parameter \\$media_files with no value type specified in iterable type array\\.$#" - count: 1 - path: src/Module/Util/ZipHandlerInterface.php - - message: "#^Access to an undefined property Ampache\\\\Repository\\\\Model\\\\Media\\:\\:\\$catalog\\.$#" count: 1 diff --git a/src/Module/Api/Xml_Data.php b/src/Module/Api/Xml_Data.php index 2558d7ba76..b8009c4def 100644 --- a/src/Module/Api/Xml_Data.php +++ b/src/Module/Api/Xml_Data.php @@ -42,6 +42,7 @@ use Ampache\Repository\Model\Catalog; use Ampache\Repository\Model\Democratic; use Ampache\Repository\Model\library_item; +use Ampache\Repository\Model\LibraryItemEnum; use Ampache\Repository\Model\Live_Stream; use Ampache\Repository\Model\Metadata; use Ampache\Repository\Model\Playlist; @@ -558,7 +559,7 @@ public static function indexes($objects, $object_type, $user, $full_xml = true, $songs = ($include) ? $playlist->get_items() : array(); $string .= "<$object_type id=\"" . $object_id . "\">\n\t\n\t" . (int)$playitem_total . "\n\t\n\ttype . "]]>\n"; foreach ($songs as $song_id) { - if ($song_id['object_type'] == 'song') { + if ($song_id['object_type'] === LibraryItemEnum::SONG) { $string .= "\t\t" . $song_id['track'] . "\n"; } } @@ -1032,7 +1033,7 @@ public static function playlists($playlists, $user, $songs = false): string $items = ''; $playlisttracks = $playlist->get_items(); foreach ($playlisttracks as $objects) { - if ($objects['object_type'] === 'song') { + if ($objects['object_type'] === LibraryItemEnum::SONG) { $items .= "\t\t" . $objects['track'] . "\n"; } } diff --git a/src/Module/Application/Batch/DefaultAction.php b/src/Module/Application/Batch/DefaultAction.php index 683ad39824..b7953e9590 100644 --- a/src/Module/Application/Batch/DefaultAction.php +++ b/src/Module/Application/Batch/DefaultAction.php @@ -28,7 +28,10 @@ use Ampache\Module\Authorization\AccessFunctionEnum; use Ampache\Module\Util\RequestParserInterface; use Ampache\Repository\Model\library_item; +use Ampache\Repository\Model\LibraryItemEnum; +use Ampache\Repository\Model\LibraryItemLoaderInterface; use Ampache\Repository\Model\ModelFactoryInterface; +use Ampache\Repository\Model\playable_item; use Ampache\Repository\Model\User; use Ampache\Module\Application\ApplicationActionInterface; use Ampache\Module\Application\Exception\AccessDeniedException; @@ -36,7 +39,6 @@ use Ampache\Module\Authorization\GuiGatekeeperInterface; use Ampache\Module\System\Core; use Ampache\Module\System\LegacyLogger; -use Ampache\Module\Util\InterfaceImplementationChecker; use Ampache\Module\Util\ObjectTypeToClassNameMapper; use Ampache\Module\Util\ZipHandlerInterface; use Ampache\Repository\SongRepositoryInterface; @@ -57,12 +59,12 @@ public function __construct( private FunctionCheckerInterface $functionChecker, private SongRepositoryInterface $songRepository, private ResponseFactoryInterface $responseFactory, + private LibraryItemLoaderInterface $libraryItemLoader, ) { } public function run(ServerRequestInterface $request, GuiGatekeeperInterface $gatekeeper): ?ResponseInterface { - ob_end_clean(); if ( !defined('NO_SESSION') && !$this->functionChecker->check(AccessFunctionEnum::FUNCTION_BATCH_DOWNLOAD) @@ -70,15 +72,12 @@ public function run(ServerRequestInterface $request, GuiGatekeeperInterface $gat throw new AccessDeniedException(); } - /* Drop the normal Time limit constraints, this can take a while */ - set_time_limit(0); - $media_ids = []; $default_name = 'Unknown'; $name = $default_name; $action = $this->requestParser->getFromRequest('action'); $flat_path = (in_array($action, array('browse', 'playlist', 'tmp_playlist'))); - $object_type = ($action == 'browse') + $object_type = $action === 'browse' ? $this->requestParser->getFromRequest('type') : $action; @@ -90,28 +89,29 @@ public function run(ServerRequestInterface $request, GuiGatekeeperInterface $gat throw new AccessDeniedException(); } - if (InterfaceImplementationChecker::is_playable_item($object_type)) { - $object_id = (int)$this->requestParser->getFromRequest('id'); - $this->logger->debug( - 'Requested item ' . $object_id, - [LegacyLogger::CONTEXT_TYPE => __CLASS__] - ); - $className = ObjectTypeToClassNameMapper::map($object_type); - /** @var class-string $className */ - $libitem = new $className($object_id); - if ($libitem->isNew() === false) { - if (method_exists($libitem, 'format')) { - $libitem->format(); - } - $name = (string)$libitem->get_fullname(); - $media_ids = array_merge($media_ids, $libitem->get_medias()); + $object_id = (int)$this->requestParser->getFromRequest('id'); + $this->logger->debug( + 'Requested item ' . $object_id, + [LegacyLogger::CONTEXT_TYPE => __CLASS__] + ); + + $libItem = $this->libraryItemLoader->load( + LibraryItemEnum::from($object_type), + $object_id, + ); + + if ($libItem instanceof playable_item) { + if (method_exists($libItem, 'format')) { + $libItem->format(); } + $name = (string)$libItem->get_fullname(); + $media_ids = array_merge($media_ids, $libItem->get_medias()); } else { // Switch on the actions switch ($action) { case 'tmp_playlist': - $user = Core::get_global('user'); + $user = $gatekeeper->getUser(); if ($user instanceof User) { $user->load_playlist(); $media_ids = $user->playlist?->get_items() ?? []; @@ -146,7 +146,7 @@ public function run(ServerRequestInterface $request, GuiGatekeeperInterface $gat $video = $this->modelFactory->createVideo($media_id); if ($video->isNew() === false) { $media_ids[] = [ - 'object_type' => 'Video', + 'object_type' => LibraryItemEnum::VIDEO, 'object_id' => $media_id ]; } @@ -166,57 +166,41 @@ public function run(ServerRequestInterface $request, GuiGatekeeperInterface $gat throw new AccessDeniedException(); } - // Write/close session data to release session lock for this script. - // This to allow other pages from the same session to be processed - // Do NOT change any session variable after this call - session_write_close(); - - // Take whatever we've got and send the zip - $media_files = $this->getMediaFiles($media_ids); - if (is_array($media_files['0'])) { - set_memory_limit($media_files['1'] + 32); - - return $this->zipHandler->zip( - $this->responseFactory->createResponse(), - $name, - $media_files['0'], - $flat_path - ); - } - - return null; + return $this->zipHandler->zip( + $this->responseFactory->createResponse(), + $name, + $this->getMediaFiles($media_ids), + $flat_path + ); } /** * Takes an array of media ids and returns an array of the actual filenames * - * @param array $media_ids Media IDs. - * @return array + * @param iterable $medias Media IDs. + * @return array{ + * files: array>, + * total_size: int + * } */ - private function getMediaFiles(array $media_ids): array + private function getMediaFiles(iterable $medias): array { $media_files = []; $total_size = 0; - foreach ($media_ids as $element) { + foreach ($medias as $element) { if (is_array($element)) { - if (isset($element['object_type'])) { - $type = $element['object_type']->value; - $mediaid = $element['object_id']; - } else { - $type = array_shift($element); - $mediaid = array_shift($element); - } - $className = ObjectTypeToClassNameMapper::map($type); - /** @var class-string $className */ - $media = new $className($mediaid); + $media = $this->libraryItemLoader->load( + $element['object_type'], + $element['object_id'] + ); } else { $media = $this->modelFactory->createSong((int) $element); } - if ($media->isNew()) { + if ($media === null || $media->isNew()) { continue; } if ($media->enabled) { - $total_size = ((int)$total_size) + ($media->size ?? 0); + $total_size += $media->size ?? 0; $dirname = ''; $parent = $media->get_parent(); if ($parent != null) { @@ -233,9 +217,9 @@ private function getMediaFiles(array $media_ids): array } } - return array( - $media_files, - $total_size - ); + return [ + 'files' => $media_files, + 'total_size' => $total_size + ]; } } diff --git a/src/Module/Util/ZipHandler.php b/src/Module/Util/ZipHandler.php index 8b7be8e000..1879d48b17 100644 --- a/src/Module/Util/ZipHandler.php +++ b/src/Module/Util/ZipHandler.php @@ -61,22 +61,35 @@ public function isZipable(string $object_type): bool * zips them, adds art and m3u, and sends them * * @param string $name name of the zip file to be created - * @param array $media_files array of full paths to medias to zip create w/ call to get_media_files + * @param array{total_size: int, files: iterable>} $files array of full paths to medias to zip create w/ call to get_media_files * @param bool $flat_path put the files into a single folder */ public function zip( ResponseInterface $response, string $name, - array $media_files, + array $files, bool $flat_path ): ResponseInterface { - $art_name = $this->configContainer->get(ConfigurationKeyEnum::ALBUM_ART_PREFERRED_FILENAME); - $addart = $this->configContainer->isFeatureEnabled(ConfigurationKeyEnum::ART_ZIP_ADD); - $filter = (string)preg_replace('/[^a-zA-Z0-9. -]/', '', $name); - $comment = (string)$this->configContainer->get(ConfigurationKeyEnum::FILE_ZIP_COMMENT); + $art_name = $this->configContainer->get(ConfigurationKeyEnum::ALBUM_ART_PREFERRED_FILENAME); + $addart = $this->configContainer->isFeatureEnabled(ConfigurationKeyEnum::ART_ZIP_ADD); + $archiveName = (string)preg_replace('/[^a-zA-Z0-9. -]/', '', $name); + $comment = (string)$this->configContainer->get(ConfigurationKeyEnum::FILE_ZIP_COMMENT); $this->zipFile = Core::get_tmp_dir() . DIRECTORY_SEPARATOR . uniqid('ampache-zip-'); + ob_end_clean(); + + /* Drop the normal Time limit constraints, this can take a while */ + set_time_limit(0); + + // Write/close session data to release session lock for this script. + // This to allow other pages from the same session to be processed + // Do NOT change any session variable after this call + session_write_close(); + + // Take whatever we've got and send the zip + set_memory_limit($files['total_size'] + 32); + $arc = new ZipArchive(); $arc->open($this->zipFile, ZipArchive::CREATE); if (!empty($comment)) { @@ -86,48 +99,47 @@ public function zip( $playlist = ''; $folder = ''; $artpath = ''; - foreach ($media_files as $files) { - foreach ($files as $file) { + foreach ($files['files'] as $file_list) { + foreach ($file_list as $file) { if (!is_file($file)) { continue; } $dirname = ($flat_path) - ? $filter + ? $archiveName : dirname($file); $artpath = $dirname . DIRECTORY_SEPARATOR . $art_name; $folder = explode(DIRECTORY_SEPARATOR, $dirname)[substr_count($dirname, DIRECTORY_SEPARATOR)]; $playlist .= $folder . DIRECTORY_SEPARATOR . basename($file) . "\n"; + $arc->addEmptyDir($folder, ZipArchive::CREATE); $arc->addFile($file, $folder . DIRECTORY_SEPARATOR . basename($file)); } - if ( - $addart === true && - !empty($folder) && - is_file($artpath) - ) { - $arc->addFile($artpath, $folder . DIRECTORY_SEPARATOR . $art_name); - } + } + if ( + $addart === true && + !empty($folder) && + is_file($artpath) + ) { + $arc->addFile($artpath, $folder . DIRECTORY_SEPARATOR . $art_name); } if (!empty($playlist) && !empty($folder)) { $arc->addEmptyDir($folder, ZipArchive::CREATE); - $arc->addFromString($filter . ".m3u", $playlist); + $arc->addFromString($archiveName . ".m3u", $playlist); } + + $arc->close(); + $this->logger->debug( - 'Sending Zip ' . $filter, + 'Sending Zip ' . $archiveName, [LegacyLogger::CONTEXT_TYPE => __CLASS__] ); - $arc->close(); - // Various different browsers dislike various characters here. Strip them all for safety. - $safeOutput = trim(str_replace(['"', "'", '\\', ';', "\n", "\r"], '', $filter . '.zip')); - - // Check if we need to UTF-8 encode the filename - $urlencoded = rawurlencode($safeOutput); + $normalizedArchiveName = trim(str_replace(['"', "'", '\\', ';', "\n", "\r"], '', $archiveName . '.zip')); return $response ->withHeader('Content-Type', 'application/zip') - ->withHeader('Content-Disposition', sprintf('attachment; filename*=UTF-8\'\'%s', $urlencoded)) + ->withHeader('Content-Disposition', sprintf('attachment; filename*=UTF-8\'\'%s', rawurlencode($normalizedArchiveName))) ->withHeader('Pragma', 'public') ->withHeader('Cache-Control', 'public, must-revalidate') ->withHeader('Content-Transfer-Encoding', 'binary') diff --git a/src/Module/Util/ZipHandlerInterface.php b/src/Module/Util/ZipHandlerInterface.php index 84da6e2ebe..5e3e2ee005 100644 --- a/src/Module/Util/ZipHandlerInterface.php +++ b/src/Module/Util/ZipHandlerInterface.php @@ -37,13 +37,13 @@ public function isZipable(string $object_type): bool; * zips them and sends them * * @param string $name name of the zip file to be created - * @param array $media_files array of full paths to medias to zip create w/ call to get_media_files + * @param array{total_size: int, files: iterable>} $files array of full paths to medias to zip create w/ call to get_media_files * @param bool $flat_path put the files into a single folder */ public function zip( ResponseInterface $response, string $name, - array $media_files, + array $files, bool $flat_path ): ResponseInterface; } diff --git a/src/Repository/Model/Playlist.php b/src/Repository/Model/Playlist.php index 6de83a6464..c8bfb949fa 100644 --- a/src/Repository/Model/Playlist.php +++ b/src/Repository/Model/Playlist.php @@ -351,7 +351,12 @@ public function get_items(): array $db_results = Dba::read($sql, $params); while ($row = Dba::fetch_assoc($db_results)) { - $results[] = ['object_type' => LibraryItemEnum::from($row['object_type']), 'object_id' => (int)$row['object_id'], 'track' => (int)$row['track'], 'track_id' => $row['id']]; + $results[] = [ + 'object_type' => LibraryItemEnum::from($row['object_type']), + 'object_id' => (int)$row['object_id'], + 'track' => (int)$row['track'], + 'track_id' => $row['id'] + ]; } } diff --git a/src/Repository/Model/Search.php b/src/Repository/Model/Search.php index a8b2622565..49ff540a22 100755 --- a/src/Repository/Model/Search.php +++ b/src/Repository/Model/Search.php @@ -1343,7 +1343,12 @@ public function get_items(): array $count = 1; $db_results = Dba::read($sql, $sqltbl['parameters']); while ($row = Dba::fetch_assoc($db_results)) { - $results[] = ['object_id' => $row['id'], 'object_type' => LibraryItemEnum::from($this->objectType), 'track' => $count++, 'track_id' => $row['id']]; + $results[] = [ + 'object_id' => $row['id'], + 'object_type' => LibraryItemEnum::from($this->objectType), + 'track' => $count++, + 'track_id' => $row['id'] + ]; } $this->date = time(); diff --git a/src/Repository/Model/playlist_object.php b/src/Repository/Model/playlist_object.php index 53cfcecbe9..f1670863f1 100644 --- a/src/Repository/Model/playlist_object.php +++ b/src/Repository/Model/playlist_object.php @@ -72,7 +72,9 @@ abstract class playlist_object extends database_object implements library_item /** * @return list */ abstract public function get_items(): array;