From 498d547f55fbea82693ff4e57fa7fa3480a1a3f8 Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Thu, 1 Oct 2026 13:53:56 +0000 Subject: [PATCH 1/2] fix(system): predict resizer dimensions for cold-cache URLs --- .github/workflows/tests.yml | 2 +- modules/system/classes/ImageResizer.php | 477 ++++++++++- .../system/tests/classes/ImageResizerTest.php | 807 +++++++++++++++++- 3 files changed, 1256 insertions(+), 30 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 398fe5a813..c1b0ad8a4f 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -100,7 +100,7 @@ jobs: runs-on: ${{ matrix.operatingSystem }} name: ${{ matrix.operatingSystem }} / PHP ${{ matrix.phpVersion }} env: - extensions: curl, fileinfo, gd, mbstring, openssl, pdo, pdo_sqlite, sqlite3, xml, zip + extensions: curl, exif, fileinfo, gd, mbstring, openssl, pdo, pdo_sqlite, sqlite3, xml, zip key: winter-cms-cache-develop steps: - name: Checkout changes diff --git a/modules/system/classes/ImageResizer.php b/modules/system/classes/ImageResizer.php index 5f178f4c43..9e131b6494 100644 --- a/modules/system/classes/ImageResizer.php +++ b/modules/system/classes/ImageResizer.php @@ -8,6 +8,7 @@ use Storage; use Exception; use SystemException; +use Str; use File as FileHelper; use Illuminate\Filesystem\FilesystemAdapter; use System\Models\File as SystemFileModel; @@ -426,8 +427,26 @@ public function crop(): void /** * Get the internal temporary drirectory and ensure it exists + * + * NOTE: the copy made to measure a remote source now goes through + * getResizerTempPath(), which returns the same directory but is not overridable + * per instance. A subclass that replaced this method to relocate the resizer's + * working directory no longer affects that copy; override getResizerTempPath() + * instead. */ public function getTempPath(): string + { + return static::getResizerTempPath(); + } + + /** + * Get the internal temporary directory and ensure it exists + * + * Kept separate from getTempPath() because that method is part of the public + * API and therefore cannot be made static without breaking subclasses that + * override it. + */ + protected static function getResizerTempPath(): string { $path = temp_path() . '/resizer'; @@ -438,6 +457,28 @@ public function getTempPath(): string return $path; } + /** + * Get the time to live used for the cached resizer configuration + * + * This is a bound on growth, not a correctness mechanism, and it is deliberately not + * applied to the source or dimension caches. Those are immutable for their key + * because the identifier embeds the source image's mtime, so a changed source lands + * on a different key and a TTL could only buy a needless re-read -- a full file + * transfer on a remote disk. The exception is a filemodel source, whose identifier + * does not embed the configuration at all; there the entries carry the mtime + * alongside the dimensions and validate themselves. + * + * The residual failure it leaves is worth stating. A resizer URL that is never + * visited does not have its resize performed, so once this expires the URL stops + * resolving. Retention lengthens the window from a single fetch to a day rather than + * removing it, because bounding growth and keeping a promise for ever are not both + * available at once. + */ + protected static function dimensionCacheTtl(): \DateTimeInterface + { + return now()->addDay(); + } + /** * Stores the current source image in the temp directory and returns the path to it * @@ -816,7 +857,11 @@ public function storeConfig(): void { // If the image hasn't been resized yet, then store the config data for the resizer to use if (!$this->isResized()) { - Cache::put(static::CACHE_PREFIX . $this->getIdentifier(), $this->getConfig()); + Cache::put( + static::CACHE_PREFIX . $this->getIdentifier(), + $this->getConfig(), + static::dimensionCacheTtl() + ); } } @@ -840,15 +885,392 @@ public static function fromIdentifier(string $identifier): self $resizer = new static($config['image'], $config['width'], $config['height'], $config['options']); - // Remove the data from the cache only after successfully instantiating the resizer - // in order to make it easier to debug should any issues occur during the instantiation - // since the browser will "steal" the configuration with the first request it makes - // if we pull the configuration data out immediately. - Cache::forget($cacheKey); - + // The configuration is left in the cache rather than consumed here. It used to be + // evicted on the grounds that the browser "stealing" it with the first request + // could not race a second visitor, which is a real fault but a poor trade: a page + // holding a resizer URL -- a cached fragment, a client-side template, a URL stored + // in content -- then stopped resolving after the first visitor. + // + // Nothing sensitive is exposed by keeping it. The contents cannot be recovered + // from the identifier, which is a one-way HMAC over the resized path, and the + // resized path carries only a hash of the configuration. What settles it is that + // anyone holding the identifier already holds a self-authenticating /resizer/ URL + // -- the route re-verifies that pairing in getValidResizedUrl() -- so the entry + // is not a capability in the first place, and retaining it reveals nothing the + // URL did not. + // + // The window is lengthened rather than removed. A resizer URL nobody ever visits + // never has its resize performed, so once the TTL expires the URL stops + // resolving; see dimensionCacheTtl(). return $resizer; } + /** + * Calculate the size of an image constrained to a fixed height + * + * Mirrors Resizer::getSizeByFixedHeight(), which returns an unrounded float that + * GD truncates when it allocates the canvas. Rounding it here instead is a + * one-pixel divergence from the real resizer in a large fraction of cases. + */ + protected static function sizeByFixedHeight($newHeight, int $origWidth, int $origHeight): float + { + return $newHeight * ($origWidth / $origHeight); + } + + /** + * Calculate the size of an image constrained to a fixed width + * + * Mirrors Resizer::getSizeByFixedWidth(), which returns an unrounded float. + * @see static::sizeByFixedHeight() + */ + protected static function sizeByFixedWidth($newWidth, int $origWidth, int $origHeight): float + { + return $newWidth * ($origHeight / $origWidth); + } + + /** + * Mirror of Resizer::getSizeByAuto() + * + * The order of the two multiplications is not interchangeable with the other + * algebraic form; across roughly 44 million bound combinations about 0.18% of + * them truncate to a different pixel, so this is transcribed rather than + * re-derived. + */ + protected static function autoDimensions($newWidth, $newHeight, int $origWidth, int $origHeight): array + { + if ($newWidth <= 1 && $newHeight <= 1) { + $newWidth = $origWidth; + $newHeight = $origHeight; + } elseif ($newWidth <= 1) { + $newWidth = static::sizeByFixedHeight($newHeight, $origWidth, $origHeight); + } elseif ($newHeight <= 1) { + $newHeight = static::sizeByFixedWidth($newWidth, $origWidth, $origHeight); + } + + if ($origHeight < $origWidth || ($origHeight === $origWidth && $newHeight < $newWidth)) { + return [ + 'width' => (int) $newWidth, + 'height' => (int) static::sizeByFixedWidth($newWidth, $origWidth, $origHeight), + ]; + } + + if ($origHeight > $origWidth || ($origHeight === $origWidth && $newHeight > $newWidth)) { + return [ + 'width' => (int) static::sizeByFixedHeight($newHeight, $origWidth, $origHeight), + 'height' => (int) $newHeight, + ]; + } + + return ['width' => (int) $newWidth, 'height' => (int) $newHeight]; + } + + /** + * Calculate the dimensions the resizer will produce for the given bounds + * + * This deliberately duplicates Resizer::getDimensions() rather than calling it, + * because that method is protected and widening its visibility is a backwards + * compatibility change on a released package. The duplication is safe only as + * long as testCalculateResizedDimensionsMatchesResizerOutput passes: that test + * compares against the real Resizer::resize() output and fails if the upstream + * formula changes. Unification is tracked as a follow-up against Storm, which + * cannot ship in the same change because the core pins winter/storm to a + * dev-develop branch that needs its own release first. + * + * Two details of the upstream implementation are easy to get wrong and are + * therefore spelled out here: + * + * 1. The requested bounds are sanitised before the mode is dispatched on, not + * inside each mode. A single-bound request is filled in from the source + * first, which can push both bounds under the "less than one pixel" + * threshold and make the result the original size. + * 2. Only "fit" rounds. "portrait", "landscape" and the sanitisation all rely on + * GD truncating an unrounded float. + * + * An unrecognised mode is assumed to be "auto". That is the least-wrong answer + * rather than a correct one: a custom mode means a custom resizer, whose real + * output cannot be predicted from here. + * + * @return array ['width' => int, 'height' => int] + */ + protected static function calculateResizedDimensions( + int $origWidth, + int $origHeight, + int $reqWidth, + int $reqHeight, + string $mode + ): array { + $dimensions = static::computeResizedDimensions($origWidth, $origHeight, $reqWidth, $reqHeight, $mode); + + // GD cannot allocate a canvas with a sub-pixel dimension: it truncates the + // float to zero and throws. Claiming a size smaller than a pixel would be + // reporting an image that cannot be produced, so report it as unknown. This + // is also the exact condition under which the resizer refuses, so the two + // agree rather than one inventing a size the other rejects. + if ($dimensions['width'] < 1 || $dimensions['height'] < 1) { + return ['width' => 0, 'height' => 0]; + } + + return $dimensions; + } + + /** + * Apply the resize mode to a pair of sanitised bounds + * + * @see static::calculateResizedDimensions() + * @return array ['width' => int, 'height' => int] + */ + protected static function computeResizedDimensions( + int $origWidth, + int $origHeight, + int $reqWidth, + int $reqHeight, + string $mode + ): array { + if ($origWidth <= 0 || $origHeight <= 0) { + return ['width' => $reqWidth, 'height' => $reqHeight]; + } + + // Mirrors the sanitisation at the top of Resizer::resize(). It runs for every + // mode and must precede the switch. + if (!$reqWidth && !$reqHeight) { + $newWidth = $origWidth; + $newHeight = $origHeight; + } elseif (!$reqWidth) { + $newHeight = $reqHeight; + $newWidth = static::sizeByFixedHeight($newHeight, $origWidth, $origHeight); + } elseif (!$reqHeight) { + $newWidth = $reqWidth; + $newHeight = static::sizeByFixedWidth($newWidth, $origWidth, $origHeight); + } else { + $newWidth = $reqWidth; + $newHeight = $reqHeight; + } + + switch ($mode) { + case 'exact': + return ['width' => (int) $newWidth, 'height' => (int) $newHeight]; + + case 'crop': + // The "crop" mode builds a larger intermediate canvas via + // getOptimalCrop() and then crops to exactly the requested box, so the + // image that is finally written is the request itself and not the + // canvas. + return ['width' => (int) $newWidth, 'height' => (int) $newHeight]; + + case 'portrait': + return [ + 'width' => (int) static::sizeByFixedHeight($newHeight, $origWidth, $origHeight), + 'height' => (int) $newHeight, + ]; + + case 'landscape': + return [ + 'width' => (int) $newWidth, + 'height' => (int) static::sizeByFixedWidth($newWidth, $origWidth, $origHeight), + ]; + + case 'fit': + $effectiveRatio = min($newWidth / $origWidth, $newHeight / $origHeight); + return [ + 'width' => (int) round($origWidth * $effectiveRatio), + 'height' => (int) round($origHeight * $effectiveRatio), + ]; + + case 'auto': + return static::autoDimensions($newWidth, $newHeight, $origWidth, $origHeight); + + default: + return static::autoDimensions($newWidth, $newHeight, $origWidth, $origHeight); + } + } + + /** + * Read the dimensions of a source image, and, if possible, its local path + * + * @param mixed $disk A FilesystemAdapter or a filesystem configuration name + * @return array ['width' => int, 'height' => int] + */ + protected static function readSourceDimensions($disk, string $path): array + { + $tempPath = null; + + try { + if (FileHelper::isLocalDisk($disk)) { + $localPath = $disk->getPathPrefix() . $path; + } else { + // A remote disk has to be copied somewhere readable by GD before it + // can be measured. tempnam() reserves the name atomically, unlike + // uniqid(), so two concurrent lookups cannot select the same path and + // then unlink each other's copy. + $tempPath = tempnam(static::getResizerTempPath(), 'src'); + FileHelper::put($tempPath, $disk->get($path)); + $localPath = $tempPath; + } + + $size = @getimagesize($localPath); + if ($size === false) { + return ['width' => 0, 'height' => 0]; + } + + $origWidth = (int) $size[0]; + $origHeight = (int) $size[1]; + + // An orientation of 6 or 8 means the pixels are stored rotated. The + // resizer reports the displayed dimensions, so match that here. Only + // JPEG carries EXIF, and only JPEG goes down this path upstream. + if (($size['mime'] ?? null) === 'image/jpeg' && function_exists('exif_read_data')) { + $exif = @exif_read_data($localPath); + if (!empty($exif['Orientation']) && in_array($exif['Orientation'], [6, 8], true)) { + [$origWidth, $origHeight] = [$origHeight, $origWidth]; + } + } + + return ['width' => $origWidth, 'height' => $origHeight]; + } finally { + if ($tempPath !== null && file_exists($tempPath)) { + @unlink($tempPath); + } + } + } + + /** + * Resolve the identifier of a system resizer URL + * + * Returns null when the provided string is not a resizer URL. Both relative and + * absolute URLs are accepted, since a forced link policy rewrites the resizer URL + * to an absolute one. parse_url() is deliberately not used for the scheme-less case + * because it reads a Windows drive letter as a scheme. + * + * @return string|null The resizer identifier, or null if this is not a resizer URL + */ + protected static function resizerUrlSegments(string $url): ?string + { + $path = $url; + + if (Str::startsWith($url, ['http://', 'https://'])) { + $path = (string) parse_url($url, PHP_URL_PATH); + } elseif (Str::startsWith($url, '//')) { + $path = (string) parse_url('https:' . $url, PHP_URL_PATH); + } + + $segments = explode('/', ltrim(static::normalizePath($path), '/')); + + // Not required to be the first segment: under a subfolder install with a forced + // link policy, Url::to() produces https://host/sub/resizer//..., so the route + // is not at index zero. Searching rather than indexing keeps that deployment + // working, and requiring a valid identifier at the next position is what stops + // an ordinary media path containing "resizer" from matching. + $position = array_search('resizer', $segments, true); + + if ( + $position !== false + && isset($segments[$position + 1]) + && static::isValidIdentifier($segments[$position + 1]) + ) { + return $segments[$position + 1]; + } + + return null; + } + + /** + * Resolve the dimensions of a cached resizer configuration + * + * @return array ['width' => int, 'height' => int] + */ + protected static function computeCachedDimensions(string $identifier): array + { + $config = Cache::get(static::CACHE_PREFIX . $identifier); + + if (!is_array($config)) { + return ['width' => 0, 'height' => 0]; + } + + return static::dimensionsForConfig($config, $identifier); + } + + /** + * Resolve the dimensions described by a resizer configuration + * + * A zero pair is returned when the configuration is incomplete or the source + * cannot be measured. A partial pair is never returned: a width with a zero + * height reaches an HTML attribute and collapses the layout, whereas a zero pair + * reads as "unknown", which is what it is. + * + * @return array ['width' => int, 'height' => int] + */ + protected static function dimensionsForConfig($config, string $identifier): array + { + if ( + !is_array($config) + || !isset($config['width'], $config['height'], $config['options']['mode']) + || !isset($config['image']['disk'], $config['image']['path']) + ) { + return ['width' => 0, 'height' => 0]; + } + + $cacheKey = static::CACHE_PREFIX . $identifier; + $sourceCacheKey = $cacheKey . '.source'; + $dimensionsCacheKey = $cacheKey . '.dimensions'; + + // For every source whose resized path is derived from the configuration, the + // identifier embeds the source's mtime, so this entry is immutable for its key + // and keeping it for ever costs nothing. A filemodel source is the exception: + // its resized path is the thumb filename, which carries the attachment id and + // the requested bounds and nothing at all about the file's contents, so the + // identifier does not move when the bytes do. Recording the mtime alongside the + // dimensions makes the entry self-validating for those too, for one comparison. + // + // It cannot cover the case where the cached configuration itself is out of date, + // because storeConfig() skips writing once the thumb exists. A caller still + // holding an old /resizer/ URL for a filemodel whose bytes were replaced in + // place therefore keeps the first answer. That is pre-existing and narrow. + $sourceMtime = isset($config['image']['mtime']) ? (int) $config['image']['mtime'] : null; + $cachedSource = Cache::get($sourceCacheKey); + $sourceIsCurrent = is_array($cachedSource) + && ($sourceMtime === null || ($cachedSource['mtime'] ?? null) === $sourceMtime); + + if (!$sourceIsCurrent) { + // The dimensions are derived from the source, so they have to move with it + Cache::forget($dimensionsCacheKey); + + $disk = $config['image']['disk']; + if (is_string($disk)) { + $disk = Storage::disk($disk); + } + + $cachedSource = static::readSourceDimensions($disk, (string) $config['image']['path']); + + // Only a successful read is cached. The guard has to sit outside the write + // rather than inside a remember() closure, because remember() stores + // whatever the closure returns, so a transient failure would otherwise + // become a zero dimension for ever. + if ($cachedSource['width'] > 0 && $cachedSource['height'] > 0) { + $cachedSource['mtime'] = $sourceMtime; + Cache::forever($sourceCacheKey, $cachedSource); + } + } + + if ($cachedSource['width'] <= 0 || $cachedSource['height'] <= 0) { + return ['width' => 0, 'height' => 0]; + } + + // Forever for the same reason as the source cache above: pure function of the key, + // and dropped above whenever that key had to be re-read + return Cache::rememberForever( + $dimensionsCacheKey, + function () use ($config, $cachedSource) { + return static::calculateResizedDimensions( + (int) $cachedSource['width'], + (int) $cachedSource['height'], + (int) $config['width'], + (int) $config['height'], + (string) $config['options']['mode'] + ); + } + ); + } + /** * Check the provided encoded URL to verify its signature and return the decoded URL * @@ -902,6 +1324,11 @@ public static function filterGetUrl($image, $width = null, $height = null, $opti /** * Gets the dimensions of the provided image file + * + * A system resizer URL is resolved from its cached configuration rather than by + * reading the file, because on a cold cache the resized image has not been written + * yet and there is nothing on disk to measure. + * * NOTE: Doesn't currently support being passed a FileModel image that has already been resized * * @param mixed $image Supported values below: @@ -909,30 +1336,26 @@ public static function filterGetUrl($image, $width = null, $height = null, $opti * instance of Winter\Storm\Database\Attach\File, * string containing URL or path accessible to the application's filesystem manager * @throws SystemException If the provided input was unable to be processed + * @return array ['width' => int, 'height' => int] */ public static function filterGetDimensions($image): array { - $resizer = new static($image); - - return Cache::rememberForever(static::CACHE_PREFIX . 'dimensions.' . $resizer->getIdentifier(), function () use ($resizer) { - // Prepare the local file for assessment - $tempPath = $resizer->getLocalTempPath(); - $dimensions = []; - - // 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 (is_string($image) && ($identifier = static::resizerUrlSegments($image)) !== null) { + return static::computeCachedDimensions($identifier); + } - // Cleanup afterwards - @unlink($tempPath); + // Anything that cannot be identified still raises, as it always has. Swallowing + // it here would turn a template pointing at a missing image from a visible error + // into a silently rendered width="0", and that signal is worth more than the + // convenience. Only a resizer URL -- handled above, where there is no file to + // identify -- reports unknown. + $resizer = new static($image); + $config = $resizer->getConfig(); - return $dimensions; - }); + // The configuration is handed over directly rather than written to the cache + // and read back. Writing it here would leave an entry that no resizer request + // can ever consume, because storeConfig() is the only writer that a resizer URL + // depends on and it has already run by the time such a URL exists. + return static::dimensionsForConfig($config, $resizer->getIdentifier()); } } diff --git a/modules/system/tests/classes/ImageResizerTest.php b/modules/system/tests/classes/ImageResizerTest.php index 26c379b73e..4d0dba781c 100644 --- a/modules/system/tests/classes/ImageResizerTest.php +++ b/modules/system/tests/classes/ImageResizerTest.php @@ -5,6 +5,8 @@ use Backend\Facades\Backend; use Cms\Classes\Controller as CmsController; use Cms\Classes\Theme; +use Cache; +use File; use Config; use DMS\PHPUnitExtensions\ArraySubset\ArraySubsetAsserts; use Event; @@ -14,7 +16,12 @@ use System\Models\File as FileModel; use System\Tests\Bootstrap\PluginTestCase; use URL; +use Throwable; +use GdImage; +use ReflectionMethod; +use ReflectionProperty; use Winter\Storm\Exception\SystemException; +use Winter\Storm\Database\Attach\Resizer as DefaultResizer; class ImageResizerTest extends PluginTestCase { @@ -38,6 +45,8 @@ public function tearDown(): void { $this->removeMedia(); + File::deleteDirectory(storage_path('temp/resizer-parity')); + Config::set('cms.themesPath', $this->originalThemesPath); ImageResizer::flushAvailableSources(); @@ -436,6 +445,788 @@ public function testResizerRedirect() Storage::disk('test_local')->deleteDirectory('resized'); } + /** + * Assert that the predicted dimensions match what the resizer actually produces + * + * The existing committed fixtures are square, and at a ratio of 1.0 every mode + * agrees with every other: the round-versus-truncate divergence collapses and the + * landscape and portrait branches of the auto calculation are unreachable. A green + * suite over square sources therefore establishes nothing about the arithmetic, so + * the sources here are generated at the ratios that actually exercise it. + * + * Sources are generated rather than committed because a dimension assertion does + * not care about the picture, and a 3x7 fixture costs 200 bytes rather than 40 KiB. + * Anything that needs to see detail -- a resampling quality assertion -- needs a + * photographic source instead. + * + * Cases where the resizer itself refuses are compared too, by asserting that the + * prediction does not claim a size. A harness that catches and skips them hides + * exactly the degenerate boundaries this test exists to cover. + * + * @return void + */ + public function testCalculateResizedDimensionsMatchesResizerOutput() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $modes = ['exact', 'portrait', 'landscape', 'auto', 'fit', 'crop']; + $requests = [ + [200, 150], [100, 100], [150, 200], [50, 200], [200, 50], + [0, 0], [0, 150], [200, 0], [1, 1], [0, 1], [1, 0], + [1, 100], [100, 1], [1, 50], [50, 1], [37, 91], [640, 480], + ]; + + $compared = 0; + $refused = 0; + $failures = []; + $refusedWithoutPrediction = []; + $predictionTypeFailures = []; + + foreach ($this->createParityFixtures() as $fixture) { + $size = getimagesize($fixture); + $origWidth = $size[0]; + $origHeight = $size[1]; + + foreach ($modes as $mode) { + foreach ($requests as $request) { + [$reqWidth, $reqHeight] = $request; + + $predicted = $this->predictDimensions($origWidth, $origHeight, $reqWidth, $reqHeight, $mode); + + if (!is_int($predicted['width']) || !is_int($predicted['height'])) { + $predictionTypeFailures[] = sprintf( + '%s mode=%s req=%dx%d produced %s', + basename($fixture), + $mode, + $reqWidth, + $reqHeight, + gettype($predicted['width']) . '/' . gettype($predicted['height']) + ); + } + + $options = ['mode' => $mode]; + if ($mode === 'crop') { + $options['offset'] = [0, 0]; + } + + $actual = $this->resizeAndMeasure($fixture, $reqWidth, $reqHeight, $options); + + if ($actual === null) { + // The resizer refused to produce an image. The prediction must + // not claim a size for it. + $refused++; + if ($predicted['width'] > 0 || $predicted['height'] > 0) { + $refusedWithoutPrediction[] = sprintf( + 'resizer refused but predicted %dx%d: %s mode=%s req=%dx%d', + $predicted['width'], + $predicted['height'], + basename($fixture), + $mode, + $reqWidth, + $reqHeight + ); + } + continue; + } + + $compared++; + if ($actual !== $predicted) { + $failures[] = sprintf( + '%s (%dx%d) mode=%s req=%dx%d: predicted %dx%d, produced %dx%d', + basename($fixture), + $origWidth, + $origHeight, + $mode, + $reqWidth, + $reqHeight, + $predicted['width'], + $predicted['height'], + $actual['width'], + $actual['height'] + ); + } + } + } + } + + $this->assertGreaterThan(0, $compared, 'No comparisons were made; the fixtures or requests are empty.'); + $this->assertSame([], $predictionTypeFailures, 'Dimensions must be integers, not floats.'); + $this->assertSame([], $failures, "{$compared} comparisons, {$refused} refused by the resizer."); + $this->assertSame([], $refusedWithoutPrediction, "{$refused} cases refused by the resizer."); + } + + /** + * A resized image has to report the same dimensions through the Twig filters as the + * file the resizer route writes + * + * The cold path is covered by the parity test; this is the state every image is in + * after its first browser load, where getUrl() returns the static path and the + * resizer configuration is never written by the URL builder. + * + * @return void + */ + public function testSteadyStateResizedUrlStillReportsDimensions() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + Config::set('cms.storage.resized', [ + 'disk' => 'test_local', + 'folder' => 'resized', + // The local adapter's url() ignores a non-default root and returns + // /storage//..., so the configured public path has to match that + // for a resized image to be recognised as the 'resized' source at all. + 'path' => '/storage/resized', + ]); + + // Deliberately not square: on a square source every mode predicts the same + // thing and this test would pass even if the arithmetic were wrong. + $imageResizer = new ImageResizer($this->createMediaImage('steady.png', 400, 300), 200, 150); + $identifier = $imageResizer->getIdentifier(); + + // Cold: the resizer filter returns a /resizer/ URL, and nothing has been + // written yet, so the prediction has to come from the source dimensions + $resizerUrl = $imageResizer->getResizerUrl(); + $cold = ImageResizer::filterGetDimensions($resizerUrl); + $this->assertSame(200, $cold['width']); + $this->assertSame(150, $cold['height']); + + // Produce the image the way the resizer route does + ImageResizer::fromIdentifier($identifier)->resize(); + + $produced = getimagesize(Storage::disk('test_local')->path($imageResizer->getPathToResizedImage())); + $this->assertSame(200, $produced[0]); + $this->assertSame(150, $produced[1]); + + // Steady state: the file exists, so getUrl() bypasses getResizerUrl() and + // storeConfig() never runs again for this image + // Steady state: the resizer filter now returns the static resized path, and + // getUrl() bypasses getResizerUrl() so storeConfig() never runs again + $this->assertTrue($imageResizer->isResized()); + $this->assertSame($imageResizer->getResizedUrl(), $imageResizer->getUrl()); + $steady = ImageResizer::filterGetDimensions($imageResizer->getUrl()); + + $this->assertSame( + ['width' => $produced[0], 'height' => $produced[1]], + $steady, + 'A resized image must still report its dimensions after the first browser load.' + ); + + Storage::disk('test_local')->deleteDirectory('resized'); + } + + /** + * A failed source read must not be cached as a zero dimension + * + * @return void + */ + public function testFilterGetDimensionsDoesNotCacheAFailedRead() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + Config::set('cms.storage.resized', [ + 'disk' => 'test_local', + 'folder' => 'resized', + 'path' => '/storage/resized', + ]); + + $url = $this->createMediaImage('transient.png', 400, 300); + $imageResizer = new ImageResizer($url, 200, 150); + $identifier = $imageResizer->getIdentifier(); + $config = $imageResizer->getConfig(); + + // Captured while the source still exists: getResizerUrl() resolves the + // identifier, which reads the source's mtime. + $resizerUrl = $imageResizer->getResizerUrl(); + + // Seed the configuration directly so the assertion does not depend on + // getUrl() having written it, then remove the source out from under it. + Cache::put(ImageResizer::CACHE_PREFIX . $identifier, $config, now()->addDay()); + Storage::disk('test_local')->delete($config['image']['path']); + + $this->assertSame( + ['width' => 0, 'height' => 0], + ImageResizer::filterGetDimensions($resizerUrl), + 'An unmeasurable source reports unknown rather than a partial pair.' + ); + + // Restore the source. The failure must not have reached the source cache, so + // the next read recovers with no manual cache invalidation at all. + $this->createMediaImage('transient.png', 400, 300); + + $this->assertSame( + ['width' => 200, 'height' => 150], + ImageResizer::filterGetDimensions($resizerUrl), + 'A failed read must not become a cached zero dimension.' + ); + } + + /** + * The resizer configuration survives the request that consumes it + * + * The entry used to be evicted by fromIdentifier() so that a second visitor could + * not race the first. That made any page holding a resizer URL stop resolving after + * the first request, which is the case this whole change exists to fix. + * + * @return void + */ + public function testResizerConfigSurvivesTheRequestThatConsumesIt() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + Config::set('cms.storage.resized', [ + 'disk' => 'test_local', + 'folder' => 'resized', + 'path' => '/storage/resized', + ]); + + $imageResizer = new ImageResizer($this->createMediaImage('shared.png', 400, 300), 200, 150); + $identifier = $imageResizer->getIdentifier(); + $resizerUrl = $imageResizer->getResizerUrl(); + + $this->assertTrue( + Cache::has(ImageResizer::CACHE_PREFIX . $identifier), + 'The URL builder is expected to write the configuration.' + ); + + // A second visitor hits the same URL. The configuration is still resolvable, + // so the dimensions still resolve too. + ImageResizer::fromIdentifier($identifier)->resize(); + + $this->assertTrue( + Cache::has(ImageResizer::CACHE_PREFIX . $identifier), + 'The configuration must not be consumed by the request that uses it.' + ); + + $this->assertSame( + ['width' => 200, 'height' => 150], + ImageResizer::filterGetDimensions($resizerUrl), + 'A resizer URL must keep resolving after a previous request consumed it.' + ); + + Storage::disk('test_local')->deleteDirectory('resized'); + } + + /** + * An EXIF orientation of 6 or 8 is reflected in the reported dimensions + * + * The resizer reports the displayed dimensions rather than the stored pixel + * buffer, so a source tagged as rotated has to be reported swapped or every + * portrait and landscape calculation is computed against the wrong orientation. + * + * GD cannot write an EXIF segment, so the fixture is assembled here rather than + * committed. The structural assertions run everywhere; the orientation assertion + * only runs where the exif extension is present, which is why it was added to the + * CI extension list for this job. + * + * @return void + */ + public function testExifOrientationIsReflectedInTheDimensions() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + Config::set('cms.storage.resized', [ + 'disk' => 'test_local', + 'folder' => 'resized', + 'path' => '/storage/resized', + ]); + + $url = $this->createRotatedJpeg('rotated.jpg', 400, 225, 6); + $this->assertExifSegmentIsWellFormed($url, 6, 400, 225); + + $imageResizer = new ImageResizer($url, 200, 150); + $resizerUrl = $imageResizer->getResizerUrl(); + $config = $imageResizer->getConfig(); + $identifier = $imageResizer->getIdentifier(); + Cache::put(ImageResizer::CACHE_PREFIX . $identifier, $config, now()->addDay()); + + if (!function_exists('exif_read_data')) { + $this->markTestSkipped('The exif extension is not available.'); + } + + // Stored 400x225, displayed as 225x400 once the orientation is applied. + // Requesting 200x150 in auto mode against a portrait source produces + // 84x150 (150 constrained by height, 225 * 150/400 wide). Without the swap + // the source reads as a 400x225 landscape and the answer is 200x112 instead, + // so this pair is what distinguishes the two. + $dimensions = ImageResizer::filterGetDimensions($resizerUrl); + + $this->assertSame( + ['width' => 84, 'height' => 150], + $dimensions, + 'An image tagged with orientation 6 must be reported in its displayed orientation.' + ); + } + + /** + * A source on a symlinked disk is still measured correctly + * + * getConfig() rewrites a local disk's path prefix to its realpath so that atomic + * deployments survive a changing base path, and the dimension reader has to + * resolve the same file through that rewrite. + * + * @return void + */ + public function testSymlinkedSourceIsMeasuredCorrectly() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + // The storage path has to be in place before the fixture is written, or the + // fixture lands outside the disk root that is about to be configured. + $this->setUpStorage(); + + $real = storage_path('app/real-media'); + $link = storage_path('app/linked-media'); + + if (!is_dir($real)) { + mkdir($real, 0777, true); + } + + $image = imagecreatetruecolor(400, 300); + imagefill($image, 0, 0, imagecolorallocate($image, 40, 80, 120)); + imagepng($image, $real . DIRECTORY_SEPARATOR . 'linked.png'); + imagedestroy($image); + + if (is_link($link) || file_exists($link)) { + unlink($link); + } + + if (!@symlink($real, $link)) { + $this->markTestSkipped('This platform does not allow creating symlinks here.'); + } + + Config::set('filesystems.disks.test_symlink', [ + 'driver' => 'local', + 'root' => storage_path('app'), + ]); + + Config::set('cms.storage.media', [ + 'disk' => 'test_symlink', + 'folder' => 'linked-media', + // The local adapter's url() ignores a non-default root, so the configured + // public path has to match what it produces for the source to be found. + 'path' => '/storage/linked-media', + ]); + + Config::set('cms.storage.resized', [ + 'disk' => 'test_local', + 'folder' => 'resized', + 'path' => '/storage/resized', + ]); + + // The available sources are memoised from the first call, so they have to be + // dropped after repointing the media disk at a different one. + ImageResizer::flushAvailableSources(); + + $imageResizer = new ImageResizer('/storage/linked-media/linked.png', 200, 150); + $resizerUrl = $imageResizer->getResizerUrl(); + $identifier = $imageResizer->getIdentifier(); + Cache::put(ImageResizer::CACHE_PREFIX . $identifier, $imageResizer->getConfig(), now()->addDay()); + + $this->assertSame( + ['width' => 200, 'height' => 150], + ImageResizer::filterGetDimensions($resizerUrl), + 'A symlinked source must resolve to the same file the resizer would read.' + ); + } + + /** + * Changing the source invalidates the cached dimensions + * + * This is the invariant the whole cache lifetime rests on. The identifier is an HMAC + * over the resized path, which hashes the configuration, which carries the source's + * mtime -- so a changed source produces a different identifier and therefore a + * different cache key. Without it, dropping the eviction in fromIdentifier() would + * serve a stale size indefinitely. + * + * @return void + */ + public function testSourceChangeInvalidatesCachedDimensions() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + Config::set('cms.storage.resized', [ + 'disk' => 'test_local', + 'folder' => 'resized', + 'path' => '/storage/resized', + ]); + ImageResizer::flushAvailableSources(); + + $url = $this->createMediaImage('changing.png', 400, 300); + $path = storage_path('app/media/changing.png'); + + $first = new ImageResizer($url, 200, 150); + $firstUrl = $first->getResizerUrl(); + $firstIdentifier = $first->getIdentifier(); + + $this->assertSame( + ['width' => 200, 'height' => 150], + ImageResizer::filterGetDimensions($firstUrl), + 'A 400x300 source asked for 200x150 resolves to 200x150.' + ); + + // A repeated read has to come from the cache and agree + $this->assertSame( + ['width' => 200, 'height' => 150], + ImageResizer::filterGetDimensions($firstUrl), + 'A repeated read returns the same answer.' + ); + + // Replace the source with a different shape. The mtime is moved forward + // explicitly because the timestamp is second-granularity on many filesystems, and + // a rewrite inside the same second would legitimately keep the same identifier. + // + // Square, deliberately. A replacement sharing the original's aspect ratio would + // make this assertion vacuous: auto mode preserves the ratio, so 600x450 against + // a 400x300 original predicts exactly what the original predicted, and the test + // would pass whether or not anything was recomputed. + $replacement = imagecreatetruecolor(500, 500); + imagefill($replacement, 0, 0, imagecolorallocate($replacement, 200, 40, 40)); + imagepng($replacement, $path); + imagedestroy($replacement); + touch($path, time() + 10); + + $second = new ImageResizer($url, 200, 150); + + $this->assertNotSame( + $firstIdentifier, + $second->getIdentifier(), + 'A changed source must produce a different identifier, or the cache cannot expire.' + ); + + // No arithmetic assertion here, deliberately. For a media source a changed mtime + // produces a different identifier, so the second read hits a different cache key + // and could not have served a stale entry even in principle -- an assertion about + // recomputation would pass unconditionally. The staleness that can actually happen + // needs the identifier to stay put while the source moves, which is the filemodel + // case, covered by testSourceCacheRevalidatesWhenTheConfigurationMtimeMoves. + } + + /** + * The source entry re-validates when the configuration's mtime moves under a fixed key + * + * This is the case the identifier cannot catch. For a filemodel source the resized + * path is the thumb filename -- attachment id and requested bounds, nothing about the + * file's contents -- so the identifier does not move when the bytes do, and a cached + * source entry would otherwise be trusted for ever. Here the situation is simulated + * directly: the key is held fixed and the cached configuration is given a newer mtime, + * which is exactly what a re-running storeConfig() would do. + * + * @return void + */ + public function testSourceCacheRevalidatesWhenTheConfigurationMtimeMoves() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->setUpStorage(); + Config::set('cms.storage.resized', [ + 'disk' => 'test_local', + 'folder' => 'resized', + 'path' => '/storage/resized', + ]); + ImageResizer::flushAvailableSources(); + + $url = $this->createMediaImage('revalidate.png', 400, 300); + $imageResizer = new ImageResizer($url, 200, 150); + $identifier = $imageResizer->getIdentifier(); + $cacheKey = ImageResizer::CACHE_PREFIX . $identifier; + $resizerUrl = $imageResizer->getResizerUrl(); + + $this->assertSame( + ['width' => 200, 'height' => 150], + ImageResizer::filterGetDimensions($resizerUrl), + 'A 400x300 source asked for 200x150 resolves to 200x150.' + ); + + // Replace the file with a square one, and refresh the configuration's mtime + // without moving the identifier. + $path = storage_path('app/media/revalidate.png'); + $replacement = imagecreatetruecolor(500, 500); + imagefill($replacement, 0, 0, imagecolorallocate($replacement, 200, 40, 40)); + imagepng($replacement, $path); + imagedestroy($replacement); + + $config = Cache::get($cacheKey); + $this->assertIsArray($config); + $config['image']['mtime'] = time() + 120; + Cache::put($cacheKey, $config, now()->addDay()); + + // The key is unchanged, so this is the same entry. Only a source re-read can turn + // the answer from 200x150 into 200x200. + $this->assertSame( + ['width' => 200, 'height' => 200], + ImageResizer::filterGetDimensions($resizerUrl), + 'A source entry must be discarded when the configuration reports a newer mtime.' + ); + } + /** + * An unrecognised mode falls back to the auto result, and says so + * + * This is the one branch where the prediction is knowingly a guess: a custom mode + * means a custom resizer, and no amount of arithmetic recovers its output. The + * parity sweep cannot cover it, because it compares against a resizer that would + * throw on an unknown mode, so the fallback needs its own test. It is asserted + * rather than assumed because the fallback is the only place the class reports a + * size it cannot justify, and a silent change to a wrong number is exactly the kind + * of guess this documents. + * + * @return void + */ + public function testUnrecognisedModeFallsBackToAuto() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + // A non-square source, so auto and the other modes do not agree + $sourceWidth = 400; + $sourceHeight = 300; + + foreach ([[200, 150], [0, 150], [200, 0], [1, 0]] as $bounds) { + [$reqWidth, $reqHeight] = $bounds; + + $this->assertSame( + $this->predictDimensions($sourceWidth, $sourceHeight, $reqWidth, $reqHeight, 'auto'), + $this->predictDimensions($sourceWidth, $sourceHeight, $reqWidth, $reqHeight, 'no-such-mode'), + "An unrecognised mode must fall back to auto for {$reqWidth}x{$reqHeight}." + ); + } + + // And the fallback is a real answer rather than a refusal: on this source and + // these bounds the auto result is a positive size, not the 0x0 used to mean + // "the resizer would refuse". + $fallback = $this->predictDimensions($sourceWidth, $sourceHeight, 200, 150, 'no-such-mode'); + $this->assertGreaterThan(0, $fallback['width']); + $this->assertGreaterThan(0, $fallback['height']); + } + /** + * Unidentifiable input raises; only a resizer URL reports unknown + * + * Two different failures that look alike from the outside. A resizer URL has no + * file behind it until the browser asks for one, so reporting unknown is the only + * honest answer. A path that does not resolve to an image at all is a broken + * template, and it used to raise -- a signal worth keeping, because a silently + * rendered width="0" is much harder to notice than an exception. + * + * @return void + */ + public function testUnidentifiableInputStillRaises() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $this->expectException(SystemException::class); + $this->expectExceptionMessageMatches('/^Unable to process the provided image/'); + + ImageResizer::filterGetDimensions('/plugins/database/tester/assets/images/MISSING.png'); + } + + /** + * A resizer URL with no cached configuration reports unknown rather than raising + * + * @return void + */ + public function testResizerUrlWithoutConfigurationReportsUnknown() + { + if (!in_array('Cms', Config::get('cms.loadModules', []))) { + $this->markTestSkipped('The CMS module is not active.'); + } + + $identifier = str_repeat('a', 40); + Cache::forget(ImageResizer::CACHE_PREFIX . $identifier); + + $this->assertSame( + ['width' => 0, 'height' => 0], + ImageResizer::filterGetDimensions('/resizer/' . $identifier . '/%252Fstorage%252Fapp%252Fresized%252Fnope.png'), + 'A resizer URL with nothing cached is unknown, not an error.' + ); + } + /** + * Write a JPEG carrying an EXIF orientation tag + * + * The APP1 segment is assembled by hand: a little-endian TIFF header followed by a + * single IFD0 entry for tag 0x0112, spliced in after the SOI marker. + */ + protected function createRotatedJpeg(string $name, int $width, int $height, int $orientation): string + { + $directory = storage_path('app/media'); + + if (!is_dir($directory)) { + mkdir($directory, 0777, true); + } + + $image = imagecreatetruecolor($width, $height); + imagefill($image, 0, 0, imagecolorallocate($image, 20, 60, 110)); + ob_start(); + imagejpeg($image, null, 92); + $jpeg = ob_get_clean(); + imagedestroy($image); + + $tiff = "II" . "\x2A\x00" + . "\x08\x00\x00\x00" + . "\x01\x00" + . "\x12\x01" + . "\x03\x00" + . "\x01\x00\x00\x00" + . pack('v', $orientation) . "\x00\x00" + . "\x00\x00\x00\x00"; + + $payload = "Exif\x00\x00" . $tiff; + $app1 = "\xFF\xE1" . pack('n', strlen($payload) + 2) . $payload; + + $path = $directory . DIRECTORY_SEPARATOR . $name; + file_put_contents($path, substr($jpeg, 0, 2) . $app1 . substr($jpeg, 2)); + + return Config::get('cms.storage.media.path') . '/' . $name; + } + + /** + * Assert that the spliced EXIF segment is where it should be and says what we meant + * + * Read back without the exif extension, so the fixture itself is proved well formed + * on every platform even where the orientation cannot be exercised. + */ + protected function assertExifSegmentIsWellFormed(string $url, int $expectedOrientation, int $width, int $height): void + { + $path = storage_path('app/media') . '/' . basename($url); + $raw = file_get_contents($path); + + $this->assertSame("\xFF\xD8\xFF\xE1", substr($raw, 0, 4), 'SOI followed by an APP1 marker.'); + + $declared = unpack('n', substr($raw, 4, 2))[1]; + $this->assertSame(substr($raw, 6, 6), "Exif\x00\x00", 'The segment identifies itself as Exif.'); + + // The payload is "Exif\0\0" followed by the TIFF block, so the header starts + // six bytes into the payload rather than at its first byte. + $tiff = substr($raw, 6 + 6, $declared - 2 - 6); + $this->assertSame('II', substr($tiff, 0, 2), 'Little-endian TIFF header.'); + $this->assertSame(42, unpack('v', substr($tiff, 2, 2))[1], 'TIFF magic number.'); + + $ifdOffset = unpack('V', substr($tiff, 4, 4))[1]; + $this->assertSame(0x0112, unpack('v', substr($tiff, $ifdOffset + 2, 2))[1], 'The first IFD0 tag is Orientation.'); + $this->assertSame($expectedOrientation, unpack('v', substr($tiff, $ifdOffset + 10, 2))[1], 'The orientation value round-trips.'); + + // The JPEG data after the segment has to still be decodable at the stored size + $remainder = substr($raw, 2 + 2 + $declared); + $size = getimagesizefromstring("\xFF\xD8" . $remainder); + $this->assertNotFalse($size, 'The splice did not corrupt the image data.'); + $this->assertSame([$width, $height], [$size[0], $size[1]]); + } + + /** + * Place a generated image on the media disk and return its public URL + */ + protected function createMediaImage(string $name, int $width, int $height): string + { + $directory = storage_path('app/media'); + + if (!is_dir($directory)) { + mkdir($directory, 0777, true); + } + + $image = imagecreatetruecolor($width, $height); + imagefill($image, 0, 0, imagecolorallocate($image, 40, 80, 120)); + imagepng($image, $directory . DIRECTORY_SEPARATOR . $name); + imagedestroy($image); + + return Config::get('cms.storage.media.path') . '/' . $name; + } + + /** + * Generate the source images used by the parity test + * + * @return array Absolute paths to the generated fixtures + */ + protected function createParityFixtures(): array + { + $directory = storage_path('temp/resizer-parity'); + + if (!is_dir($directory)) { + mkdir($directory, 0777, true); + } + + // Sizes are chosen for the ratio, not the resolution, since the parity + // assertion only depends on the ratio. The near-square case is the sharpest + // one for the round-versus-truncate divergence. + $sizes = [ + 'landscape-101x64.png' => [101, 64], + 'portrait-64x101.png' => [64, 101], + 'nearsquare-101x97.png' => [101, 97], + 'extreme-3x7.png' => [3, 7], + ]; + + $fixtures = []; + + foreach ($sizes as $name => $size) { + $path = $directory . DIRECTORY_SEPARATOR . $name; + $image = imagecreatetruecolor($size[0], $size[1]); + imagefill($image, 0, 0, imagecolorallocate($image, 40, 80, 120)); + imagepng($image, $path); + imagedestroy($image); + $fixtures[] = $path; + } + + return $fixtures; + } + + /** + * Run the real resizer and measure what it produced + * + * @return array|null ['width' => int, 'height' => int], or null if the resizer refused + */ + protected function resizeAndMeasure(string $path, int $width, int $height, array $options): ?array + { + try { + $resizer = DefaultResizer::open($path)->resize($width, $height, $options); + } catch (Throwable $ex) { + return null; + } + + $property = new ReflectionProperty($resizer, 'image'); + $property->setAccessible(true); + $image = $property->getValue($resizer); + + if (!$image instanceof GdImage) { + return null; + } + + return ['width' => imagesx($image), 'height' => imagesy($image)]; + } + + /** + * Ask the resizer class to predict the dimensions it will produce + * + * @return array ['width' => int, 'height' => int] + */ + protected function predictDimensions(int $origWidth, int $origHeight, int $reqWidth, int $reqHeight, string $mode): array + { + $method = new ReflectionMethod(ImageResizer::class, 'calculateResizedDimensions'); + $method->setAccessible(true); + + return $method->invoke(null, $origWidth, $origHeight, $reqWidth, $reqHeight, $mode); + } + protected function setUpStorage() { $this->app->useStoragePath(base_path('storage/temp')); @@ -472,11 +1263,23 @@ protected function removeMedia() return; } + // Anything this class creates outside of app/media has to go too, or the + // rmdir() below fails and takes an unrelated test class down with it. + File::deleteDirectory(storage_path('app/resized')); + File::deleteDirectory(storage_path('app/real-media')); + + // A symlink left behind by a failed test would outlive this class and confuse + // whichever one ran next + $link = storage_path('app/linked-media'); + if (is_link($link) || file_exists($link)) { + unlink($link); + } + foreach (glob(storage_path('app/media/*')) as $file) { unlink($file); } - rmdir(storage_path('app/media')); - rmdir(storage_path('app')); + @rmdir(storage_path('app/media')); + @rmdir(storage_path('app')); } } From 2bc4b9f274724672cc3d2a2799ec5e7be7f54f8d Mon Sep 17 00:00:00 2001 From: Matteo Trubini <7964032+matteotrubini@users.noreply.github.com> Date: Thu, 1 Oct 2026 21:26:12 +0000 Subject: [PATCH 2/2] test(system): remove the fixture symlink the way each platform requires --- .../system/tests/classes/ImageResizerTest.php | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/modules/system/tests/classes/ImageResizerTest.php b/modules/system/tests/classes/ImageResizerTest.php index 4d0dba781c..729a908c9b 100644 --- a/modules/system/tests/classes/ImageResizerTest.php +++ b/modules/system/tests/classes/ImageResizerTest.php @@ -1268,15 +1268,23 @@ protected function removeMedia() File::deleteDirectory(storage_path('app/resized')); File::deleteDirectory(storage_path('app/real-media')); - // A symlink left behind by a failed test would outlive this class and confuse - // whichever one ran next - $link = storage_path('app/linked-media'); - if (is_link($link) || file_exists($link)) { - unlink($link); + // The media files go first. A symlink left behind by a failed test would + // outlive this class and confuse whichever one ran next -- but removing it is + // the step most likely to fail, and on Windows it is: unlink() cannot remove a + // directory symlink there, so an uncaught throw here used to abort the rest of + // the cleanup and leave this class's media files on disk for the next class to + // count. Windows wants rmdir(), Linux wants unlink(), so try both and suppress + // both rather than choosing wrong on either platform. + foreach (glob(storage_path('app/media/*')) as $file) { + @unlink($file); } - foreach (glob(storage_path('app/media/*')) as $file) { - unlink($file); + $link = storage_path('app/linked-media'); + if (is_link($link) || file_exists($link)) { + @unlink($link); + if (is_link($link) || file_exists($link)) { + @rmdir($link); + } } @rmdir(storage_path('app/media'));