From 92b87574f5972b6760b801c9742324a38eb0be3d Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Wed, 26 Aug 2026 18:24:07 +0200 Subject: [PATCH 1/2] fix(media): keep cached derivatives a sibling attachment still uses cleanup_cached_images() matched the resizer cache by basename alone. The cache is keyed by file, but one file routinely carries several attachment rows -- WPML writes one per language, and a duplicate upload can be pointed at an existing path -- so deleting any one row took the shared derivatives with it. Measured on a five-language site: 5542 files shared by 25981 attachment rows. The homepage hero lost its derivatives while five rows and the rendered page still referenced them. The guard fails closed. Where the sibling question cannot be answered the files are kept: a stale derivative is overwritten by the next resize, while one deleted in error disappears from a page that is still serving it. Deliberately NOT done here: - No StarterBase feature flag. The convention puts behaviour changes behind one, default off, but that would leave a data-loss path live for every consumer until they opted out of it. Nothing observable is added; a delete that destroyed in-use files stops happening. - The basename collision across upload-year folders is untouched. 336 basenames map to more than one file, and the flat cache namespace cannot tell them apart. That is a cache-naming decision, not a guard, and it gets its own issue. - The query uses %i rather than interpolating $wpdb->postmeta, which keeps the string literal for PHPStan instead of buying silence with an ignore. --- CHANGELOG.md | 25 ++++ src/StarterBase.php | 49 ++++++ .../StarterBase/CleanupCachedImagesTest.php | 141 ++++++++++++++++++ 3 files changed, 215 insertions(+) create mode 100644 tests/Unit/StarterBase/CleanupCachedImagesTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index e9f6024..de25b4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,31 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ## [Unreleased] +### Fixed + +- `StarterBase::cleanup_cached_images()` deleted resizer derivatives that other + attachments were still using. The cache under `wp-content/cache/image` is + keyed by file, but one file routinely carries several attachment rows: WPML + writes one per language, and a duplicate upload can be pointed at a path that + already exists. Deleting any one of those rows wiped the shared derivatives, + and the site then regenerated them — or, where the encoder was broken, served + a damaged image. + + Measured on a five-language site: 5542 files were shared by 25981 attachment + rows, so roughly four fifths of the media library could take a live image + down with it. The homepage hero lost its derivatives while five attachment + rows and the rendered page still referenced them. + + The delete now runs only when no other attachment points at the same + `_wp_attached_file`. It fails closed: where that question cannot be answered + the files are kept, because a stale derivative is overwritten by the next + resize while one deleted in error vanishes from a page still serving it. + + Not fixed here, and tracked separately: two different files that share a + basename across upload-year folders still collide in the flat cache + namespace, so deleting one can remove the other's derivative. That needs a + cache-naming change, not a guard. + ## [1.41.1] - 2026-08-26 ### Fixed diff --git a/src/StarterBase.php b/src/StarterBase.php index 39d3604..d9367f6 100644 --- a/src/StarterBase.php +++ b/src/StarterBase.php @@ -3846,6 +3846,15 @@ public function cleanup_cached_images( $attachment_id ) { return; } + // The cache is keyed by file, not by attachment, and one file routinely + // carries several attachment rows: WPML writes one per language, and a + // duplicate upload can be pointed at a path that already exists. Those + // rows share one set of derivatives, so deleting one of them must leave + // the files alone. + if ( $this->attached_file_is_shared( $attachment_id ) ) { + return; + } + // Extract filename without path $filename = basename( $file_path ); $path_info = pathinfo( $filename ); @@ -3902,6 +3911,46 @@ public function cleanup_cached_images( $attachment_id ) { } } + /** + * Whether an attachment other than this one still points at the same file. + * + * Compares `_wp_attached_file` because that is the value siblings share. + * `get_attached_file()` is filtered and absolute, so it is not comparable + * across rows. + * + * Fails closed. When the question cannot be answered the caller keeps the + * cached files: a stale derivative is overwritten by the next resize, while + * one deleted in error vanishes from a page that is still serving it. + * + * @param int $attachment_id The attachment being deleted. + * @return bool True when the file is shared, or when sharing cannot be determined. + */ + private function attached_file_is_shared( $attachment_id ) { + global $wpdb; + + if ( ! $wpdb instanceof \wpdb ) { + return true; + } + + $relative_path = get_post_meta( (int) $attachment_id, '_wp_attached_file', true ); + + // No path to match on means no sibling can be keyed to it either. + if ( ! is_string( $relative_path ) || '' === $relative_path ) { + return false; + } + + $shared = $wpdb->get_var( + $wpdb->prepare( + "SELECT COUNT(*) FROM %i WHERE meta_key = '_wp_attached_file' AND meta_value = %s AND post_id != %d", + $wpdb->postmeta, + $relative_path, + (int) $attachment_id + ) + ); + + return (int) $shared > 0; + } + /** * Prevent uploading images when a file with the same basename but different extension already exists. * diff --git a/tests/Unit/StarterBase/CleanupCachedImagesTest.php b/tests/Unit/StarterBase/CleanupCachedImagesTest.php new file mode 100644 index 0000000..cba0709 --- /dev/null +++ b/tests/Unit/StarterBase/CleanupCachedImagesTest.php @@ -0,0 +1,141 @@ +base = $this->createStarterBase(); + + $this->cache_dir = WP_CONTENT_DIR . '/cache/image'; + $this->removeCacheDir(); + mkdir( $this->cache_dir . '/900x0-center', 0777, true ); + mkdir( $this->cache_dir . '/1439x0-center', 0777, true ); + + $this->stubFilesystem(); + } + + protected function tearDown(): void { + $this->removeCacheDir(); + unset( $GLOBALS['wpdb'], $GLOBALS['wp_filesystem'] ); + parent::tearDown(); + } + + private function removeCacheDir(): void { + if ( ! is_dir( $this->cache_dir ) ) { + return; + } + $items = new \RecursiveIteratorIterator( + new \RecursiveDirectoryIterator( $this->cache_dir, \RecursiveDirectoryIterator::SKIP_DOTS ), + \RecursiveIteratorIterator::CHILD_FIRST + ); + foreach ( $items as $item ) { + $item->isDir() ? rmdir( $item->getPathname() ) : unlink( $item->getPathname() ); + } + rmdir( $this->cache_dir ); + } + + /** Real unlink through the WP_Filesystem seam the production code uses. */ + private function stubFilesystem(): void { + Functions\when( 'WP_Filesystem' )->justReturn( true ); + $GLOBALS['wp_filesystem'] = new class { + public function exists( string $path ): bool { + return file_exists( $path ); + } + + public function delete( string $path ): bool { + return unlink( $path ); + } + }; + } + + /** + * @param int $siblings Attachment rows OTHER than the one being deleted that + * point at the same `_wp_attached_file` value. + */ + private function stubAttachment( int $attachment_id, string $relative_path, int $siblings ): void { + Functions\when( 'get_attached_file' )->justReturn( '/var/www/wp-content/uploads/' . $relative_path ); + Functions\when( 'get_post_meta' )->justReturn( $relative_path ); + + $GLOBALS['wpdb'] = new class( $siblings ) extends \wpdb { + public string $postmeta = 'wp_postmeta'; + + public function __construct( private readonly int $siblings ) { + } + + public function prepare( string $query, mixed ...$args ): string { + return $query; + } + + public function get_var( string $query ): string { + return (string) $this->siblings; + } + }; + } + + private function seedDerivatives(): void { + file_put_contents( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif', 'x' ); + file_put_contents( $this->cache_dir . '/1439x0-center/homepage-hero-desktop.avif', 'x' ); + file_put_contents( $this->cache_dir . '/900x0-center/unrelated.avif', 'x' ); + } + + public function test_deletes_derivatives_when_no_other_attachment_shares_the_file(): void { + $this->seedDerivatives(); + $this->stubAttachment( 59741, '2026/08/homepage-hero-desktop.webp', siblings: 0 ); + + $this->base->cleanup_cached_images( 59741 ); + + $this->assertFileDoesNotExist( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif' ); + $this->assertFileDoesNotExist( $this->cache_dir . '/1439x0-center/homepage-hero-desktop.avif' ); + $this->assertFileExists( + $this->cache_dir . '/900x0-center/unrelated.avif', + 'a different basename must never be touched' + ); + } + + public function test_keeps_derivatives_when_another_attachment_shares_the_file(): void { + $this->seedDerivatives(); + // The WPML case: five rows, one file. Deleting one language's row leaves + // four rows — and the live page — still rendering these derivatives. + $this->stubAttachment( 59741, '2026/08/homepage-hero-desktop.webp', siblings: 4 ); + + $this->base->cleanup_cached_images( 59741 ); + + $this->assertFileExists( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif' ); + $this->assertFileExists( $this->cache_dir . '/1439x0-center/homepage-hero-desktop.avif' ); + } + + /** + * Without a database the sibling question cannot be answered. Skipping the + * delete leaves a stale file that the next resize overwrites; guessing wrong + * in the other direction destroys an image that is still on the page. + */ + public function test_keeps_derivatives_when_the_sibling_count_is_unavailable(): void { + $this->seedDerivatives(); + Functions\when( 'get_attached_file' )->justReturn( '/var/www/wp-content/uploads/2026/08/homepage-hero-desktop.webp' ); + Functions\when( 'get_post_meta' )->justReturn( '2026/08/homepage-hero-desktop.webp' ); + unset( $GLOBALS['wpdb'] ); + + $this->base->cleanup_cached_images( 59741 ); + + $this->assertFileExists( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif' ); + } +} From efaf9899b75457b9a654512778b63b37be040b3c Mon Sep 17 00:00:00 2001 From: Petr Parimucha Date: Wed, 26 Aug 2026 18:39:45 +0200 Subject: [PATCH 2/2] fix(media): treat an unanswerable sibling query as shared, not as zero Review of the previous commit found the fail-closed guard was not closed. get_var() reports a failed query by returning null, and (int) null is the same 0 a genuine "no siblings" answer gives -- so any transient database error read as "not shared" and deleted the files the guard exists to keep. The docblock already promised the opposite. Null and last_error are now both checked. An empty _wp_attached_file flips the same way: get_attached_file() already returned a path, so an empty meta value means a filter supplied it (offloaded media) and siblings have no key to match on. Four tests added, three of them red before this change. The fourth pins the SQL itself -- the wpdb stub returns a count whatever the query says, so without it the suite would pass against the wrong table, the wrong meta key, or a dropped post_id exclusion. Test teardown now removes only the paths it created. WP_CONTENT_DIR is a bootstrap constant, so the cache directory cannot be varied per run and deleting the tree wholesale would take a concurrent run's fixtures with it. Found by Codex (gpt-5-codex) reviewing PR #152. %i is kept: the reviewer read its WP 6.2 floor as a silent-delete path, and that consequence is what this commit removes -- an unsupported placeholder now fails the query, and a failed query keeps the files. Interpolating the table instead would have reintroduced the PHPStan literal-string error for no safety gain. --- CHANGELOG.md | 5 + src/StarterBase.php | 14 +- .../StarterBase/CleanupCachedImagesTest.php | 128 ++++++++++++++---- 3 files changed, 122 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de25b4e..80cd805 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). `_wp_attached_file`. It fails closed: where that question cannot be answered the files are kept, because a stale derivative is overwritten by the next resize while one deleted in error vanishes from a page still serving it. + "Cannot be answered" covers a missing `$wpdb`, an empty `_wp_attached_file` + (a filter supplied the path, so siblings have no key to match on), and a + failed query — `get_var()` reports an error by returning null, which casts to + the same zero a genuine "no siblings" answer gives, so the null and + `last_error` are both checked rather than read as a count. Not fixed here, and tracked separately: two different files that share a basename across upload-year folders still collide in the flat cache diff --git a/src/StarterBase.php b/src/StarterBase.php index d9367f6..723ae22 100644 --- a/src/StarterBase.php +++ b/src/StarterBase.php @@ -3934,9 +3934,11 @@ private function attached_file_is_shared( $attachment_id ) { $relative_path = get_post_meta( (int) $attachment_id, '_wp_attached_file', true ); - // No path to match on means no sibling can be keyed to it either. + // `get_attached_file()` already returned a path, so an empty meta value + // means a filter supplied that path (offloaded media). Siblings are keyed + // on the meta, so there is nothing left to compare them against. if ( ! is_string( $relative_path ) || '' === $relative_path ) { - return false; + return true; } $shared = $wpdb->get_var( @@ -3948,6 +3950,14 @@ private function attached_file_is_shared( $attachment_id ) { ) ); + // A failed query answers nothing, and it does not announce itself: on + // error `get_var()` returns null, which casts to the same 0 a genuine + // "no siblings" result gives. Reading it that way would delete on any + // transient database error, so both signals are checked instead. + if ( null === $shared || '' !== $wpdb->last_error ) { + return true; + } + return (int) $shared > 0; } diff --git a/tests/Unit/StarterBase/CleanupCachedImagesTest.php b/tests/Unit/StarterBase/CleanupCachedImagesTest.php index cba0709..7a5b0b9 100644 --- a/tests/Unit/StarterBase/CleanupCachedImagesTest.php +++ b/tests/Unit/StarterBase/CleanupCachedImagesTest.php @@ -21,36 +21,54 @@ class CleanupCachedImagesTest extends StarterBaseTestCase { private string $cache_dir; + /** + * Paths this test created, so teardown removes its own trace and nothing + * else. WP_CONTENT_DIR is a constant fixed by the bootstrap, so the cache + * directory cannot be varied per run — deleting the tree wholesale would + * take a concurrent run's fixtures (or a developer's scratch files) with it. + * + * @var list + */ + private array $created = []; + protected function setUp(): void { parent::setUp(); $this->base = $this->createStarterBase(); $this->cache_dir = WP_CONTENT_DIR . '/cache/image'; - $this->removeCacheDir(); - mkdir( $this->cache_dir . '/900x0-center', 0777, true ); - mkdir( $this->cache_dir . '/1439x0-center', 0777, true ); + $this->makeDir( $this->cache_dir . '/900x0-center' ); + $this->makeDir( $this->cache_dir . '/1439x0-center' ); $this->stubFilesystem(); } protected function tearDown(): void { - $this->removeCacheDir(); + foreach ( array_reverse( $this->created ) as $path ) { + if ( is_file( $path ) ) { + unlink( $path ); + } elseif ( is_dir( $path ) ) { + @rmdir( $path ); // Only succeeds while empty, which is the intent. + } + } + $this->created = []; unset( $GLOBALS['wpdb'], $GLOBALS['wp_filesystem'] ); parent::tearDown(); } - private function removeCacheDir(): void { - if ( ! is_dir( $this->cache_dir ) ) { - return; - } - $items = new \RecursiveIteratorIterator( - new \RecursiveDirectoryIterator( $this->cache_dir, \RecursiveDirectoryIterator::SKIP_DOTS ), - \RecursiveIteratorIterator::CHILD_FIRST - ); - foreach ( $items as $item ) { - $item->isDir() ? rmdir( $item->getPathname() ) : unlink( $item->getPathname() ); + /** Create $path and every missing parent, recording only what did not exist. */ + private function makeDir( string $path ): void { + $missing = []; + for ( $dir = $path; ! is_dir( $dir ); $dir = dirname( $dir ) ) { + $missing[] = $dir; } - rmdir( $this->cache_dir ); + mkdir( $path, 0777, true ); + // Deepest first, so teardown's reverse walk removes children before parents. + $this->created = array_merge( $this->created, array_reverse( $missing ) ); + } + + private function seedFile( string $path ): void { + file_put_contents( $path, 'x' ); + $this->created[] = $path; } /** Real unlink through the WP_Filesystem seam the production code uses. */ @@ -71,30 +89,40 @@ public function delete( string $path ): bool { * @param int $siblings Attachment rows OTHER than the one being deleted that * point at the same `_wp_attached_file` value. */ - private function stubAttachment( int $attachment_id, string $relative_path, int $siblings ): void { + private function stubAttachment( int $attachment_id, string $relative_path, int|null $siblings, string $last_error = '' ): void { Functions\when( 'get_attached_file' )->justReturn( '/var/www/wp-content/uploads/' . $relative_path ); Functions\when( 'get_post_meta' )->justReturn( $relative_path ); - $GLOBALS['wpdb'] = new class( $siblings ) extends \wpdb { + $GLOBALS['wpdb'] = new class( $siblings, $last_error ) extends \wpdb { public string $postmeta = 'wp_postmeta'; - public function __construct( private readonly int $siblings ) { + public string $last_error = ''; + + /** @var list Arguments the production code passed to prepare(). */ + public array $prepare_args = []; + + public string $prepared_query = ''; + + public function __construct( private readonly int|null $siblings, string $last_error ) { + $this->last_error = $last_error; } public function prepare( string $query, mixed ...$args ): string { + $this->prepared_query = $query; + $this->prepare_args = $args; return $query; } - public function get_var( string $query ): string { - return (string) $this->siblings; + public function get_var( string $query ): string|null { + return null === $this->siblings ? null : (string) $this->siblings; } }; } private function seedDerivatives(): void { - file_put_contents( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif', 'x' ); - file_put_contents( $this->cache_dir . '/1439x0-center/homepage-hero-desktop.avif', 'x' ); - file_put_contents( $this->cache_dir . '/900x0-center/unrelated.avif', 'x' ); + $this->seedFile( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif' ); + $this->seedFile( $this->cache_dir . '/1439x0-center/homepage-hero-desktop.avif' ); + $this->seedFile( $this->cache_dir . '/900x0-center/unrelated.avif' ); } public function test_deletes_derivatives_when_no_other_attachment_shares_the_file(): void { @@ -128,6 +156,60 @@ public function test_keeps_derivatives_when_another_attachment_shares_the_file() * delete leaves a stale file that the next resize overwrites; guessing wrong * in the other direction destroys an image that is still on the page. */ + /** + * A failed query answers nothing. `get_var()` returns null on error, and the + * naive `(int) null > 0` reading of that is indistinguishable from a real + * zero — which would delete the files on any transient database error. + */ + public function test_keeps_derivatives_when_the_query_fails(): void { + $this->seedDerivatives(); + $this->stubAttachment( 59741, '2026/08/homepage-hero-desktop.webp', siblings: null ); + + $this->base->cleanup_cached_images( 59741 ); + + $this->assertFileExists( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif' ); + } + + public function test_keeps_derivatives_when_the_query_reported_an_error(): void { + $this->seedDerivatives(); + $this->stubAttachment( 59741, '2026/08/homepage-hero-desktop.webp', siblings: 0, last_error: 'MySQL server has gone away' ); + + $this->base->cleanup_cached_images( 59741 ); + + $this->assertFileExists( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif' ); + } + + /** + * An empty `_wp_attached_file` with a non-empty `get_attached_file()` means a + * filter supplied the path (offloaded media). Sharing is then unanswerable. + */ + public function test_keeps_derivatives_when_the_attached_file_meta_is_empty(): void { + $this->seedDerivatives(); + $this->stubAttachment( 59741, '2026/08/homepage-hero-desktop.webp', siblings: 0 ); + Functions\when( 'get_post_meta' )->justReturn( '' ); + + $this->base->cleanup_cached_images( 59741 ); + + $this->assertFileExists( $this->cache_dir . '/900x0-center/homepage-hero-desktop.avif' ); + } + + /** The stub returns a count whatever the SQL says, so pin the SQL itself. */ + public function test_asks_the_database_the_question_it_claims_to_ask(): void { + $this->seedDerivatives(); + $this->stubAttachment( 59741, '2026/08/homepage-hero-desktop.webp', siblings: 0 ); + + $this->base->cleanup_cached_images( 59741 ); + + $wpdb = $GLOBALS['wpdb']; + $this->assertStringContainsString( "meta_key = '_wp_attached_file'", $wpdb->prepared_query ); + $this->assertStringContainsString( 'post_id != %d', $wpdb->prepared_query ); + $this->assertSame( + [ 'wp_postmeta', '2026/08/homepage-hero-desktop.webp', 59741 ], + $wpdb->prepare_args, + 'the table, the shared path and the excluded row must all reach prepare()' + ); + } + public function test_keeps_derivatives_when_the_sibling_count_is_unavailable(): void { $this->seedDerivatives(); Functions\when( 'get_attached_file' )->justReturn( '/var/www/wp-content/uploads/2026/08/homepage-hero-desktop.webp' );