From 119547e39218c152f40fad4f2705e3a7469a2366 Mon Sep 17 00:00:00 2001 From: skjnldsv Date: Tue, 22 Sep 2026 10:22:57 +0200 Subject: [PATCH] feat(preview): honor enabledPreviewProviders as generation try-order Split out of #63796. Providers registered via registerProviderClosure() now keep their class name, and PreviewManager rebuilds the provider map on each getProviders() call so foreach order follows the index of each class in enabledPreviewProviders (registration order for anything not listed). Generator::generateProviderPreview() collects matches first to preserve that order, then wraps getThumbnail() in a try/catch so one throwing provider is skipped in favor of the next instead of aborting generation. When enabledPreviewProviders is unset, the computed default now prefers Imaginary first if preview_imaginary_url is configured, with native HEIC appended as a fallback when Imagick supports it. The shared default/ recommended provider lists live in the new PreviewProviderDefaults so the upcoming admin settings page (PR B) can reuse them without duplicating the provider list literals. Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: skjnldsv --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Preview/Generator.php | 92 +++++++++------ .../Preview/PreviewProviderDefaults.php | 58 +++++++++ lib/private/PreviewManager.php | 96 +++++++++++---- tests/lib/Preview/GeneratorTest.php | 111 ++++++++++++++++++ .../PreviewManagerProviderOrderTest.php | 74 ++++++++++++ 7 files changed, 373 insertions(+), 60 deletions(-) create mode 100644 lib/private/Preview/PreviewProviderDefaults.php create mode 100644 tests/lib/Preview/PreviewManagerProviderOrderTest.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 848bd66185d83..db4462006f280 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -2171,6 +2171,7 @@ 'OC\\Preview\\Photoshop' => $baseDir . '/lib/private/Preview/Photoshop.php', 'OC\\Preview\\Postscript' => $baseDir . '/lib/private/Preview/Postscript.php', 'OC\\Preview\\PreviewMigrationService' => $baseDir . '/lib/private/Preview/PreviewMigrationService.php', + 'OC\\Preview\\PreviewProviderDefaults' => $baseDir . '/lib/private/Preview/PreviewProviderDefaults.php', 'OC\\Preview\\PreviewService' => $baseDir . '/lib/private/Preview/PreviewService.php', 'OC\\Preview\\ProviderV2' => $baseDir . '/lib/private/Preview/ProviderV2.php', 'OC\\Preview\\SGI' => $baseDir . '/lib/private/Preview/SGI.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 787e012e3215a..492c2f7d33928 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -2212,6 +2212,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Preview\\Photoshop' => __DIR__ . '/../../..' . '/lib/private/Preview/Photoshop.php', 'OC\\Preview\\Postscript' => __DIR__ . '/../../..' . '/lib/private/Preview/Postscript.php', 'OC\\Preview\\PreviewMigrationService' => __DIR__ . '/../../..' . '/lib/private/Preview/PreviewMigrationService.php', + 'OC\\Preview\\PreviewProviderDefaults' => __DIR__ . '/../../..' . '/lib/private/Preview/PreviewProviderDefaults.php', 'OC\\Preview\\PreviewService' => __DIR__ . '/../../..' . '/lib/private/Preview/PreviewService.php', 'OC\\Preview\\ProviderV2' => __DIR__ . '/../../..' . '/lib/private/Preview/ProviderV2.php', 'OC\\Preview\\SGI' => __DIR__ . '/../../..' . '/lib/private/Preview/SGI.php', diff --git a/lib/private/Preview/Generator.php b/lib/private/Preview/Generator.php index 94fea7314b565..375ddbe39301d 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -349,60 +349,74 @@ private function getMaxPreview(array $previews, File $file, string $mimeType, ?s * @throws NotFoundException */ private function generateProviderPreview(File $file, int $width, int $height, bool $crop, bool $max, string $mimeType, ?string $version): Preview { - $previewProviders = $this->previewManager->getProviders(); - foreach ($previewProviders as $supportedMimeType => $providers) { - // Filter out providers that does not support this mime + $entries = []; + foreach ($this->previewManager->getProviders() as $supportedMimeType => $providers) { if (!preg_match($supportedMimeType, $mimeType)) { continue; } foreach ($providers as $providerClosure) { - $provider = $this->helper->getProvider($providerClosure); if (!$provider) { continue; } - if (!$provider->isAvailable($file)) { continue; } + $entries[] = [ + 'class' => ltrim($provider::class, '\\'), + 'provider' => $provider, + ]; + } + } - $previewConcurrency = $this->getNumConcurrentPreviews('preview_concurrency_new'); - $sem = self::guardWithSemaphore(self::SEMAPHORE_ID_NEW, $previewConcurrency); - try { - $this->logger->debug('Calling preview provider for {mimeType} with width={width}, height={height}', [ - 'mimeType' => $mimeType, - 'width' => $width, - 'height' => $height, - ]); - $preview = $this->helper->getThumbnail($provider, $file, $width, $height); - } finally { - self::unguardWithSemaphore($sem); - } + foreach ($entries as $entry) { + $provider = $entry['provider']; + $class = $entry['class']; - if (!($preview instanceof IImage)) { - continue; - } + $previewConcurrency = $this->getNumConcurrentPreviews('preview_concurrency_new'); + $sem = self::guardWithSemaphore(self::SEMAPHORE_ID_NEW, $previewConcurrency); + try { + $this->logger->debug('Calling preview provider {provider} for {mimeType} with width={width}, height={height}', [ + 'provider' => $class, + 'mimeType' => $mimeType, + 'width' => $width, + 'height' => $height, + ]); + $preview = $this->helper->getThumbnail($provider, $file, $width, $height); + } catch (\Throwable $e) { + $this->logger->warning('Preview provider {provider} failed for {mimeType}', [ + 'provider' => $class, + 'mimeType' => $mimeType, + 'exception' => $e, + ]); + continue; + } finally { + self::unguardWithSemaphore($sem); + } - try { - $previewEntry = new Preview(); - $previewEntry->generateId(); - $previewEntry->setFileId($file->getId()); - $previewEntry->setStorageId($file->getMountPoint()->getNumericStorageId()); - $previewEntry->setSourceMimeType($file->getMimeType()); - $previewEntry->setWidth($preview->width()); - $previewEntry->setHeight($preview->height()); - $previewEntry->setVersion($version); - $previewEntry->setMax($max); - $previewEntry->setCropped($crop); - $previewEntry->setEncrypted(false); - $previewEntry->setMimetype($preview->dataMimeType()); - $previewEntry->setEtag($file->getEtag()); - $previewEntry->setMtime((new \DateTime())->getTimestamp()); - return $this->savePreview($previewEntry, $preview); - } catch (NotPermittedException) { - throw new NotFoundException(); - } + if (!($preview instanceof IImage)) { + continue; + } + + try { + $previewEntry = new Preview(); + $previewEntry->generateId(); + $previewEntry->setFileId($file->getId()); + $previewEntry->setStorageId($file->getMountPoint()->getNumericStorageId()); + $previewEntry->setSourceMimeType($file->getMimeType()); + $previewEntry->setWidth($preview->width()); + $previewEntry->setHeight($preview->height()); + $previewEntry->setVersion($version); + $previewEntry->setMax($max); + $previewEntry->setCropped($crop); + $previewEntry->setEncrypted(false); + $previewEntry->setMimetype($preview->dataMimeType()); + $previewEntry->setEtag($file->getEtag()); + $previewEntry->setMtime((new \DateTime())->getTimestamp()); + return $this->savePreview($previewEntry, $preview); + } catch (NotPermittedException) { + throw new NotFoundException(); } } diff --git a/lib/private/Preview/PreviewProviderDefaults.php b/lib/private/Preview/PreviewProviderDefaults.php new file mode 100644 index 0000000000000..4f72d9bf53e92 --- /dev/null +++ b/lib/private/Preview/PreviewProviderDefaults.php @@ -0,0 +1,58 @@ + + */ + public static function getBuiltinDefaultProviders(): array { + return [ + MarkDown::class, + TXT::class, + OpenDocument::class, + PNG::class, + JPEG::class, + GIF::class, + BMP::class, + XBitmap::class, + Krita::class, + WebP::class, + AVIF::class, + ]; + } + + /** + * Nextcloud-encouraged provider list. + * + * Matches core defaults, plus Imaginary first when it is configured (server + * tuning / AIO). Native HEIC is appended as a fallback because Imaginary + * does not handle every HEIC/HEIF file. + * + * @return list + */ + public static function getRecommendedEnabledProviders(bool $imaginaryConfigured, bool $heicFallback = false): array { + $providers = self::getBuiltinDefaultProviders(); + if (!$imaginaryConfigured) { + return $providers; + } + $providers = array_merge([Imaginary::class], $providers); + if ($heicFallback) { + $providers[] = HEIC::class; + } + return array_values(array_unique($providers)); + } +} diff --git a/lib/private/PreviewManager.php b/lib/private/PreviewManager.php index c3f780de39ecb..dfc5f12b92c6b 100644 --- a/lib/private/PreviewManager.php +++ b/lib/private/PreviewManager.php @@ -41,6 +41,7 @@ use OC\Preview\PNG; use OC\Preview\Postscript; use OC\Preview\PreviewMigrationService; +use OC\Preview\PreviewProviderDefaults; use OC\Preview\SGI; use OC\Preview\StarOffice; use OC\Preview\Storage\StorageFactory; @@ -77,6 +78,14 @@ class PreviewManager implements IPreview { /** @var array> $providers */ protected array $providers = []; + /** + * Registrations in insertion order, rebuilt into {@see $providers} using + * ``enabledPreviewProviders`` as key order (PHP maps keep insertion order). + * + * @var list + */ + private array $providerRegistrations = []; + /** @var array mime type => support status */ protected array $mimeTypeSupportMap = []; @@ -111,15 +120,16 @@ public function __construct( * @param string $mimeTypeRegex Regex with the mime types that are supported by this provider * @param ProviderClosure $callable */ - private function registerProviderClosure(string $mimeTypeRegex, Closure $callable): void { + private function registerProviderClosure(string $mimeTypeRegex, Closure $callable, ?string $class = null): void { if (!$this->enablePreviews) { return; } - if (!isset($this->providers[$mimeTypeRegex])) { - $this->providers[$mimeTypeRegex] = []; - } - $this->providers[$mimeTypeRegex][] = $callable; + $this->providerRegistrations[] = [ + 'regex' => $mimeTypeRegex, + 'class' => $class !== null && $class !== '' ? ltrim($class, '\\') : null, + 'callable' => $callable, + ]; $this->providerListDirty = true; } @@ -132,21 +142,57 @@ public function getProviders(): array { $this->registerCoreProviders(); $this->registerBootstrapProviders(); if ($this->providerListDirty) { - $keys = array_map('strlen', array_keys($this->providers)); - array_multisort($keys, SORT_DESC, $this->providers); + $this->rebuildProvidersMap(); $this->providerListDirty = false; } return $this->providers; } + /** + * Rebuild {@see $providers} so foreach order follows ``enabledPreviewProviders``. + * + * PHP arrays preserve insertion order. Each provider has its own MIME regex + * in core, so visiting map keys in whitelist order is try-order for Generator. + */ + private function rebuildProvidersMap(): void { + $priority = []; + foreach ($this->getEnabledDefaultProvider() as $index => $class) { + if (!is_string($class) || $class === '') { + continue; + } + $priority[ltrim($class, '\\')] = $index; + } + + $registrations = []; + foreach ($this->providerRegistrations as $index => $registration) { + $registration['index'] = $index; + $registrations[] = $registration; + } + usort($registrations, function (array $left, array $right) use ($priority): int { + $leftClass = $left['class']; + $rightClass = $right['class']; + $leftPriority = is_string($leftClass) && isset($priority[$leftClass]) ? $priority[$leftClass] : PHP_INT_MAX; + $rightPriority = is_string($rightClass) && isset($priority[$rightClass]) ? $priority[$rightClass] : PHP_INT_MAX; + if ($leftPriority !== $rightPriority) { + return $leftPriority <=> $rightPriority; + } + return $left['index'] <=> $right['index']; + }); + + $this->providers = []; + foreach ($registrations as $registration) { + $this->providers[$registration['regex']][] = $registration['callable']; + } + } + /** * Does the manager have any providers */ #[\Override] public function hasProviders(): bool { $this->registerCoreProviders(); - return !empty($this->providers); + return $this->providerRegistrations !== []; } private function getGenerator(): Generator { @@ -279,11 +325,15 @@ protected function getEnabledDefaultProvider(): array { AVIF::class, ]; - $this->defaultProviders = $this->config->getSystemValue('enabledPreviewProviders', array_merge([ - MarkDown::class, - TXT::class, - OpenDocument::class, - ], $imageProviders)); + $configured = $this->config->getSystemValue('enabledPreviewProviders', null); + if (!is_array($configured)) { + $this->defaultProviders = PreviewProviderDefaults::getRecommendedEnabledProviders( + $this->config->getSystemValueString('preview_imaginary_url', '') !== '', + $this->imagickSupport->hasExtension() && $this->imagickSupport->supportsFormat('HEIC'), + ); + } else { + $this->defaultProviders = $configured; + } if (in_array(Image::class, $this->defaultProviders, true)) { $this->defaultProviders = array_merge($this->defaultProviders, $imageProviders); @@ -302,7 +352,7 @@ protected function registerCoreProvider(string $class, string $mimeType, array $ $this->registerProviderClosure($mimeType, function () use ($class, $options): IProviderV2 { /** @var IProviderV2 $class */ return new $class($options); - }); + }, $class); } } @@ -429,13 +479,17 @@ private function registerBootstrapProviders(): void { } $this->loadedBootstrapProviders[$key] = null; - $this->registerProviderClosure($provider->getMimeTypeRegex(), function () use ($provider): IProviderV2|false { - try { - return $this->container->get($provider->getService()); - } catch (NotFoundExceptionInterface) { - return false; - } - }); + $this->registerProviderClosure( + $provider->getMimeTypeRegex(), + function () use ($provider): IProviderV2|false { + try { + return $this->container->get($provider->getService()); + } catch (NotFoundExceptionInterface) { + return false; + } + }, + $provider->getService(), + ); } } diff --git a/tests/lib/Preview/GeneratorTest.php b/tests/lib/Preview/GeneratorTest.php index 9024d2d0b6887..336fb9870fdd8 100644 --- a/tests/lib/Preview/GeneratorTest.php +++ b/tests/lib/Preview/GeneratorTest.php @@ -399,6 +399,117 @@ public function testNoProvider(): void { $this->generator->getPreview($file, 100, 100); } + public function testTriesNextProviderWhenEarlierFails(): void { + $file = $this->getFile(42, 'myMimeType'); + + $this->previewMapper->method('getAvailablePreviews') + ->with($this->equalTo([42])) + ->willReturn([42 => []]); + $this->config->method('getSystemValueString')->willReturnCallback(fn ($key, $default) => $default); + $this->config->method('getSystemValueInt')->willReturnCallback(fn ($key, $default) => $default); + + $failing = $this->createMock(IProviderV2::class); + $failing->method('isAvailable')->willReturn(true); + $ok = $this->createMock(IProviderV2::class); + $ok->method('isAvailable')->willReturn(true); + $this->previewManager->method('isMimeSupported')->willReturn(true); + $this->previewManager->method('getProviders')->willReturn([ + '/myMimeType/' => ['failing', 'ok'], + ]); + $this->helper->method('getProvider')->willReturnOnConsecutiveCalls($failing, $ok); + + $image = $this->createMock(IImage::class); + $image->method('width')->willReturn(2048); + $image->method('height')->willReturn(2048); + $image->method('valid')->willReturn(true); + $image->method('dataMimeType')->willReturn('image/png'); + $image->method('data')->willReturn('my data'); + $this->helper->method('getThumbnail')->willReturnCallback(function ($provider) use ($failing, $image) { + if ($provider === $failing) { + throw new \RuntimeException('imaginary down'); + } + return $image; + }); + $this->helper->method('getImage')->willReturn($this->getMockImage(2048, 2048, 'my resized data')); + $this->previewMapper->method('insert')->willReturnCallback(fn (Preview $preview): Preview => $preview); + $this->previewMapper->method('update')->willReturnCallback(fn (Preview $preview): Preview => $preview); + $this->storageFactory->method('writePreview')->willReturn(1000); + + $result = $this->generator->getPreview($file, 100, 100); + $this->assertSame('256-256.png', $result->getName()); + } + + public function testTriesProvidersInEnabledPreviewProvidersOrder(): void { + $file = $this->getFile(42, 'image/heic'); + + $this->previewMapper->method('getAvailablePreviews') + ->with($this->equalTo([42])) + ->willReturn([42 => []]); + $this->config->method('getSystemValueString')->willReturnCallback(fn ($key, $default) => $default); + $this->config->method('getSystemValueInt')->willReturnCallback(fn ($key, $default) => $default); + + $imaginary = new class implements IProviderV2 { + public function getMimeType(): string { + return '/image\/heic/'; + } + public function isAvailable(\OCP\Files\FileInfo $file): bool { + return true; + } + public function getThumbnail(\OCP\Files\File $file, int $maxX, int $maxY): ?IImage { + return null; + } + }; + $heic = new class implements IProviderV2 { + public function getMimeType(): string { + return '/image\/heic/'; + } + public function isAvailable(\OCP\Files\FileInfo $file): bool { + return true; + } + public function getThumbnail(\OCP\Files\File $file, int $maxX, int $maxY): ?IImage { + return null; + } + }; + + $this->previewManager->method('isMimeSupported')->willReturn(true); + // PreviewManager emits this map in enabledPreviewProviders key order + // (PHP insertion order), not regex-length order. + $this->previewManager->method('getProviders')->willReturn([ + '/image\/heic/' => ['heic'], + '/(image\/(bmp|png|jpeg|heic|heif))/' => ['imaginary'], + ]); + $this->helper->method('getProvider')->willReturnCallback(function ($id) use ($imaginary, $heic) { + return match ($id) { + 'imaginary' => $imaginary, + 'heic' => $heic, + default => false, + }; + }); + + $image = $this->createMock(IImage::class); + $image->method('width')->willReturn(2048); + $image->method('height')->willReturn(2048); + $image->method('valid')->willReturn(true); + $image->method('dataMimeType')->willReturn('image/jpeg'); + $image->method('data')->willReturn('heic data'); + $tried = []; + $this->helper->method('getThumbnail')->willReturnCallback(function ($provider) use ($heic, $imaginary, $image, &$tried) { + $tried[] = $provider; + if ($provider === $heic) { + return $image; + } + $this->fail('Imaginary must not run first when HEIC is earlier in enabledPreviewProviders'); + }); + $this->helper->method('getImage')->willReturn($this->getMockImage(2048, 2048, 'resized')); + $this->previewMapper->method('insert')->willReturnCallback(fn (Preview $preview): Preview => $preview); + $this->previewMapper->method('update')->willReturnCallback(fn (Preview $preview): Preview => $preview); + $this->storageFactory->method('writePreview')->willReturn(1000); + + $result = $this->generator->getPreview($file, 100, 100); + $this->assertSame([$heic], $tried); + $this->assertSame('256-256.png', $result->getName()); + } + private function getMockImage(int $width, int $height, string $data = '') { $image = $this->createMock(IImage::class); $image->method('height')->willReturn($width); diff --git a/tests/lib/Preview/PreviewManagerProviderOrderTest.php b/tests/lib/Preview/PreviewManagerProviderOrderTest.php new file mode 100644 index 0000000000000..2ba3df1001f96 --- /dev/null +++ b/tests/lib/Preview/PreviewManagerProviderOrderTest.php @@ -0,0 +1,74 @@ +createMock(IConfig::class); + $config->method('getSystemValueBool')->willReturnCallback(fn (string $key, bool $default) => match ($key) { + 'enable_previews' => true, + default => $default, + }); + $config->method('getSystemValue')->willReturnCallback(function (string $key, mixed $default) { + if ($key === 'enabledPreviewProviders') { + return [HEIC::class, Imaginary::class, JPEG::class]; + } + return $default; + }); + $config->method('getSystemValueString')->willReturn(''); + + $imagick = $this->createMock(IMagickSupport::class); + $imagick->method('hasExtension')->willReturn(true); + $imagick->method('supportsFormat')->willReturn(true); + + $finder = $this->createMock(IBinaryFinder::class); + $finder->method('findBinaryPath')->willReturn(false); + + $coordinator = $this->createMock(Coordinator::class); + $coordinator->method('getRegistrationContext')->willReturn(null); + + $manager = new PreviewManager( + $config, + $this->createMock(IRootFolder::class), + $this->createMock(IEventDispatcher::class), + $this->createMock(GeneratorHelper::class), + null, + $coordinator, + $this->createMock(ContainerInterface::class), + $finder, + $imagick, + ); + + $keys = array_keys($manager->getProviders()); + $heicIndex = array_search('/image\/(x-)?hei(f|c)/', $keys, true); + $imaginaryIndex = array_search(Imaginary::supportedMimeTypes(), $keys, true); + $jpegIndex = array_search('/image\/jpeg/', $keys, true); + + $this->assertNotFalse($heicIndex); + $this->assertNotFalse($imaginaryIndex); + $this->assertNotFalse($jpegIndex); + $this->assertLessThan($imaginaryIndex, $heicIndex); + $this->assertLessThan($jpegIndex, $imaginaryIndex); + } +}