diff --git a/CHANGELOG.md b/CHANGELOG.md index b82c2a4..5347212 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,36 @@ 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. + "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 + namespace, so deleting one can remove the other's derivative. That needs a + cache-naming change, not a guard. + ## [1.42.0] - 2026-08-26 ### Added diff --git a/src/StarterBase.php b/src/StarterBase.php index d75c6a3..c4dcddb 100644 --- a/src/StarterBase.php +++ b/src/StarterBase.php @@ -3877,6 +3877,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 ); @@ -3933,6 +3942,56 @@ 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 ); + + // `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 true; + } + + $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 + ) + ); + + // 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; + } + /** * 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..7a5b0b9 --- /dev/null +++ b/tests/Unit/StarterBase/CleanupCachedImagesTest.php @@ -0,0 +1,223 @@ + + */ + private array $created = []; + + protected function setUp(): void { + parent::setUp(); + $this->base = $this->createStarterBase(); + + $this->cache_dir = WP_CONTENT_DIR . '/cache/image'; + $this->makeDir( $this->cache_dir . '/900x0-center' ); + $this->makeDir( $this->cache_dir . '/1439x0-center' ); + + $this->stubFilesystem(); + } + + protected function tearDown(): void { + 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(); + } + + /** 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; + } + 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. */ + 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|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, $last_error ) extends \wpdb { + public string $postmeta = 'wp_postmeta'; + + 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|null { + return null === $this->siblings ? null : (string) $this->siblings; + } + }; + } + + private function seedDerivatives(): void { + $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 { + $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. + */ + /** + * 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' ); + 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' ); + } +}