From ab6e239f5c14ce33e761ccf7517de53166cb949e Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Wed, 30 Sep 2026 08:58:29 +0200 Subject: [PATCH 1/5] fix(media): hook wp_get_attachment_image_src to keep Cloudinary URLs as the last word WordPress applies its own filter after image_downsize() has already resolved, and CDN/ image-optimization plugins commonly target it directly -- so a competing plugin, or filter ordering on image_downsize, could leave a local URL in place with no second chance to correct it. The new filter is a cheap guard: it bows out immediately if the URL is already a Cloudinary URL, and otherwise reuses the same deliverable/re-entrancy checks as filter_downsize(), so it only does work in the rare case something else missed. Also extracts the shared save-state/ re-entrancy guard (in_downsize, content_save_pre, is_saving_metadata, video exclusion) into Media::is_replacement_paused(), reused by filter_downsize(), the new filter, and attachment_url() (WPP-1183). --- php/class-media.php | 100 +++++-- .../tests/test-attachment-image-src.php | 268 ++++++++++++++++++ 2 files changed, 350 insertions(+), 18 deletions(-) create mode 100644 tests/phpunit/tests/test-attachment-image-src.php diff --git a/php/class-media.php b/php/class-media.php index ea14b2ab6..412238ff8 100644 --- a/php/class-media.php +++ b/php/class-media.php @@ -1186,23 +1186,20 @@ public function attachment_url( $url, $attachment_id ) { return str_replace( trailingslashit( $dirs['baseurl'] ), '', $url ); } - if ( - false === $this->in_downsize - && ! doing_filter( 'content_save_pre' ) - && ! Utils::is_saving_metadata() - /** - * Filter doing upload. - * If so, return the default attachment URL. - * - * @param bool Default false. - * - * @return bool - */ - && ! apply_filters( 'cloudinary_doing_upload', false ) - ) { - if ( ! $this->is_cloudinary_url( $url ) && $this->cloudinary_id( $attachment_id ) ) { - $url = $this->cloudinary_url( $attachment_id ); - } + /** + * Filter doing upload. + * If so, return the default attachment URL. + * + * @param bool Default false. + * + * @return bool + */ + if ( $this->is_replacement_paused( $attachment_id, false ) || apply_filters( 'cloudinary_doing_upload', false ) ) { + return $url; + } + + if ( ! $this->is_cloudinary_url( $url ) && $this->cloudinary_id( $attachment_id ) ) { + $url = $this->cloudinary_url( $attachment_id ); } return $url; @@ -1802,7 +1799,7 @@ public function filter_downsize( $image, $attachment_id, $size ) { } // Don't do this while saving. - if ( true === $this->in_downsize || doing_filter( 'content_save_pre' ) || wp_attachment_is( 'video', $attachment_id ) || Utils::is_saving_metadata() ) { + if ( $this->is_replacement_paused( $attachment_id ) ) { return $image; } @@ -1836,6 +1833,72 @@ public function filter_downsize( $image, $attachment_id, $size ) { return $image; } + /** + * Whether URL replacement should be paused for this attachment right now: while a + * downsize is already in progress (re-entrancy guard), while content or metadata is being + * saved, or -- unless explicitly included -- for a video attachment, since there is no + * "image size" to downsize a video to. + * + * Shared by filter_downsize(), filter_attachment_image_src(), and attachment_url(), so the + * save-state/re-entrancy logic lives in one place. + * + * @param int $attachment_id The attachment ID. + * @param bool $exclude_videos Whether a video attachment should also count as paused. + * + * @return bool + */ + private function is_replacement_paused( $attachment_id, $exclude_videos = true ) { + if ( true === $this->in_downsize || doing_filter( 'content_save_pre' ) || Utils::is_saving_metadata() ) { + return true; + } + + return $exclude_videos && wp_attachment_is( 'video', $attachment_id ); + } + + /** + * Correct wp_get_attachment_image_src() results that image_downsize() missed, or that a + * later-priority plugin overwrote, so Cloudinary keeps the last word on this specific, + * commonly-targeted core hook. + * + * @param array|false $image The image src array, or false. + * @param int $attachment_id The ID of the attachment. + * @param string|array $size The requested size of the image. + * @param bool $icon Whether the image should be treated as an icon. + * + * @return array|false The image array of size and url. + * @uses filter:wp_get_attachment_image_src + */ + public function filter_attachment_image_src( $image, $attachment_id, $size, $icon ) { + if ( empty( $image ) || $icon ) { + return $image; + } + + // Fast bow-out: already a Cloudinary URL, nothing to do. + if ( $this->is_cloudinary_url( $image[0] ) ) { + return $image; + } + + if ( $this->is_replacement_paused( $attachment_id ) ) { + return $image; + } + + if ( ! $this->plugin->get_component( 'delivery' )->is_deliverable( $attachment_id ) ) { + return $image; + } + + $cloudinary_id = $this->cloudinary_id( $attachment_id ); + if ( ! $cloudinary_id ) { + return $image; + } + + $url = $this->cloudinary_url( $attachment_id, $size, array(), $cloudinary_id ); + if ( $url ) { + $image[0] = $url; + } + + return $image; + } + /** * At the point of running wp_get_attachment_image_srcset, the $image_src Should be a Cloudinary URL, unless not synced. * This will fix the $image_meta so that the there's a match $src_matched on wp_calculate_image_srcset. @@ -3150,6 +3213,7 @@ public function add_live_url_filters() { add_filter( 'wp_get_attachment_url', array( $this, 'attachment_url' ), 10, 2 ); add_filter( 'wp_get_original_image_url', array( $this, 'original_attachment_url' ), 10, 2 ); add_filter( 'image_downsize', array( $this, 'filter_downsize' ), 10, 3 ); + add_filter( 'wp_get_attachment_image_src', array( $this, 'filter_attachment_image_src' ), PHP_INT_MAX, 4 ); add_filter( 'wp_calculate_image_srcset_meta', array( $this, 'calculate_image_srcset_meta' ), 10, 3 ); // Hook into Featured Image cycle. diff --git a/tests/phpunit/tests/test-attachment-image-src.php b/tests/phpunit/tests/test-attachment-image-src.php new file mode 100644 index 000000000..7de6d386d --- /dev/null +++ b/tests/phpunit/tests/test-attachment-image-src.php @@ -0,0 +1,268 @@ +attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' ); + } + + /** + * Build a Test_Attachment_Image_Src_Media instance wired to the real, already-booted plugin. + * + * @return Test_Attachment_Image_Src_Media + */ + protected function get_media() { + $media = new Test_Attachment_Image_Src_Media( \Cloudinary\get_plugin_instance() ); + $media->base_url = 'https://res.cloudinary.com/test-cloud'; + + return $media; + } + + /** + * Utils::is_saving_metadata() reads did_action() counters (add_post_meta, etc.) that + * WordPress never resets between tests within one PHPUnit process -- any earlier test, or + * even our own wpSetUpBeforeClass() fixture, already ticks them past zero. Momentarily + * clearing them here isolates the checks that come after that guard in + * filter_attachment_image_src(), so those tests exercise their own branch instead of + * always bowing out at is_saving_metadata() for an unrelated, process-wide reason. + * + * @param callable $callback Callback to run with the guard cleared. + * + * @return mixed + */ + protected function without_saving_metadata_guard( callable $callback ) { + $keys = array( 'add_post_meta', 'update_post_meta', 'add_term_meta', 'update_term_meta', 'add_user_meta', 'update_user_meta' ); + $saved = array(); + foreach ( $keys as $key ) { + if ( isset( $GLOBALS['wp_actions'][ $key ] ) ) { + $saved[ $key ] = $GLOBALS['wp_actions'][ $key ]; + unset( $GLOBALS['wp_actions'][ $key ] ); + } + } + try { + return $callback(); + } finally { + foreach ( $saved as $key => $value ) { + $GLOBALS['wp_actions'][ $key ] = $value; + } + } + } + + /** + * A URL that already lives on the configured Cloudinary domain is returned untouched -- the + * cheap bow-out path the ticket calls for, taken before any sync/delivery check runs. + * + * @return void + */ + public function test_already_cloudinary_url_is_returned_unchanged() { + $media = $this->get_media(); + $image = array( 'https://res.cloudinary.com/test-cloud/image/upload/v1/sample.jpg', 100, 100, true ); + + $result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); + + $this->assertSame( $image, $result ); + } + + /** + * The icon fallback path in wp_get_attachment_image_src() is left alone entirely. + * + * @return void + */ + public function test_icon_result_is_returned_unchanged() { + $media = $this->get_media(); + $image = array( 'http://example.org/wp-includes/images/media/default.png', 48, 64, false ); + + $result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', true ); + + $this->assertSame( $image, $result ); + } + + /** + * A false (no image) result has nothing to correct and must pass through unchanged. + * + * @return void + */ + public function test_false_image_is_returned_unchanged() { + $media = $this->get_media(); + + $this->assertFalse( $media->filter_attachment_image_src( false, self::$attachment_id, 'thumbnail', false ) ); + } + + /** + * A video attachment is left untouched, mirroring the same guard filter_downsize() uses. + * + * @return void + */ + public function test_video_attachment_is_returned_unchanged() { + $video_id = self::factory()->post->create( + array( + 'post_type' => 'attachment', + 'post_mime_type' => 'video/mp4', + ) + ); + + $media = $this->get_media(); + $image = array( 'http://example.org/wp-content/uploads/video.mp4', 640, 360, true ); + + $result = $this->without_saving_metadata_guard( + function () use ( $media, $image, $video_id ) { + return $media->filter_attachment_image_src( $image, $video_id, 'thumbnail', false ); + } + ); + + $this->assertSame( $image, $result ); + } + + /** + * An image attachment with no generated metadata is not deliverable (no width/height to + * hand Cloudinary), so the local URL passes through unchanged. + * + * @return void + */ + public function test_non_deliverable_attachment_is_returned_unchanged() { + $bare_id = self::factory()->post->create( + array( + 'post_type' => 'attachment', + 'post_mime_type' => 'image/jpeg', + ) + ); + + $media = $this->get_media(); + $media->stub_cloudinary_id = 'sample.jpg'; + $image = array( 'http://example.org/wp-content/uploads/bare.jpg', 100, 100, true ); + + $result = $this->without_saving_metadata_guard( + function () use ( $media, $image, $bare_id ) { + return $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', false ); + } + ); + + $this->assertSame( $image, $result ); + } + + /** + * A deliverable attachment that has no Cloudinary ID (not synced) is left untouched -- there + * is nothing to correct it to. + * + * @return void + */ + public function test_no_cloudinary_id_returns_image_unchanged() { + $media = $this->get_media(); + $media->stub_cloudinary_id = false; + $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/should-not-be-used.jpg'; + $image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true ); + + $result = $this->without_saving_metadata_guard( + function () use ( $media, $image ) { + return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); + } + ); + + $this->assertSame( $image, $result ); + } + + /** + * The actual fix: a local URL on a deliverable, synced attachment is corrected to the + * Cloudinary URL, with width/height/is-intermediate preserved from the original array. + * + * @return void + */ + public function test_local_url_is_corrected_to_cloudinary_url() { + $media = $this->get_media(); + $media->stub_cloudinary_id = 'sample.jpg'; + $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/sample.jpg'; + $image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true ); + + $result = $this->without_saving_metadata_guard( + function () use ( $media, $image ) { + return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); + } + ); + + $this->assertSame( $media->stub_cloudinary_url, $result[0] ); + $this->assertSame( 100, $result[1] ); + $this->assertSame( 100, $result[2] ); + $this->assertTrue( $result[3] ); + } +} + +/** + * A Media subclass with cloudinary_id()/cloudinary_url() stubbed out, so + * filter_attachment_image_src() can be exercised without driving the real sync/signature + * pipeline (which needs a live Cloudinary account -- see tests/e2e for that coverage). + */ +class Test_Attachment_Image_Src_Media extends \Cloudinary\Media { + + /** + * Canned cloudinary_id() return value. + * + * @var string|false + */ + public $stub_cloudinary_id = false; + + /** + * Canned cloudinary_url() return value. + * + * @var string|false + */ + public $stub_cloudinary_url = false; + + /** + * Stubbed to avoid the real sync/signature pipeline. + * + * @param int $attachment_id The attachment ID. + * + * @return string|false + */ + public function cloudinary_id( $attachment_id ) { + return $this->stub_cloudinary_id; + } + + /** + * Stubbed to avoid the real sync/signature pipeline. + * + * @param int $attachment_id The attachment ID. + * @param array|string $size The requested size. + * @param array $transformations Transformations to apply. + * @param string|null $cloudinary_id A forced Cloudinary ID. + * @param bool $overwrite_transformations Whether to overwrite transformations. + * + * @return string|false + */ + public function cloudinary_url( $attachment_id, $size = array(), $transformations = array(), $cloudinary_id = null, $overwrite_transformations = false ) { + return $this->stub_cloudinary_url; + } +} From df197d1db2c4d2f1d293cf315d279789e8b73258 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Thu, 1 Oct 2026 10:51:14 +0200 Subject: [PATCH 2/5] fix(media): run image corrections on the front end, and fix a list-mode blind spot image_downsize()/wp_get_attachment_image_src() are exercised by ordinary front-end theme code (e.g. the_post_thumbnail()), not just wp-admin screens -- manual QA confirmed a featured image rendered by Twenty Twenty-Five's front-end template hit the filter, so scoping correction to Utils::is_admin() (admin screens only) left the front end exposed to the same "something overwrote the URL after image_downsize()" problem this hook was added to guard against. Registers both filters unconditionally instead of inside add_live_url_filters()'s admin-only set; that method's other filters are untouched. Also fixes a bug the broadened testing surfaced: wp_get_attachment_image_src() passes the caller's $icon argument straight through to the filter regardless of whether a real image was actually found (it just means "fall back to a mime icon if none is found"), but filter_attachment_image_src() treated $icon as "this is a fallback icon" and bowed out unconditionally. WP_Media_List_Table's list-mode thumbnail column always calls with icon=true, even for real synced images, which silently blocked correction there while grid mode (driven by image_downsize() instead) worked fine. Dropping the $icon check is safe: the existing is_deliverable()/cloudinary_id() checks already leave a genuine icon fallback alone, since an unsynced/non-deliverable attachment has no cloudinary_id to correct it to (WPP-1183). --- php/class-media.php | 12 +++-- .../tests/test-attachment-image-src.php | 51 +++++++++++++++++-- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/php/class-media.php b/php/class-media.php index 412238ff8..8417bc224 100644 --- a/php/class-media.php +++ b/php/class-media.php @@ -1869,7 +1869,7 @@ private function is_replacement_paused( $attachment_id, $exclude_videos = true ) * @uses filter:wp_get_attachment_image_src */ public function filter_attachment_image_src( $image, $attachment_id, $size, $icon ) { - if ( empty( $image ) || $icon ) { + if ( empty( $image ) ) { return $image; } @@ -3212,8 +3212,6 @@ public function add_live_url_filters() { add_filter( 'wp_calculate_image_srcset', array( $this, 'image_srcset' ), 10, 5 ); add_filter( 'wp_get_attachment_url', array( $this, 'attachment_url' ), 10, 2 ); add_filter( 'wp_get_original_image_url', array( $this, 'original_attachment_url' ), 10, 2 ); - add_filter( 'image_downsize', array( $this, 'filter_downsize' ), 10, 3 ); - add_filter( 'wp_get_attachment_image_src', array( $this, 'filter_attachment_image_src' ), PHP_INT_MAX, 4 ); add_filter( 'wp_calculate_image_srcset_meta', array( $this, 'calculate_image_srcset_meta' ), 10, 3 ); // Hook into Featured Image cycle. @@ -3255,6 +3253,14 @@ public function setup() { if ( Utils::is_admin() ) { $this->add_live_url_filters(); } + + // image_downsize() and wp_get_attachment_image_src() are exercised by ordinary + // front-end theme code (e.g. the_post_thumbnail()), not just wp-admin screens, so + // these run everywhere rather than being scoped to add_live_url_filters()'s + // admin-only set. + add_filter( 'image_downsize', array( $this, 'filter_downsize' ), 10, 3 ); + add_filter( 'wp_get_attachment_image_src', array( $this, 'filter_attachment_image_src' ), PHP_INT_MAX, 4 ); + // Filter default image Quality and Format transformations. add_filter( 'cloudinary_default_qf_transformations_image', array( $this, 'default_image_transformations' ), 10 ); add_filter( 'cloudinary_default_freeform_transformations_image', array( $this, 'default_image_freeform_transformations' ), 10 ); diff --git a/tests/phpunit/tests/test-attachment-image-src.php b/tests/phpunit/tests/test-attachment-image-src.php index 7de6d386d..ace091fb7 100644 --- a/tests/phpunit/tests/test-attachment-image-src.php +++ b/tests/phpunit/tests/test-attachment-image-src.php @@ -4,8 +4,11 @@ * * This covers the wp_get_attachment_image_src filter Cloudinary hooks so it keeps the last * word on that specific, commonly-targeted core hook (see WPP-1183): it must bow out cheaply - * when a URL is already a Cloudinary URL, leave icon/false results alone, and only correct a - * local URL when the attachment is actually deliverable and synced. + * when a URL is already a Cloudinary URL or there is no image at all, correct a local URL when + * the attachment is actually deliverable and synced regardless of the caller's $icon argument + * (WP_Media_List_Table's list-mode thumbnail column passes icon=true for every attachment, real + * images included -- see wp-admin/includes/class-wp-media-list-table.php), and otherwise leave + * the result alone. * * cloudinary_id() and cloudinary_url() are stubbed out (Test_Attachment_Image_Src_Media below) * rather than driven through the real sync/signature pipeline, matching the approach in @@ -97,15 +100,53 @@ public function test_already_cloudinary_url_is_returned_unchanged() { } /** - * The icon fallback path in wp_get_attachment_image_src() is left alone entirely. + * $icon is the caller's permission to fall back to a mime-type icon if image_downsize() + * found nothing -- WordPress passes the original argument through to the filter unchanged, + * regardless of whether $image is actually a real image or a fallback icon (see + * wp_get_attachment_image_src() in wp-includes/media.php). WP_Media_List_Table's list-mode + * thumbnail column calls wp_get_attachment_image() with icon=true for every attachment, real + * synced images included, so icon=true must not by itself block correction. * * @return void */ - public function test_icon_result_is_returned_unchanged() { + public function test_icon_true_does_not_block_correcting_a_real_synced_image() { + $media = $this->get_media(); + $media->stub_cloudinary_id = 'sample.jpg'; + $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/sample.jpg'; + $image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true ); + + $result = $this->without_saving_metadata_guard( + function () use ( $media, $image ) { + return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', true ); + } + ); + + $this->assertSame( $media->stub_cloudinary_url, $result[0] ); + } + + /** + * A genuine mime-icon fallback (no cloudinary_id to correct it to, e.g. an unsynced or + * non-deliverable attachment) is still left alone -- not because of the $icon flag, but + * because there is nothing to swap it for. + * + * @return void + */ + public function test_icon_fallback_is_unchanged_when_nothing_to_correct_it_to() { + $bare_id = self::factory()->post->create( + array( + 'post_type' => 'attachment', + 'post_mime_type' => 'image/jpeg', + ) + ); + $media = $this->get_media(); $image = array( 'http://example.org/wp-includes/images/media/default.png', 48, 64, false ); - $result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', true ); + $result = $this->without_saving_metadata_guard( + function () use ( $media, $image, $bare_id ) { + return $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', true ); + } + ); $this->assertSame( $image, $result ); } From da113bf751af4bc960dc4f7b08c0818545faf8e9 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Mon, 5 Oct 2026 09:06:51 +0200 Subject: [PATCH 3/5] fix(media): restore hookdoc adjacency for cloudinary_doing_upload filter Moving the docblock above the if statement (rather than immediately before the apply_filters() call it documents) broke PHPStan's parsing of the @param tag, caught by composer phpstan ahead of opening the PR. Keeps the same relative position as before, just inside the inverted condition. --- php/class-media.php | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/php/class-media.php b/php/class-media.php index 8417bc224..32ef7ab7d 100644 --- a/php/class-media.php +++ b/php/class-media.php @@ -1186,15 +1186,18 @@ public function attachment_url( $url, $attachment_id ) { return str_replace( trailingslashit( $dirs['baseurl'] ), '', $url ); } - /** - * Filter doing upload. - * If so, return the default attachment URL. - * - * @param bool Default false. - * - * @return bool - */ - if ( $this->is_replacement_paused( $attachment_id, false ) || apply_filters( 'cloudinary_doing_upload', false ) ) { + if ( + $this->is_replacement_paused( $attachment_id, false ) + /** + * Filter doing upload. + * If so, return the default attachment URL. + * + * @param bool Default false. + * + * @return bool + */ + || apply_filters( 'cloudinary_doing_upload', false ) + ) { return $url; } From 58c164ed40ebbcf4c5f660a771f7ef06b15327e0 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Mon, 5 Oct 2026 09:33:44 +0200 Subject: [PATCH 4/5] docs(media): clarify the positional false in attachment_url()'s is_replacement_paused() call Addresses a readability nit from PR review: the bool was unexplained at the call site, requiring a trip to the method signature to know what it toggles. --- php/class-media.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/php/class-media.php b/php/class-media.php index 32ef7ab7d..84038bd69 100644 --- a/php/class-media.php +++ b/php/class-media.php @@ -1187,7 +1187,7 @@ public function attachment_url( $url, $attachment_id ) { } if ( - $this->is_replacement_paused( $attachment_id, false ) + $this->is_replacement_paused( $attachment_id, false ) // false: videos still need their own URL corrected here. /** * Filter doing upload. * If so, return the default attachment URL. From 6bfc2a577b7d6be5c8ae54d07320f8d8705c97d1 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Mon, 5 Oct 2026 15:11:34 +0200 Subject: [PATCH 5/5] fix(media): address PR review feedback on wp_get_attachment_image_src filter Utils::is_saving_metadata() checked did_action(), which is cumulative for the whole request: any post/term/user meta write earlier in the request (a view counter, a session plugin, etc.) would leave it true for the rest of the page once filter_attachment_image_src()/filter_downsize() started running on the front end, permanently blocking correction. Switches to doing_action() to detect a write actually in progress. It has a single caller in the plugin (Media::is_replacement_paused()), so this is a contained fix. filter_attachment_image_src() also treated every deliverable+synced attachment as correctable, but Delivery::is_deliverable() returns true for any non-image, non-video type -- so a synced non-image attachment (audio, zip, docx, ...) requested with icon=true (as WP_Media_List_Table always does) would have its generic mime icon replaced with the raw asset's Cloudinary URL, breaking the resulting tag. Scopes correction to wp_attachment_is_image() || is_preview_only(), matching the preview-capable-formats carve-out filter_downsize() already uses. Test changes: added coverage for both fixes, including a regression test for an unrelated earlier meta write no longer blocking correction. Also fixed several existing tests that were passing for the wrong reason -- they relied on wp_attachment_is()/wp_attachment_is_image(), which return false unconditionally without a real _wp_attached_file postmeta regardless of post_mime_type, and a couple never set stub_cloudinary_url, so the asserted "stays unchanged" outcome held even with the check they meant to cover disabled. Every bow-out path was mutation-tested (temporarily disabled, confirmed the test fails, restored) rather than assumed correct. --- php/class-media.php | 9 + php/class-utils.php | 8 +- .../tests/test-attachment-image-src.php | 176 +++++++++++------- 3 files changed, 121 insertions(+), 72 deletions(-) diff --git a/php/class-media.php b/php/class-media.php index 84038bd69..cf4a3370b 100644 --- a/php/class-media.php +++ b/php/class-media.php @@ -1881,6 +1881,15 @@ public function filter_attachment_image_src( $image, $attachment_id, $size, $ico return $image; } + // Only images and preview-capable formats (PDF, PSD) have a real image representation to + // correct to. is_deliverable() treats every other attachment type as deliverable too, so + // without this, a synced non-image (audio, zip, docx, ...) requested with icon=true would + // have its generic mime icon replaced with the raw asset's Cloudinary URL, breaking the + // resulting tag. + if ( ! wp_attachment_is_image( $attachment_id ) && ! $this->is_preview_only( $attachment_id ) ) { + return $image; + } + if ( $this->is_replacement_paused( $attachment_id ) ) { return $image; } diff --git a/php/class-utils.php b/php/class-utils.php index 8674a584a..2f3af7459 100644 --- a/php/class-utils.php +++ b/php/class-utils.php @@ -720,7 +720,11 @@ public static function strip_inline_svg_data_uris( $content ) { /** - * Is saving metadata. + * Is metadata currently being saved, right now. + * + * Uses doing_action(), not did_action(): the latter is cumulative for the whole request, so + * it would stay true for the rest of the page after any earlier, unrelated post/term/user + * meta write (a view counter, a session plugin, etc.), long after that write finished. * * @return bool */ @@ -731,7 +735,7 @@ public static function is_saving_metadata() { foreach ( $metadata['actions'] as $action ) { foreach ( $metadata['objects'] as $object ) { $inline_action = str_replace( array( '{object}', 'metadata' ), array( $object, 'meta' ), $action ); - if ( did_action( $inline_action ) ) { + if ( doing_action( $inline_action ) ) { $saving = true; break; } diff --git a/tests/phpunit/tests/test-attachment-image-src.php b/tests/phpunit/tests/test-attachment-image-src.php index ace091fb7..b5ab3efbf 100644 --- a/tests/phpunit/tests/test-attachment-image-src.php +++ b/tests/phpunit/tests/test-attachment-image-src.php @@ -4,11 +4,13 @@ * * This covers the wp_get_attachment_image_src filter Cloudinary hooks so it keeps the last * word on that specific, commonly-targeted core hook (see WPP-1183): it must bow out cheaply - * when a URL is already a Cloudinary URL or there is no image at all, correct a local URL when - * the attachment is actually deliverable and synced regardless of the caller's $icon argument - * (WP_Media_List_Table's list-mode thumbnail column passes icon=true for every attachment, real - * images included -- see wp-admin/includes/class-wp-media-list-table.php), and otherwise leave - * the result alone. + * when a URL is already a Cloudinary URL or there is no image at all, only correct an attachment + * that actually has a real image representation (an image, or a preview-capable format like PDF/ + * PSD -- anything else showing here with icon=true is a generic mime icon, not something to + * replace with the raw asset URL), correct a local URL when the attachment is actually + * deliverable and synced regardless of the caller's $icon argument (WP_Media_List_Table's + * list-mode thumbnail column passes icon=true for every attachment, real images included -- see + * wp-admin/includes/class-wp-media-list-table.php), and otherwise leave the result alone. * * cloudinary_id() and cloudinary_url() are stubbed out (Test_Attachment_Image_Src_Media below) * rather than driven through the real sync/signature pipeline, matching the approach in @@ -54,36 +56,6 @@ protected function get_media() { return $media; } - /** - * Utils::is_saving_metadata() reads did_action() counters (add_post_meta, etc.) that - * WordPress never resets between tests within one PHPUnit process -- any earlier test, or - * even our own wpSetUpBeforeClass() fixture, already ticks them past zero. Momentarily - * clearing them here isolates the checks that come after that guard in - * filter_attachment_image_src(), so those tests exercise their own branch instead of - * always bowing out at is_saving_metadata() for an unrelated, process-wide reason. - * - * @param callable $callback Callback to run with the guard cleared. - * - * @return mixed - */ - protected function without_saving_metadata_guard( callable $callback ) { - $keys = array( 'add_post_meta', 'update_post_meta', 'add_term_meta', 'update_term_meta', 'add_user_meta', 'update_user_meta' ); - $saved = array(); - foreach ( $keys as $key ) { - if ( isset( $GLOBALS['wp_actions'][ $key ] ) ) { - $saved[ $key ] = $GLOBALS['wp_actions'][ $key ]; - unset( $GLOBALS['wp_actions'][ $key ] ); - } - } - try { - return $callback(); - } finally { - foreach ( $saved as $key => $value ) { - $GLOBALS['wp_actions'][ $key ] = $value; - } - } - } - /** * A URL that already lives on the configured Cloudinary domain is returned untouched -- the * cheap bow-out path the ticket calls for, taken before any sync/delivery check runs. @@ -91,8 +63,10 @@ protected function without_saving_metadata_guard( callable $callback ) { * @return void */ public function test_already_cloudinary_url_is_returned_unchanged() { - $media = $this->get_media(); - $image = array( 'https://res.cloudinary.com/test-cloud/image/upload/v1/sample.jpg', 100, 100, true ); + $media = $this->get_media(); + $media->stub_cloudinary_id = 'sample.jpg'; + $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/should-not-be-used.jpg'; + $image = array( 'https://res.cloudinary.com/test-cloud/image/upload/v1/sample.jpg', 100, 100, true ); $result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); @@ -115,11 +89,7 @@ public function test_icon_true_does_not_block_correcting_a_real_synced_image() { $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/sample.jpg'; $image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true ); - $result = $this->without_saving_metadata_guard( - function () use ( $media, $image ) { - return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', true ); - } - ); + $result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', true ); $this->assertSame( $media->stub_cloudinary_url, $result[0] ); } @@ -138,19 +108,71 @@ public function test_icon_fallback_is_unchanged_when_nothing_to_correct_it_to() 'post_mime_type' => 'image/jpeg', ) ); + // wp_attachment_is()/wp_attachment_is_image() return false unconditionally without a + // real _wp_attached_file, regardless of post_mime_type -- see wp-includes/post.php. + update_post_meta( $bare_id, '_wp_attached_file', 'bare.jpg' ); $media = $this->get_media(); $image = array( 'http://example.org/wp-includes/images/media/default.png', 48, 64, false ); - $result = $this->without_saving_metadata_guard( - function () use ( $media, $image, $bare_id ) { - return $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', true ); - } + $result = $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', true ); + + $this->assertSame( $image, $result ); + } + + /** + * A synced non-image, non-preview attachment (audio, zip, docx, ...) requested with icon=true + * must keep its generic mime icon. Delivery::is_deliverable() treats every non-image, + * non-video attachment as deliverable, so without a type check here, this would otherwise get + * "corrected" to the raw asset's Cloudinary URL -- breaking the resulting tag (caught in + * PR review: WP_Media_List_Table's list view requests icon=true for every attachment type). + * + * @return void + */ + public function test_synced_non_image_attachment_with_icon_is_returned_unchanged() { + $zip_id = self::factory()->post->create( + array( + 'post_type' => 'attachment', + 'post_mime_type' => 'application/zip', + ) ); + update_post_meta( $zip_id, '_wp_attached_file', 'archive.zip' ); + + $media = $this->get_media(); + $media->stub_cloudinary_id = 'archive.zip'; + $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/raw/upload/archive.zip'; + $image = array( 'http://example.org/wp-includes/images/media/archive.png', 48, 64, false ); + + $result = $media->filter_attachment_image_src( $image, $zip_id, 'thumbnail', true ); $this->assertSame( $image, $result ); } + /** + * A synced preview-capable format (PDF/PSD) is the deliberate carve-out: it has a real image + * preview to correct to, so icon=true must not block it either, mirroring the image case. + * + * @return void + */ + public function test_synced_preview_only_attachment_with_icon_is_corrected() { + $pdf_id = self::factory()->post->create( + array( + 'post_type' => 'attachment', + 'post_mime_type' => 'application/pdf', + ) + ); + update_post_meta( $pdf_id, '_wp_attached_file', 'document.pdf' ); + + $media = $this->get_media(); + $media->stub_cloudinary_id = 'document.pdf'; + $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/document.jpg'; + $image = array( 'http://example.org/wp-includes/images/media/document.png', 48, 64, false ); + + $result = $media->filter_attachment_image_src( $image, $pdf_id, 'thumbnail', true ); + + $this->assertSame( $media->stub_cloudinary_url, $result[0] ); + } + /** * A false (no image) result has nothing to correct and must pass through unchanged. * @@ -163,7 +185,9 @@ public function test_false_image_is_returned_unchanged() { } /** - * A video attachment is left untouched, mirroring the same guard filter_downsize() uses. + * A video attachment is left untouched -- caught by the image/preview-only type check before + * ever reaching the deliverable/sync checks (a video has no image representation to correct + * to here; its own URL is handled by attachment_url() instead). * * @return void */ @@ -174,15 +198,12 @@ public function test_video_attachment_is_returned_unchanged() { 'post_mime_type' => 'video/mp4', ) ); + update_post_meta( $video_id, '_wp_attached_file', 'video.mp4' ); $media = $this->get_media(); $image = array( 'http://example.org/wp-content/uploads/video.mp4', 640, 360, true ); - $result = $this->without_saving_metadata_guard( - function () use ( $media, $image, $video_id ) { - return $media->filter_attachment_image_src( $image, $video_id, 'thumbnail', false ); - } - ); + $result = $media->filter_attachment_image_src( $image, $video_id, 'thumbnail', false ); $this->assertSame( $image, $result ); } @@ -200,16 +221,14 @@ public function test_non_deliverable_attachment_is_returned_unchanged() { 'post_mime_type' => 'image/jpeg', ) ); + update_post_meta( $bare_id, '_wp_attached_file', 'bare.jpg' ); - $media = $this->get_media(); - $media->stub_cloudinary_id = 'sample.jpg'; - $image = array( 'http://example.org/wp-content/uploads/bare.jpg', 100, 100, true ); + $media = $this->get_media(); + $media->stub_cloudinary_id = 'sample.jpg'; + $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/should-not-be-used.jpg'; + $image = array( 'http://example.org/wp-content/uploads/bare.jpg', 100, 100, true ); - $result = $this->without_saving_metadata_guard( - function () use ( $media, $image, $bare_id ) { - return $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', false ); - } - ); + $result = $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', false ); $this->assertSame( $image, $result ); } @@ -226,11 +245,7 @@ public function test_no_cloudinary_id_returns_image_unchanged() { $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/should-not-be-used.jpg'; $image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true ); - $result = $this->without_saving_metadata_guard( - function () use ( $media, $image ) { - return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); - } - ); + $result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); $this->assertSame( $image, $result ); } @@ -247,17 +262,38 @@ public function test_local_url_is_corrected_to_cloudinary_url() { $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/sample.jpg'; $image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true ); - $result = $this->without_saving_metadata_guard( - function () use ( $media, $image ) { - return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); - } - ); + $result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); $this->assertSame( $media->stub_cloudinary_url, $result[0] ); $this->assertSame( 100, $result[1] ); $this->assertSame( 100, $result[2] ); $this->assertTrue( $result[3] ); } + + /** + * Regression test for a PR review finding: Utils::is_saving_metadata() must reflect a meta + * write happening right now, not one that happened earlier in the request. It used to read + * did_action(), which is cumulative for the whole request -- so an unrelated post/term/user + * meta write anywhere earlier (a view counter, a session plugin, anything) would permanently + * block correction for the rest of the page once these filters started running on the front + * end. Writing unrelated meta here, before calling the filter, catches a regression back to + * that behaviour. + * + * @return void + */ + public function test_earlier_unrelated_meta_write_does_not_block_correction() { + $other_post_id = self::factory()->post->create(); + update_post_meta( $other_post_id, 'unrelated_counter', 1 ); + + $media = $this->get_media(); + $media->stub_cloudinary_id = 'sample.jpg'; + $media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/sample.jpg'; + $image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true ); + + $result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false ); + + $this->assertSame( $media->stub_cloudinary_url, $result[0] ); + } } /**