Skip to content

Commit 6bfc2a5

Browse files
gabriel-detassignygabriel-detassigny
committed
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 <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.
1 parent 58c164e commit 6bfc2a5

3 files changed

Lines changed: 121 additions & 72 deletions

File tree

‎php/class-media.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1881,6 +1881,15 @@ public function filter_attachment_image_src( $image, $attachment_id, $size, $ico
18811881
return $image;
18821882
}
18831883

1884+
// Only images and preview-capable formats (PDF, PSD) have a real image representation to
1885+
// correct to. is_deliverable() treats every other attachment type as deliverable too, so
1886+
// without this, a synced non-image (audio, zip, docx, ...) requested with icon=true would
1887+
// have its generic mime icon replaced with the raw asset's Cloudinary URL, breaking the
1888+
// resulting <img> tag.
1889+
if ( ! wp_attachment_is_image( $attachment_id ) && ! $this->is_preview_only( $attachment_id ) ) {
1890+
return $image;
1891+
}
1892+
18841893
if ( $this->is_replacement_paused( $attachment_id ) ) {
18851894
return $image;
18861895
}

‎php/class-utils.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,11 @@ public static function strip_inline_svg_data_uris( $content ) {
720720

721721

722722
/**
723-
* Is saving metadata.
723+
* Is metadata currently being saved, right now.
724+
*
725+
* Uses doing_action(), not did_action(): the latter is cumulative for the whole request, so
726+
* it would stay true for the rest of the page after any earlier, unrelated post/term/user
727+
* meta write (a view counter, a session plugin, etc.), long after that write finished.
724728
*
725729
* @return bool
726730
*/
@@ -731,7 +735,7 @@ public static function is_saving_metadata() {
731735
foreach ( $metadata['actions'] as $action ) {
732736
foreach ( $metadata['objects'] as $object ) {
733737
$inline_action = str_replace( array( '{object}', 'metadata' ), array( $object, 'meta' ), $action );
734-
if ( did_action( $inline_action ) ) {
738+
if ( doing_action( $inline_action ) ) {
735739
$saving = true;
736740
break;
737741
}

‎tests/phpunit/tests/test-attachment-image-src.php‎

Lines changed: 106 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,13 @@
44
*
55
* This covers the wp_get_attachment_image_src filter Cloudinary hooks so it keeps the last
66
* word on that specific, commonly-targeted core hook (see WPP-1183): it must bow out cheaply
7-
* when a URL is already a Cloudinary URL or there is no image at all, correct a local URL when
8-
* the attachment is actually deliverable and synced regardless of the caller's $icon argument
9-
* (WP_Media_List_Table's list-mode thumbnail column passes icon=true for every attachment, real
10-
* images included -- see wp-admin/includes/class-wp-media-list-table.php), and otherwise leave
11-
* the result alone.
7+
* when a URL is already a Cloudinary URL or there is no image at all, only correct an attachment
8+
* that actually has a real image representation (an image, or a preview-capable format like PDF/
9+
* PSD -- anything else showing here with icon=true is a generic mime icon, not something to
10+
* replace with the raw asset URL), correct a local URL when the attachment is actually
11+
* deliverable and synced regardless of the caller's $icon argument (WP_Media_List_Table's
12+
* list-mode thumbnail column passes icon=true for every attachment, real images included -- see
13+
* wp-admin/includes/class-wp-media-list-table.php), and otherwise leave the result alone.
1214
*
1315
* cloudinary_id() and cloudinary_url() are stubbed out (Test_Attachment_Image_Src_Media below)
1416
* rather than driven through the real sync/signature pipeline, matching the approach in
@@ -54,45 +56,17 @@ protected function get_media() {
5456
return $media;
5557
}
5658

57-
/**
58-
* Utils::is_saving_metadata() reads did_action() counters (add_post_meta, etc.) that
59-
* WordPress never resets between tests within one PHPUnit process -- any earlier test, or
60-
* even our own wpSetUpBeforeClass() fixture, already ticks them past zero. Momentarily
61-
* clearing them here isolates the checks that come after that guard in
62-
* filter_attachment_image_src(), so those tests exercise their own branch instead of
63-
* always bowing out at is_saving_metadata() for an unrelated, process-wide reason.
64-
*
65-
* @param callable $callback Callback to run with the guard cleared.
66-
*
67-
* @return mixed
68-
*/
69-
protected function without_saving_metadata_guard( callable $callback ) {
70-
$keys = array( 'add_post_meta', 'update_post_meta', 'add_term_meta', 'update_term_meta', 'add_user_meta', 'update_user_meta' );
71-
$saved = array();
72-
foreach ( $keys as $key ) {
73-
if ( isset( $GLOBALS['wp_actions'][ $key ] ) ) {
74-
$saved[ $key ] = $GLOBALS['wp_actions'][ $key ];
75-
unset( $GLOBALS['wp_actions'][ $key ] );
76-
}
77-
}
78-
try {
79-
return $callback();
80-
} finally {
81-
foreach ( $saved as $key => $value ) {
82-
$GLOBALS['wp_actions'][ $key ] = $value;
83-
}
84-
}
85-
}
86-
8759
/**
8860
* A URL that already lives on the configured Cloudinary domain is returned untouched -- the
8961
* cheap bow-out path the ticket calls for, taken before any sync/delivery check runs.
9062
*
9163
* @return void
9264
*/
9365
public function test_already_cloudinary_url_is_returned_unchanged() {
94-
$media = $this->get_media();
95-
$image = array( 'https://res.cloudinary.com/test-cloud/image/upload/v1/sample.jpg', 100, 100, true );
66+
$media = $this->get_media();
67+
$media->stub_cloudinary_id = 'sample.jpg';
68+
$media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/should-not-be-used.jpg';
69+
$image = array( 'https://res.cloudinary.com/test-cloud/image/upload/v1/sample.jpg', 100, 100, true );
9670

9771
$result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false );
9872

@@ -115,11 +89,7 @@ public function test_icon_true_does_not_block_correcting_a_real_synced_image() {
11589
$media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/sample.jpg';
11690
$image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true );
11791

118-
$result = $this->without_saving_metadata_guard(
119-
function () use ( $media, $image ) {
120-
return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', true );
121-
}
122-
);
92+
$result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', true );
12393

12494
$this->assertSame( $media->stub_cloudinary_url, $result[0] );
12595
}
@@ -138,19 +108,71 @@ public function test_icon_fallback_is_unchanged_when_nothing_to_correct_it_to()
138108
'post_mime_type' => 'image/jpeg',
139109
)
140110
);
111+
// wp_attachment_is()/wp_attachment_is_image() return false unconditionally without a
112+
// real _wp_attached_file, regardless of post_mime_type -- see wp-includes/post.php.
113+
update_post_meta( $bare_id, '_wp_attached_file', 'bare.jpg' );
141114

142115
$media = $this->get_media();
143116
$image = array( 'http://example.org/wp-includes/images/media/default.png', 48, 64, false );
144117

145-
$result = $this->without_saving_metadata_guard(
146-
function () use ( $media, $image, $bare_id ) {
147-
return $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', true );
148-
}
118+
$result = $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', true );
119+
120+
$this->assertSame( $image, $result );
121+
}
122+
123+
/**
124+
* A synced non-image, non-preview attachment (audio, zip, docx, ...) requested with icon=true
125+
* must keep its generic mime icon. Delivery::is_deliverable() treats every non-image,
126+
* non-video attachment as deliverable, so without a type check here, this would otherwise get
127+
* "corrected" to the raw asset's Cloudinary URL -- breaking the resulting <img> tag (caught in
128+
* PR review: WP_Media_List_Table's list view requests icon=true for every attachment type).
129+
*
130+
* @return void
131+
*/
132+
public function test_synced_non_image_attachment_with_icon_is_returned_unchanged() {
133+
$zip_id = self::factory()->post->create(
134+
array(
135+
'post_type' => 'attachment',
136+
'post_mime_type' => 'application/zip',
137+
)
149138
);
139+
update_post_meta( $zip_id, '_wp_attached_file', 'archive.zip' );
140+
141+
$media = $this->get_media();
142+
$media->stub_cloudinary_id = 'archive.zip';
143+
$media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/raw/upload/archive.zip';
144+
$image = array( 'http://example.org/wp-includes/images/media/archive.png', 48, 64, false );
145+
146+
$result = $media->filter_attachment_image_src( $image, $zip_id, 'thumbnail', true );
150147

151148
$this->assertSame( $image, $result );
152149
}
153150

151+
/**
152+
* A synced preview-capable format (PDF/PSD) is the deliberate carve-out: it has a real image
153+
* preview to correct to, so icon=true must not block it either, mirroring the image case.
154+
*
155+
* @return void
156+
*/
157+
public function test_synced_preview_only_attachment_with_icon_is_corrected() {
158+
$pdf_id = self::factory()->post->create(
159+
array(
160+
'post_type' => 'attachment',
161+
'post_mime_type' => 'application/pdf',
162+
)
163+
);
164+
update_post_meta( $pdf_id, '_wp_attached_file', 'document.pdf' );
165+
166+
$media = $this->get_media();
167+
$media->stub_cloudinary_id = 'document.pdf';
168+
$media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/document.jpg';
169+
$image = array( 'http://example.org/wp-includes/images/media/document.png', 48, 64, false );
170+
171+
$result = $media->filter_attachment_image_src( $image, $pdf_id, 'thumbnail', true );
172+
173+
$this->assertSame( $media->stub_cloudinary_url, $result[0] );
174+
}
175+
154176
/**
155177
* A false (no image) result has nothing to correct and must pass through unchanged.
156178
*
@@ -163,7 +185,9 @@ public function test_false_image_is_returned_unchanged() {
163185
}
164186

165187
/**
166-
* A video attachment is left untouched, mirroring the same guard filter_downsize() uses.
188+
* A video attachment is left untouched -- caught by the image/preview-only type check before
189+
* ever reaching the deliverable/sync checks (a video has no image representation to correct
190+
* to here; its own URL is handled by attachment_url() instead).
167191
*
168192
* @return void
169193
*/
@@ -174,15 +198,12 @@ public function test_video_attachment_is_returned_unchanged() {
174198
'post_mime_type' => 'video/mp4',
175199
)
176200
);
201+
update_post_meta( $video_id, '_wp_attached_file', 'video.mp4' );
177202

178203
$media = $this->get_media();
179204
$image = array( 'http://example.org/wp-content/uploads/video.mp4', 640, 360, true );
180205

181-
$result = $this->without_saving_metadata_guard(
182-
function () use ( $media, $image, $video_id ) {
183-
return $media->filter_attachment_image_src( $image, $video_id, 'thumbnail', false );
184-
}
185-
);
206+
$result = $media->filter_attachment_image_src( $image, $video_id, 'thumbnail', false );
186207

187208
$this->assertSame( $image, $result );
188209
}
@@ -200,16 +221,14 @@ public function test_non_deliverable_attachment_is_returned_unchanged() {
200221
'post_mime_type' => 'image/jpeg',
201222
)
202223
);
224+
update_post_meta( $bare_id, '_wp_attached_file', 'bare.jpg' );
203225

204-
$media = $this->get_media();
205-
$media->stub_cloudinary_id = 'sample.jpg';
206-
$image = array( 'http://example.org/wp-content/uploads/bare.jpg', 100, 100, true );
226+
$media = $this->get_media();
227+
$media->stub_cloudinary_id = 'sample.jpg';
228+
$media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/should-not-be-used.jpg';
229+
$image = array( 'http://example.org/wp-content/uploads/bare.jpg', 100, 100, true );
207230

208-
$result = $this->without_saving_metadata_guard(
209-
function () use ( $media, $image, $bare_id ) {
210-
return $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', false );
211-
}
212-
);
231+
$result = $media->filter_attachment_image_src( $image, $bare_id, 'thumbnail', false );
213232

214233
$this->assertSame( $image, $result );
215234
}
@@ -226,11 +245,7 @@ public function test_no_cloudinary_id_returns_image_unchanged() {
226245
$media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/should-not-be-used.jpg';
227246
$image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true );
228247

229-
$result = $this->without_saving_metadata_guard(
230-
function () use ( $media, $image ) {
231-
return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false );
232-
}
233-
);
248+
$result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false );
234249

235250
$this->assertSame( $image, $result );
236251
}
@@ -247,17 +262,38 @@ public function test_local_url_is_corrected_to_cloudinary_url() {
247262
$media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/sample.jpg';
248263
$image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true );
249264

250-
$result = $this->without_saving_metadata_guard(
251-
function () use ( $media, $image ) {
252-
return $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false );
253-
}
254-
);
265+
$result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false );
255266

256267
$this->assertSame( $media->stub_cloudinary_url, $result[0] );
257268
$this->assertSame( 100, $result[1] );
258269
$this->assertSame( 100, $result[2] );
259270
$this->assertTrue( $result[3] );
260271
}
272+
273+
/**
274+
* Regression test for a PR review finding: Utils::is_saving_metadata() must reflect a meta
275+
* write happening right now, not one that happened earlier in the request. It used to read
276+
* did_action(), which is cumulative for the whole request -- so an unrelated post/term/user
277+
* meta write anywhere earlier (a view counter, a session plugin, anything) would permanently
278+
* block correction for the rest of the page once these filters started running on the front
279+
* end. Writing unrelated meta here, before calling the filter, catches a regression back to
280+
* that behaviour.
281+
*
282+
* @return void
283+
*/
284+
public function test_earlier_unrelated_meta_write_does_not_block_correction() {
285+
$other_post_id = self::factory()->post->create();
286+
update_post_meta( $other_post_id, 'unrelated_counter', 1 );
287+
288+
$media = $this->get_media();
289+
$media->stub_cloudinary_id = 'sample.jpg';
290+
$media->stub_cloudinary_url = 'https://res.cloudinary.com/test-cloud/image/upload/sample.jpg';
291+
$image = array( 'http://example.org/wp-content/uploads/canola.jpg', 100, 100, true );
292+
293+
$result = $media->filter_attachment_image_src( $image, self::$attachment_id, 'thumbnail', false );
294+
295+
$this->assertSame( $media->stub_cloudinary_url, $result[0] );
296+
}
261297
}
262298

263299
/**

0 commit comments

Comments
 (0)