Repository navigation
fix: hook wp_get_attachment_image_src to keep Cloudinary URLs as the last word - #1303
gabrielcld2 wants to merge 5 commits into
Conversation
…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).
…de 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).
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.
…placement_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.
utkarshcloudinary
left a comment
There was a problem hiding this comment.
Looks good, Left 2 suggestions!
| // admin-only set. | ||
| add_filter( 'image_downsize', array( $this, 'filter_downsize' ), 10, 3 ); |
There was a problem hiding this comment.
blocker: These filters now run on the front end, but both bail out through is_replacement_paused() -> Utils::is_saving_metadata(), which is based on did_action(). After any post, term, or user meta write earlier in the request (view counters, session or analytics plugins, etc.), that check stays true until the request ends. Synced images then keep their local URLs for the rest of the page, which defeats the purpose of this change.
The new test file has to clear $GLOBALS['wp_actions'] in without_saving_metadata_guard() because of this same behaviour.
The guard should detect a metadata write that is running now (for example doing_action() / doing_filter(), or a flag set and cleared around the write), not one that ran before.
There was a problem hiding this comment.
Good catch! This should be fixed now 6bfc2a5
| $url = $this->cloudinary_url( $attachment_id, $size, array(), $cloudinary_id ); | ||
| if ( $url ) { | ||
| $image[0] = $url; |
There was a problem hiding this comment.
warning: When $icon is true for a synced non-image attachment (audio, zip, docx, ...), WordPress gives a MIME icon array here. Delivery::is_deliverable() returns true for every non-image, non-video type, so this replaces the icon with the raw Cloudinary asset URL, and the result is a broken <img> (for example in the media list view).
Please limit this correction to images and supported preview formats (wp_attachment_is_image() || is_preview_only()). Please also add a test for a synced non-image attachment with icon=true.
There was a problem hiding this comment.
That makes sense, thanks. Fixed as part of 6bfc2a5
… 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 <img> 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.
Approach
wp_get_attachment_image_srcis a commonly-targeted hook that runs afterimage_downsize(), so a competing plugin (or filter ordering) could leave a local URL in place with no second chance to correct it.Adds a filter on it that bows out immediately if the URL is already Cloudinary, and otherwise reuses the same deliverable/re-entrancy checks as
filter_downsize().Along the way:
Media::is_replacement_paused(), reused byfilter_downsize(), the new filter, andattachment_url().image_downsizeandwp_get_attachment_image_srccorrections unconditionally (not just admin-only). Both are exercised by ordinary front-end theme code (e.g.the_post_thumbnail()), not just wp-admin screens.Scope notes
wp_calculate_image_srcsetstays admin-only on purpose:srcnow self-corrects on the front end, butsrcsetbuilt directly viawp_get_attachment_image_srcset()outside post_content would not, since that filter isn't part of this change.Delivery::set_usability()) - not new. This PR adds a second path to that same existing behavior: any attachment rendered viawp_get_attachment_image()/image_downsize()on the front end, not just ones the content-rewrite pipeline recognizes in rendered HTML.QA notes
image_downsize():To reproduce yourself: