From 2cab6290ef934428da9b3e6cb14e41cd59a8de84 Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Fri, 14 Aug 2026 16:06:43 +0200 Subject: [PATCH 1/8] fix(ImageResizer): predict source image dimensions before resize - Add readSourceDimensions() to detect source image size from local or remote disks, with graceful fallbacks for missing/unreadable files. - Add calculateResizedDimensions() to mirror Storm's aspect-ratio math without loading GD resources. - Refactor filterGetDimensions() to warm up the config cache and delegate to a shared computeCachedDimensions() helper using Cache::rememberForever, eliminating temp file creation for reads. - Add getDimensionsFromResizerUrl() fallback so /resizer/ URLs resolve dimensions from cache or source. - Add unit tests covering dimension parity, missing images, and resizer URL dimension retrieval. --- modules/system/classes/ImageResizer.php | 249 ++++++++++++++++-- .../system/tests/classes/ImageResizerTest.php | 90 +++++++ 2 files changed, 322 insertions(+), 17 deletions(-) diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index 5f178f4c43..e469d0bcc5 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -257,6 +257,63 @@ public function getConfig(): array return $config; } + /** + * Read the source image dimensions from the given disk and path. + * + * Returns ['width' => 0, 'height' => 0] when the file is missing, + * unreadable, or the dimensions cannot be determined. + * + * For local disks the file is read directly to avoid unnecessary I/O. + * Remote disks (S3, FTP, etc.) are downloaded to a temporary file first + * because getimagesize() requires a local path. + * + * @param FilesystemAdapter|string $disk + * @param string $path Path to the image on the disk + * @return array + */ + protected static function readSourceDimensions(FilesystemAdapter|string $disk, string $path): array + { + if (is_string($disk)) { + $disk = Storage::disk($disk); + } + + $origWidth = 0; + $origHeight = 0; + + try { + if (!$disk->exists($path)) { + return ['width' => 0, 'height' => 0]; + } + + if (FileHelper::isLocalDisk($disk)) { + $localPath = $disk->getPathPrefix() . $path; + $size = @getimagesize($localPath); + if ($size !== false) { + return ['width' => $size[0], 'height' => $size[1]]; + } + } + + $tempDir = temp_path() . '/resizer'; + $tempPath = $tempDir . '/' . uniqid() . '.' . FileHelper::extension($path); + + if (!FileHelper::isDirectory($tempDir)) { + FileHelper::makeDirectory($tempDir, 0777, true, true); + } + + FileHelper::put($tempPath, $disk->get($path)); + $size = @getimagesize($tempPath); + if ($size !== false) { + $origWidth = $size[0]; + $origHeight = $size[1]; + } + @unlink($tempPath); + } catch (\Exception $ex) { + // Ignore failures to read source dimensions + } + + return ['width' => $origWidth, 'height' => $origHeight]; + } + /** * Process the resize request */ @@ -912,27 +969,185 @@ public static function filterGetUrl($image, $width = null, $height = null, $opti */ public static function filterGetDimensions($image): array { - $resizer = new static($image); + try { + $resizer = new static($image); + } catch (\SystemException $ex) { + if (is_string($image) && str_starts_with($image, '/resizer/')) { + return static::getDimensionsFromResizerUrl($image); + } + return ['width' => 0, 'height' => 0]; + } - return Cache::rememberForever(static::CACHE_PREFIX . 'dimensions.' . $resizer->getIdentifier(), function () use ($resizer) { - // Prepare the local file for assessment - $tempPath = $resizer->getLocalTempPath(); - $dimensions = []; + $identifier = $resizer->getIdentifier(); + $configCacheKey = static::CACHE_PREFIX . $identifier; - // Attempt to get the image size - try { - $size = getimagesize($tempPath); - $dimensions['width'] = $size[0]; - $dimensions['height'] = $size[1]; - } catch (\Exception $ex) { - @unlink($tempPath); - throw $ex; - } + if (!Cache::has($configCacheKey)) { + Cache::put($configCacheKey, $resizer->getConfig()); + } - // Cleanup afterwards - @unlink($tempPath); + return static::computeCachedDimensions($identifier); + } - return $dimensions; + /** + * Extract dimensions from a /resizer/* URL by reading the cached + * resizer configuration and, if possible, the source file. + * + * @param string $url The /resizer/* URL + * @return array + */ + protected static function getDimensionsFromResizerUrl(string $url): array + { + $path = parse_url($url, PHP_URL_PATH); + $segments = explode('/', ltrim($path, '/')); + + if (count($segments) < 3 || $segments[0] !== 'resizer') { + return ['width' => 0, 'height' => 0]; + } + + $identifier = $segments[1]; + + if (!static::isValidIdentifier($identifier)) { + return ['width' => 0, 'height' => 0]; + } + + return static::computeCachedDimensions($identifier); + } + + /** + * Compute and cache the output dimensions for a resizer configuration + * identified by its cache key suffix. + * + * @param string $identifier The resizer identifier + * @return array + */ + protected static function computeCachedDimensions(string $identifier): array + { + $cacheKey = static::CACHE_PREFIX . $identifier . '.dimensions'; + + return Cache::rememberForever($cacheKey, function () use ($identifier) { + $config = Cache::get(static::CACHE_PREFIX . $identifier); + + if (empty($config) || !isset($config['width'], $config['height'], $config['options']['mode'])) { + return ['width' => 0, 'height' => 0]; + } + + $sourceDimensions = static::readSourceDimensions( + $config['image']['disk'], + $config['image']['path'] + ); + $origWidth = $sourceDimensions['width']; + $origHeight = $sourceDimensions['height']; + + return static::calculateResizedDimensions( + $origWidth, + $origHeight, + $config['width'], + $config['height'], + $config['options']['mode'] + ); }); } + + /** + * Calculate the expected output dimensions for a resize operation. + * + * This method intentionally duplicates the aspect-ratio math from + * \Winter\Storm\Database\Attach\Resizer::getDimensions() rather than + * delegating to it. Reasons: + * + * - getDimensions() is protected in Storm; making it public would be a + * BC surface expansion that Storm maintainers may not accept. + * - Calling Resizer::open() allocates GD resources solely to read + * dimensions, which is wasteful when getimagesize() is sufficient. + * - The formulas are stable arithmetic that has not changed in years. + * + * If Storm's math ever diverges, a single integration test comparing + * both implementations will catch the drift. + * + * @param int $origWidth Original image width (0 if unknown) + * @param int $origHeight Original image height (0 if unknown) + * @param int $reqWidth Requested output width + * @param int $reqHeight Requested output height + * @param string $mode Resize mode: exact, portrait, landscape, auto, fit, crop + * @return array + */ + protected static function calculateResizedDimensions( + int $origWidth, + int $origHeight, + int $reqWidth, + int $reqHeight, + string $mode + ): array { + if ($origWidth <= 0 || $origHeight <= 0) { + return ['width' => $reqWidth, 'height' => $reqHeight]; + } + + switch ($mode) { + case 'exact': + return ['width' => $reqWidth, 'height' => $reqHeight]; + + case 'crop': + $heightRatio = $origHeight / $reqHeight; + $widthRatio = $origWidth / $reqWidth; + $optimalRatio = $heightRatio < $widthRatio ? $heightRatio : $widthRatio; + + return [ + 'width' => (int) round($origWidth / $optimalRatio), + 'height' => (int) round($origHeight / $optimalRatio), + ]; + + case 'fit': + $ratioW = $reqWidth / $origWidth; + $ratioH = $reqHeight / $origHeight; + $effectiveRatio = min($ratioW, $ratioH); + return [ + 'width' => (int) round($origWidth * $effectiveRatio), + 'height' => (int) round($origHeight * $effectiveRatio), + ]; + + case 'portrait': + $ratio = $origWidth / $origHeight; + return [ + 'width' => (int) round($reqHeight * $ratio), + 'height' => $reqHeight, + ]; + + case 'landscape': + $ratio = $origHeight / $origWidth; + return [ + 'width' => $reqWidth, + 'height' => (int) round($reqWidth * $ratio), + ]; + + case 'auto': + default: + if ($reqWidth > 0 && $reqHeight > 0) { + if ($origHeight < $origWidth) { + $optimalHeight = (int) round($origHeight * ($reqWidth / $origWidth)); + return ['width' => $reqWidth, 'height' => $optimalHeight]; + } elseif ($origHeight > $origWidth) { + $optimalWidth = (int) round($origWidth * ($reqHeight / $origHeight)); + return ['width' => $optimalWidth, 'height' => $reqHeight]; + } else { + if ($reqHeight < $reqWidth) { + $optimalHeight = (int) round($origHeight * ($reqWidth / $origWidth)); + return ['width' => $reqWidth, 'height' => $optimalHeight]; + } elseif ($reqHeight > $reqWidth) { + $optimalWidth = (int) round($origWidth * ($reqHeight / $origHeight)); + return ['width' => $optimalWidth, 'height' => $reqHeight]; + } else { + return ['width' => $reqWidth, 'height' => $reqHeight]; + } + } + } elseif ($reqWidth > 0) { + $optimalHeight = (int) round($origHeight * ($reqWidth / $origWidth)); + return ['width' => $reqWidth, 'height' => $optimalHeight]; + } elseif ($reqHeight > 0) { + $optimalWidth = (int) round($origWidth * ($reqHeight / $origHeight)); + return ['width' => $optimalWidth, 'height' => $reqHeight]; + } else { + return ['width' => $origWidth, 'height' => $origHeight]; + } + } + } } diff --git a/modules/system/tests/classes/ImageResizerTest.php b/modules/system/tests/classes/ImageResizerTest.php index 26c379b73e..5fc74c6417 100644 --- a/modules/system/tests/classes/ImageResizerTest.php +++ b/modules/system/tests/classes/ImageResizerTest.php @@ -5,6 +5,7 @@ use Backend\Facades\Backend; use Cms\Classes\Controller as CmsController; use Cms\Classes\Theme; +use Cache; use Config; use DMS\PHPUnitExtensions\ArraySubset\ArraySubsetAsserts; use Event; @@ -41,6 +42,8 @@ public function tearDown(): void Config::set('cms.themesPath', $this->originalThemesPath); ImageResizer::flushAvailableSources(); + Cache::flush(); + parent::tearDown(); } @@ -436,6 +439,93 @@ public function testResizerRedirect() Storage::disk('test_local')->deleteDirectory('resized'); } + public function testCalculateResizedDimensionsMatchesDefaultResizer() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $imagePath = base_path('modules/system/tests/fixtures/plugins/database/tester/assets/images/avatar.png'); + + $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $modes = ['exact', 'portrait', 'landscape', 'auto', 'fit', 'crop']; + $reqWidth = 200; + $reqHeight = 150; + + $stormGetDimensions = new \ReflectionMethod($resizer, 'getDimensions'); + $stormGetDimensions->setAccessible(true); + $stormWidth = new \ReflectionProperty($resizer, 'width'); + $stormWidth->setAccessible(true); + $stormHeight = new \ReflectionProperty($resizer, 'height'); + $stormHeight->setAccessible(true); + $winterMethod = new \ReflectionMethod(ImageResizer::class, 'calculateResizedDimensions'); + $winterMethod->setAccessible(true); + + foreach ($modes as $mode) { + $resizer->setOptions(['mode' => $mode]); + $expected = $stormGetDimensions->invoke($resizer, $reqWidth, $reqHeight); + $expected = ['width' => (int) $expected[0], 'height' => (int) $expected[1]]; + + $calculated = $winterMethod->invoke( + null, + $stormWidth->getValue($resizer), + $stormHeight->getValue($resizer), + $reqWidth, + $reqHeight, + $mode + ); + + $this->assertSame($expected, $calculated, "Mode $mode output should match DefaultResizer"); + } + } + + public function testFilterGetDimensionsReturnsFallbackForMissingImage() + { + $this->assertSame(['width' => 0, 'height' => 0], ImageResizer::filterGetDimensions( + '/plugins/database/tester/assets/images/MISSING.png' + )); + } + + public function testFilterGetDimensionsReturnsOriginalWhenNoResizeRequested() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + $this->copyMedia(); + + $url = URL::to(MediaLibrary::url('winter.png')); + $dimensions = ImageResizer::filterGetDimensions($url); + + $this->assertSame(310, $dimensions['width']); + $this->assertSame(310, $dimensions['height']); + } + + public function testFilterGetDimensionsFromResizerUrl() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + $this->copyMedia(); + + $imageResizer = new ImageResizer( + URL::to(MediaLibrary::url('winter.png')), + 100, + 100 + ); + $resizerUrl = $imageResizer->getResizerUrl(); + + $this->assertStringStartsWith('/resizer/', $resizerUrl); + + $dimensions = ImageResizer::filterGetDimensions($resizerUrl); + + $this->assertSame(100, $dimensions['width']); + $this->assertSame(100, $dimensions['height']); + } + protected function setUpStorage() { $this->app->useStoragePath(base_path('storage/temp')); From 9768e633b3bf3dfe8129de53bfaec147f5ee9932 Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Fri, 14 Aug 2026 16:42:10 +0200 Subject: [PATCH 2/8] refactor(ImageResizer): ensure temporary file cleanup in readSourceDimensions Wrap the dimension reading logic in a try-finally block to guarantee that temporary files are unlinked even if an exception occurs during the process. This prevents filesystem clutter caused by failed dimension lookups. --- modules/system/classes/ImageResizer.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index e469d0bcc5..55c5cef3d5 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -306,9 +306,12 @@ protected static function readSourceDimensions(FilesystemAdapter|string $disk, s $origWidth = $size[0]; $origHeight = $size[1]; } - @unlink($tempPath); } catch (\Exception $ex) { // Ignore failures to read source dimensions + } finally { + if (isset($tempPath)) { + @unlink($tempPath); + } } return ['width' => $origWidth, 'height' => $origHeight]; From fac5a7f3e8c42a8191f582a437e809a74fdcd84e Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Fri, 14 Aug 2026 16:56:53 +0200 Subject: [PATCH 3/8] fix(ImageResizer): support absolute URLs in filterGetDimensions Improve the detection of resizer URLs by parsing the path from the provided URL string. This ensures that absolute URLs are correctly identified and processed by getDimensionsFromResizerUrl instead of failing the string prefix check. Includes a new test case to verify dimension retrieval from absolute resizer URLs. --- modules/system/classes/ImageResizer.php | 7 +++-- .../system/tests/classes/ImageResizerTest.php | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index 55c5cef3d5..00b93b163b 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -975,8 +975,11 @@ public static function filterGetDimensions($image): array try { $resizer = new static($image); } catch (\SystemException $ex) { - if (is_string($image) && str_starts_with($image, '/resizer/')) { - return static::getDimensionsFromResizerUrl($image); + if (is_string($image)) { + $path = parse_url($image, PHP_URL_PATH); + if ($path && str_starts_with($path, '/resizer/')) { + return static::getDimensionsFromResizerUrl($image); + } } return ['width' => 0, 'height' => 0]; } diff --git a/modules/system/tests/classes/ImageResizerTest.php b/modules/system/tests/classes/ImageResizerTest.php index 5fc74c6417..6d21ecdd80 100644 --- a/modules/system/tests/classes/ImageResizerTest.php +++ b/modules/system/tests/classes/ImageResizerTest.php @@ -526,6 +526,32 @@ public function testFilterGetDimensionsFromResizerUrl() $this->assertSame(100, $dimensions['height']); } + public function testFilterGetDimensionsFromAbsoluteResizerUrl() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + $this->copyMedia(); + + Config::set('cms.linkPolicy', 'force'); + + $imageResizer = new ImageResizer( + URL::to(MediaLibrary::url('winter.png')), + 100, + 100 + ); + $resizerUrl = $imageResizer->getResizerUrl(); + + $this->assertStringStartsWith('http', $resizerUrl); + + $dimensions = ImageResizer::filterGetDimensions($resizerUrl); + + $this->assertSame(100, $dimensions['width']); + $this->assertSame(100, $dimensions['height']); + } + protected function setUpStorage() { $this->app->useStoragePath(base_path('storage/temp')); From 71b17c500eb4b842891b167073c4624cf232e3c6 Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:02:50 +0200 Subject: [PATCH 4/8] fix(ImageResizer): clear dimension cache during identifier refresh Ensure that the dimension cache is invalidated alongside the main configuration cache when `fromIdentifier` is called. --- modules/system/classes/ImageResizer.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index 00b93b163b..33b6f7ee01 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -905,6 +905,7 @@ public static function fromIdentifier(string $identifier): self // since the browser will "steal" the configuration with the first request it makes // if we pull the configuration data out immediately. Cache::forget($cacheKey); + Cache::forget($cacheKey . '.dimensions'); return $resizer; } @@ -1031,11 +1032,11 @@ protected static function computeCachedDimensions(string $identifier): array $cacheKey = static::CACHE_PREFIX . $identifier . '.dimensions'; return Cache::rememberForever($cacheKey, function () use ($identifier) { - $config = Cache::get(static::CACHE_PREFIX . $identifier); + $config = Cache::get(static::CACHE_PREFIX . $identifier); - if (empty($config) || !isset($config['width'], $config['height'], $config['options']['mode'])) { - return ['width' => 0, 'height' => 0]; - } + if (empty($config) || !isset($config['width'], $config['height'], $config['options']['mode'])) { + return ['width' => 0, 'height' => 0]; + } $sourceDimensions = static::readSourceDimensions( $config['image']['disk'], From 31104e6a94c17de8ed1254ce9ed9939658e393c5 Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:04:11 +0200 Subject: [PATCH 5/8] refactor(ImageResizer): pass configuration to dimension cache closure Inject the retrieved `$config` object directly into the `rememberForever` closure instead of re-fetching it via the `$identifier`. This streamlines the dimension computation process by utilizing the existing configuration data. --- modules/system/classes/ImageResizer.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index 33b6f7ee01..9b2733978e 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -1030,14 +1030,13 @@ protected static function getDimensionsFromResizerUrl(string $url): array protected static function computeCachedDimensions(string $identifier): array { $cacheKey = static::CACHE_PREFIX . $identifier . '.dimensions'; - - return Cache::rememberForever($cacheKey, function () use ($identifier) { $config = Cache::get(static::CACHE_PREFIX . $identifier); if (empty($config) || !isset($config['width'], $config['height'], $config['options']['mode'])) { return ['width' => 0, 'height' => 0]; } + return Cache::rememberForever($cacheKey, function () use ($config) { $sourceDimensions = static::readSourceDimensions( $config['image']['disk'], $config['image']['path'] From e5699517ee2617c331e97a1a594031d6ca2822d1 Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:14:52 +0200 Subject: [PATCH 6/8] fix(ImageResizer): prevent division by zero in ImageResizer crop mode Add a guard clause to ensure requested dimensions are greater than zero before calculating aspect ratios during a crop operation. --- modules/system/classes/ImageResizer.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index 9b2733978e..922fecf8f9 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -1093,6 +1093,10 @@ protected static function calculateResizedDimensions( return ['width' => $reqWidth, 'height' => $reqHeight]; case 'crop': + if ($reqWidth <= 0 || $reqHeight <= 0) { + return ['width' => $origWidth, 'height' => $origHeight]; + } + $heightRatio = $origHeight / $reqHeight; $widthRatio = $origWidth / $reqWidth; $optimalRatio = $heightRatio < $widthRatio ? $heightRatio : $widthRatio; From bc1febe0ce3af612a50c6bb87b4c0210e8226832 Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:54:18 +0200 Subject: [PATCH 7/8] refactor(ImageResizer): optimize dimension caching and source retrieval Improve the dimension caching mechanism by separating source dimensions from processed dimensions. This allows for more granular cache invalidation and prevents invalid (0x0) dimensions from being cached permanently, ensuring subsequent attempts can retry the source retrieval. - Introduce `.source` cache key to store original image dimensions - Update `fromIdentifier` to clear the new source cache key - Implement logic to skip caching when dimensions are non-positive - Refactor `computeCachedDimensions` to utilize the source cache --- modules/system/classes/ImageResizer.php | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index 922fecf8f9..1b95c15950 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -906,6 +906,7 @@ public static function fromIdentifier(string $identifier): self // if we pull the configuration data out immediately. Cache::forget($cacheKey); Cache::forget($cacheKey . '.dimensions'); + Cache::forget($cacheKey . '.source'); return $resizer; } @@ -1029,14 +1030,20 @@ protected static function getDimensionsFromResizerUrl(string $url): array */ protected static function computeCachedDimensions(string $identifier): array { - $cacheKey = static::CACHE_PREFIX . $identifier . '.dimensions'; $config = Cache::get(static::CACHE_PREFIX . $identifier); if (empty($config) || !isset($config['width'], $config['height'], $config['options']['mode'])) { return ['width' => 0, 'height' => 0]; } - return Cache::rememberForever($cacheKey, function () use ($config) { + $sourceCacheKey = static::CACHE_PREFIX . $identifier . '.source'; + $dimensionsCacheKey = static::CACHE_PREFIX . $identifier . '.dimensions'; + + $cachedSource = Cache::get($sourceCacheKey); + if ($cachedSource !== null) { + $origWidth = $cachedSource['width']; + $origHeight = $cachedSource['height']; + } else { $sourceDimensions = static::readSourceDimensions( $config['image']['disk'], $config['image']['path'] @@ -1044,6 +1051,14 @@ protected static function computeCachedDimensions(string $identifier): array $origWidth = $sourceDimensions['width']; $origHeight = $sourceDimensions['height']; + if ($origWidth <= 0 || $origHeight <= 0) { + return ['width' => $config['width'], 'height' => $config['height']]; + } + + Cache::forever($sourceCacheKey, ['width' => $origWidth, 'height' => $origHeight]); + } + + return Cache::rememberForever($dimensionsCacheKey, function () use ($config, $origWidth, $origHeight) { return static::calculateResizedDimensions( $origWidth, $origHeight, From 248096457366ea924b6163857738973c3adea56a Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:35:19 +0200 Subject: [PATCH 8/8] fix(system): correct image resizer dimension prediction and caching Based on Copilot AI PR review. - Read EXIF orientation when measuring source dimensions so portrait photos report correct aspect ratios - Return final cropped dimensions instead of Storm's intermediate canvas - Avoid caching failed source-dimension reads to prevent permanent stale fallback values from transient errors - Add tests asserting predicted dimensions against actual resize() output, including zero-dimension and 1px edge cases --- modules/system/classes/ImageResizer.php | 107 +++++++------ .../system/tests/classes/ImageResizerTest.php | 148 ++++++++++++++++-- 2 files changed, 198 insertions(+), 57 deletions(-) diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index 1b95c15950..d77e8ade87 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -279,6 +279,8 @@ protected static function readSourceDimensions(FilesystemAdapter|string $disk, s $origWidth = 0; $origHeight = 0; + $localPathForExif = null; + $tempPath = null; try { if (!$disk->exists($path)) { @@ -289,22 +291,34 @@ protected static function readSourceDimensions(FilesystemAdapter|string $disk, s $localPath = $disk->getPathPrefix() . $path; $size = @getimagesize($localPath); if ($size !== false) { - return ['width' => $size[0], 'height' => $size[1]]; + $origWidth = $size[0]; + $origHeight = $size[1]; + $localPathForExif = $localPath; } - } + } else { + $tempDir = temp_path() . '/resizer'; + $tempPath = $tempDir . '/' . uniqid() . '.' . FileHelper::extension($path); - $tempDir = temp_path() . '/resizer'; - $tempPath = $tempDir . '/' . uniqid() . '.' . FileHelper::extension($path); + if (!FileHelper::isDirectory($tempDir)) { + FileHelper::makeDirectory($tempDir, 0777, true, true); + } - if (!FileHelper::isDirectory($tempDir)) { - FileHelper::makeDirectory($tempDir, 0777, true, true); + FileHelper::put($tempPath, $disk->get($path)); + $size = @getimagesize($tempPath); + if ($size !== false) { + $origWidth = $size[0]; + $origHeight = $size[1]; + $localPathForExif = $tempPath; + } } - FileHelper::put($tempPath, $disk->get($path)); - $size = @getimagesize($tempPath); - if ($size !== false) { - $origWidth = $size[0]; - $origHeight = $size[1]; + if ($localPathForExif && function_exists('exif_read_data')) { + $exif = @exif_read_data($localPathForExif); + if (!empty($exif['Orientation']) && in_array($exif['Orientation'], [1, 3, 6, 8], true)) { + if (in_array($exif['Orientation'], [6, 8], true)) { + [$origWidth, $origHeight] = [$origHeight, $origWidth]; + } + } } } catch (\Exception $ex) { // Ignore failures to read source dimensions @@ -1052,12 +1066,16 @@ protected static function computeCachedDimensions(string $identifier): array $origHeight = $sourceDimensions['height']; if ($origWidth <= 0 || $origHeight <= 0) { + // Don't cache failed reads; return fallback directly without caching + // so transient errors (network blip, temp file issue) don't become permanent. return ['width' => $config['width'], 'height' => $config['height']]; } + // Cache successful reads forever since source image dimensions don't change Cache::forever($sourceCacheKey, ['width' => $origWidth, 'height' => $origHeight]); } + // If we have valid source dimensions, compute and cache the result forever return Cache::rememberForever($dimensionsCacheKey, function () use ($config, $origWidth, $origHeight) { return static::calculateResizedDimensions( $origWidth, @@ -1103,23 +1121,22 @@ protected static function calculateResizedDimensions( return ['width' => $reqWidth, 'height' => $reqHeight]; } + if ($reqWidth <= 0 && $reqHeight <= 0) { + return ['width' => $origWidth, 'height' => $origHeight]; + } elseif ($reqWidth <= 0) { + $ratio = $origWidth / $origHeight; + return ['width' => (int) ($reqHeight * $ratio), 'height' => $reqHeight]; + } elseif ($reqHeight <= 0) { + $ratio = $origHeight / $origWidth; + return ['width' => $reqWidth, 'height' => (int) ($reqWidth * $ratio)]; + } + switch ($mode) { case 'exact': return ['width' => $reqWidth, 'height' => $reqHeight]; case 'crop': - if ($reqWidth <= 0 || $reqHeight <= 0) { - return ['width' => $origWidth, 'height' => $origHeight]; - } - - $heightRatio = $origHeight / $reqHeight; - $widthRatio = $origWidth / $reqWidth; - $optimalRatio = $heightRatio < $widthRatio ? $heightRatio : $widthRatio; - - return [ - 'width' => (int) round($origWidth / $optimalRatio), - 'height' => (int) round($origHeight / $optimalRatio), - ]; + return ['width' => $reqWidth, 'height' => $reqHeight]; case 'fit': $ratioW = $reqWidth / $origWidth; @@ -1146,32 +1163,32 @@ protected static function calculateResizedDimensions( case 'auto': default: - if ($reqWidth > 0 && $reqHeight > 0) { - if ($origHeight < $origWidth) { - $optimalHeight = (int) round($origHeight * ($reqWidth / $origWidth)); + if ($reqWidth <= 1 && $reqHeight <= 1) { + return ['width' => $origWidth, 'height' => $origHeight]; + } elseif ($reqWidth <= 1) { + $ratio = $origWidth / $origHeight; + return ['width' => (int) ($reqHeight * $ratio), 'height' => $reqHeight]; + } elseif ($reqHeight <= 1) { + $ratio = $origHeight / $origWidth; + return ['width' => $reqWidth, 'height' => (int) ($reqWidth * $ratio)]; + } + + if ($origHeight < $origWidth) { + $optimalHeight = (int) ($origHeight * ($reqWidth / $origWidth)); + return ['width' => $reqWidth, 'height' => $optimalHeight]; + } elseif ($origHeight > $origWidth) { + $optimalWidth = (int) ($origWidth * ($reqHeight / $origHeight)); + return ['width' => $optimalWidth, 'height' => $reqHeight]; + } else { + if ($reqHeight < $reqWidth) { + $optimalHeight = (int) ($origHeight * ($reqWidth / $origWidth)); return ['width' => $reqWidth, 'height' => $optimalHeight]; - } elseif ($origHeight > $origWidth) { - $optimalWidth = (int) round($origWidth * ($reqHeight / $origHeight)); + } elseif ($reqHeight > $reqWidth) { + $optimalWidth = (int) ($origWidth * ($reqHeight / $origHeight)); return ['width' => $optimalWidth, 'height' => $reqHeight]; } else { - if ($reqHeight < $reqWidth) { - $optimalHeight = (int) round($origHeight * ($reqWidth / $origWidth)); - return ['width' => $reqWidth, 'height' => $optimalHeight]; - } elseif ($reqHeight > $reqWidth) { - $optimalWidth = (int) round($origWidth * ($reqHeight / $origHeight)); - return ['width' => $optimalWidth, 'height' => $reqHeight]; - } else { - return ['width' => $reqWidth, 'height' => $reqHeight]; - } + return ['width' => $reqWidth, 'height' => $reqHeight]; } - } elseif ($reqWidth > 0) { - $optimalHeight = (int) round($origHeight * ($reqWidth / $origWidth)); - return ['width' => $reqWidth, 'height' => $optimalHeight]; - } elseif ($reqHeight > 0) { - $optimalWidth = (int) round($origWidth * ($reqHeight / $origHeight)); - return ['width' => $optimalWidth, 'height' => $reqHeight]; - } else { - return ['width' => $origWidth, 'height' => $origHeight]; } } } diff --git a/modules/system/tests/classes/ImageResizerTest.php b/modules/system/tests/classes/ImageResizerTest.php index 6d21ecdd80..a1969f9475 100644 --- a/modules/system/tests/classes/ImageResizerTest.php +++ b/modules/system/tests/classes/ImageResizerTest.php @@ -438,7 +438,6 @@ public function testResizerRedirect() // Clean up the generated image Storage::disk('test_local')->deleteDirectory('resized'); } - public function testCalculateResizedDimensionsMatchesDefaultResizer() { if (!in_array('Cms', Config::get('cms.loadModules', []))) { @@ -448,28 +447,33 @@ public function testCalculateResizedDimensionsMatchesDefaultResizer() $imagePath = base_path('modules/system/tests/fixtures/plugins/database/tester/assets/images/avatar.png'); $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $widthProp = new \ReflectionProperty($resizer, 'width'); + $widthProp->setAccessible(true); + $heightProp = new \ReflectionProperty($resizer, 'height'); + $heightProp->setAccessible(true); + $origW = $widthProp->getValue($resizer); + $origH = $heightProp->getValue($resizer); + $modes = ['exact', 'portrait', 'landscape', 'auto', 'fit', 'crop']; $reqWidth = 200; $reqHeight = 150; - $stormGetDimensions = new \ReflectionMethod($resizer, 'getDimensions'); - $stormGetDimensions->setAccessible(true); - $stormWidth = new \ReflectionProperty($resizer, 'width'); - $stormWidth->setAccessible(true); - $stormHeight = new \ReflectionProperty($resizer, 'height'); - $stormHeight->setAccessible(true); $winterMethod = new \ReflectionMethod(ImageResizer::class, 'calculateResizedDimensions'); $winterMethod->setAccessible(true); foreach ($modes as $mode) { - $resizer->setOptions(['mode' => $mode]); - $expected = $stormGetDimensions->invoke($resizer, $reqWidth, $reqHeight); - $expected = ['width' => (int) $expected[0], 'height' => (int) $expected[1]]; + $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $resizer->resize($reqWidth, $reqHeight, ['mode' => $mode]); + + $imageProp = new \ReflectionProperty($resizer, 'image'); + $imageProp->setAccessible(true); + $resizedImage = $imageProp->getValue($resizer); + $expected = ['width' => imagesx($resizedImage), 'height' => imagesy($resizedImage)]; $calculated = $winterMethod->invoke( null, - $stormWidth->getValue($resizer), - $stormHeight->getValue($resizer), + $origW, + $origH, $reqWidth, $reqHeight, $mode @@ -479,6 +483,93 @@ public function testCalculateResizedDimensionsMatchesDefaultResizer() } } + public function testCalculateResizedDimensionsNormalizesZeroInputs() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $imagePath = base_path('modules/system/tests/fixtures/plugins/database/tester/assets/images/avatar.png'); + + $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $widthProp = new \ReflectionProperty($resizer, 'width'); + $widthProp->setAccessible(true); + $heightProp = new \ReflectionProperty($resizer, 'height'); + $heightProp->setAccessible(true); + $origW = $widthProp->getValue($resizer); + $origH = $heightProp->getValue($resizer); + + $modes = ['exact', 'portrait', 'landscape', 'auto', 'fit', 'crop']; + $winterMethod = new \ReflectionMethod(ImageResizer::class, 'calculateResizedDimensions'); + $winterMethod->setAccessible(true); + + foreach ($modes as $mode) { + $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $resizer->resize(0, $origH, ['mode' => $mode]); + + $imageProp = new \ReflectionProperty($resizer, 'image'); + $imageProp->setAccessible(true); + $resizedImage = $imageProp->getValue($resizer); + $expectedZeroW = ['width' => imagesx($resizedImage), 'height' => imagesy($resizedImage)]; + + $calculatedZeroW = $winterMethod->invoke(null, $origW, $origH, 0, $origH, $mode); + $this->assertSame($expectedZeroW, $calculatedZeroW, "Mode $mode with zero width should match DefaultResizer"); + + $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $resizer->resize($origW, 0, ['mode' => $mode]); + + $imageProp = new \ReflectionProperty($resizer, 'image'); + $imageProp->setAccessible(true); + $resizedImage = $imageProp->getValue($resizer); + $expectedZeroH = ['width' => imagesx($resizedImage), 'height' => imagesy($resizedImage)]; + + $calculatedZeroH = $winterMethod->invoke(null, $origW, $origH, $origW, 0, $mode); + $this->assertSame($expectedZeroH, $calculatedZeroH, "Mode $mode with zero height should match DefaultResizer"); + } + } + + public function testCalculateResizedDimensionsAutoModeOnePixelEdgeCases() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $imagePath = base_path('modules/system/tests/fixtures/plugins/database/tester/assets/images/avatar.png'); + + $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $widthProp = new \ReflectionProperty($resizer, 'width'); + $widthProp->setAccessible(true); + $heightProp = new \ReflectionProperty($resizer, 'height'); + $heightProp->setAccessible(true); + $origW = $widthProp->getValue($resizer); + $origH = $heightProp->getValue($resizer); + + $winterMethod = new \ReflectionMethod(ImageResizer::class, 'calculateResizedDimensions'); + $winterMethod->setAccessible(true); + + $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $resizer->resize(1, 100, ['mode' => 'auto']); + + $imageProp = new \ReflectionProperty($resizer, 'image'); + $imageProp->setAccessible(true); + $resizedImage = $imageProp->getValue($resizer); + $expected1x100 = ['width' => imagesx($resizedImage), 'height' => imagesy($resizedImage)]; + + $calculated1x100 = $winterMethod->invoke(null, $origW, $origH, 1, 100, 'auto'); + $this->assertSame($expected1x100, $calculated1x100, 'Auto mode 1x100 should match DefaultResizer'); + + $resizer = new \Winter\Storm\Database\Attach\Resizer($imagePath); + $resizer->resize(100, 1, ['mode' => 'auto']); + + $imageProp = new \ReflectionProperty($resizer, 'image'); + $imageProp->setAccessible(true); + $resizedImage = $imageProp->getValue($resizer); + $expected100x1 = ['width' => imagesx($resizedImage), 'height' => imagesy($resizedImage)]; + + $calculated100x1 = $winterMethod->invoke(null, $origW, $origH, 100, 1, 'auto'); + $this->assertSame($expected100x1, $calculated100x1, 'Auto mode 100x1 should match DefaultResizer'); + } + public function testFilterGetDimensionsReturnsFallbackForMissingImage() { $this->assertSame(['width' => 0, 'height' => 0], ImageResizer::filterGetDimensions( @@ -552,6 +643,39 @@ public function testFilterGetDimensionsFromAbsoluteResizerUrl() $this->assertSame(100, $dimensions['height']); } + public function testFilterGetDimensionsReturnsFallbackWhenSourceUnavailable() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + $this->copyMedia(); + + $imageResizer = new ImageResizer( + URL::to(MediaLibrary::url('winter.png')), + 100, + 50 + ); + $resizerUrl = $imageResizer->getResizerUrl(); + + $this->assertStringStartsWith('/resizer/', $resizerUrl); + + $disk = Storage::disk('test_local'); + $path = $imageResizer->getConfig()['image']['path']; + $disk->delete($path); + + $dimensions = ImageResizer::filterGetDimensions($resizerUrl); + $this->assertSame(100, $dimensions['width']); + $this->assertSame(50, $dimensions['height']); + + $disk->put($path, file_get_contents(base_path('modules/system/tests/fixtures/media/winter.png'))); + + $dimensions = ImageResizer::filterGetDimensions($resizerUrl); + $this->assertSame(100, $dimensions['width']); + $this->assertSame(100, $dimensions['height']); + } + protected function setUpStorage() { $this->app->useStoragePath(base_path('storage/temp'));