From d23b52e6cdabbea3980bf738d8ae7d2d16f6abe9 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Wed, 26 Aug 2026 12:16:59 +0200 Subject: [PATCH 1/3] fix: only overwrite an existing Cloudinary asset when it's this attachment's own orphan PR #1182 treated any attachment with no locally-saved public_id as proof that a colliding Cloudinary asset was an orphan from that same attachment's own crashed upload, and overwrote it. That's wrong for a brand new attachment too, since it also has no public_id yet -- so a fresh upload that happens to derive the same public_id as an unrelated, older asset (e.g. WordPress reusing a filename across months) silently clobbers that older asset. Only take the overwrite path when the existing asset's byte size also matches the local file, which is true for a genuine orphan of this attachment's own upload but not for an unrelated collision. Fixes #1241 --- php/sync/class-upload-sync.php | 35 ++++++- tests/phpunit/tests/test-upload-sync.php | 118 +++++++++++++++++++++++ 2 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 tests/phpunit/tests/test-upload-sync.php diff --git a/php/sync/class-upload-sync.php b/php/sync/class-upload-sync.php index dd2d9100..aabd6ffa 100644 --- a/php/sync/class-upload-sync.php +++ b/php/sync/class-upload-sync.php @@ -338,9 +338,14 @@ function ( $is_synced, $post_id ) use ( $attachment_id ) { // Check that this wasn't an existing. if ( ! empty( $result['existing'] ) ) { - // If no public_id is recorded in WordPress, this asset in Cloudinary is from a - // failed previous upload. Overwrite it instead of creating a suffixed duplicate. - if ( empty( $suffix ) && ! $this->media->get_post_meta( $attachment_id, Sync::META_KEYS['public_id'], true ) ) { + // A missing public_id in WordPress isn't enough on its own to prove the conflicting + // Cloudinary asset is an orphan of this attachment's own failed upload -- any never + // synced attachment also has no public_id. Only treat it as our own orphan, safe to + // overwrite, when the existing asset's file size also matches the local file. + if ( empty( $suffix ) + && ! $this->media->get_post_meta( $attachment_id, Sync::META_KEYS['public_id'], true ) + && $this->is_matching_existing_asset( $attachment_id, $result ) + ) { return $this->upload_asset( $attachment_id, $type, null, true ); } // Add a suffix and try again. @@ -382,6 +387,30 @@ function ( $is_synced, $post_id ) use ( $attachment_id ) { return $result; } + /** + * Check whether a Cloudinary "existing" asset is likely this attachment's own local file. + * + * Used to tell apart an orphan left by this same attachment's previously interrupted upload + * (safe to overwrite) from an unrelated asset that happens to share the same derived public + * ID, e.g. WordPress reusing a filename across months (must not be overwritten). + * + * @param int $attachment_id The attachment ID. + * @param array $result The Cloudinary upload result. + * + * @return bool + */ + public function is_matching_existing_asset( $attachment_id, $result ) { + if ( empty( $result['bytes'] ) ) { + return false; + } + $file = get_attached_file( $attachment_id ); + if ( empty( $file ) || ! file_exists( $file ) ) { + return false; + } + + return (int) filesize( $file ) === (int) $result['bytes']; + } + /** * Update an assets context.. * diff --git a/tests/phpunit/tests/test-upload-sync.php b/tests/phpunit/tests/test-upload-sync.php new file mode 100644 index 00000000..40bd98a7 --- /dev/null +++ b/tests/phpunit/tests/test-upload-sync.php @@ -0,0 +1,118 @@ +attachment->create_upload_object( $file ); + self::$attachment_bytes = filesize( get_attached_file( self::$attachment_id ) ); + } + + /** + * Build an Upload_Sync instance. + * + * is_matching_existing_asset() only calls core get_attached_file()/filesize(), never touches + * $media/$sync/$connect, so the component doesn't need setup() to have wired those up. + * + * @return Upload_Sync + */ + protected function get_upload_sync() { + return new Upload_Sync( \Cloudinary\get_plugin_instance() ); + } + + /** + * An existing asset whose byte size matches the local file is treated as this attachment's + * own orphaned upload, so it's safe to overwrite. + * + * @return void + */ + public function test_matches_when_existing_asset_bytes_equal_the_local_file() { + $result = array( 'bytes' => self::$attachment_bytes ); + + $this->assertTrue( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * An existing asset with a different byte size is a different, unrelated asset -- the + * collision this attachment must not overwrite. + * + * @return void + */ + public function test_does_not_match_when_existing_asset_bytes_differ() { + $result = array( 'bytes' => self::$attachment_bytes + 1 ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * Without a `bytes` field to compare against, there's no basis to treat the collision as + * this attachment's own asset, so it must not be overwritten. + * + * @return void + */ + public function test_does_not_match_when_result_has_no_bytes_field() { + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, array() ) + ); + } + + /** + * Without a local file to compare against, there's no basis for a match either. + * + * @return void + */ + public function test_does_not_match_when_the_attachment_has_no_local_file() { + $post_id = self::factory()->post->create( array( 'post_type' => 'attachment' ) ); + + $result = array( 'bytes' => self::$attachment_bytes ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( $post_id, $result ) + ); + } +} From 9187312e044dcd5137f2fafbfc2fa2ec20916474 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Thu, 27 Aug 2026 11:03:04 +0200 Subject: [PATCH 2/3] fix: address review feedback on the existing-asset overwrite check - Compare against the same file Api::upload() actually sends, not always the attached file. For images over big_image_size_threshold, get_attached_file() returns the "-scaled" copy while the upload itself sends the unscaled original via wp_get_original_image_path(), so the two sizes never matched and the #1182 crash-recovery path silently stopped working for large images. Extracted the shared resolution into Media::get_upload_file_path(), used by both Api::upload() and the new check, instead of a third inline copy. - Confirm with the response's etag (MD5 of the stored asset) once byte sizes already match, closing the remaining false-positive where two unrelated files coincidentally share a byte count. - Log via Utils::log() when the check bails out for lack of a `bytes` field, so a future API response change doesn't silently resurrect the #1182 duplicate-per-cycle bug. - Mark is_matching_existing_asset() @internal and narrow its docblock to the sync type it actually runs for. Extends the test suite with the scaled-image and etag scenarios. --- php/class-media.php | 21 ++++++ php/connect/class-api.php | 7 +- php/sync/class-upload-sync.php | 21 ++++-- tests/phpunit/tests/test-upload-sync.php | 87 ++++++++++++++++++++++-- 4 files changed, 121 insertions(+), 15 deletions(-) diff --git a/php/class-media.php b/php/class-media.php index 43f95e68..3f4700c1 100644 --- a/php/class-media.php +++ b/php/class-media.php @@ -495,6 +495,27 @@ function_exists( 'wp_get_original_image_path' ) return $file_size; } + /** + * Get the local file path used to upload an attachment. + * + * Mirrors the file resolution in Connect\Api::upload(): the unscaled original when + * `cloudinary_use_original_image` allows it, the attached file otherwise -- e.g. the + * `-scaled` copy WordPress creates for images over `big_image_size_threshold`. + * + * @param int $attachment_id The attachment ID. + * + * @return string + */ + public function get_upload_file_path( $attachment_id ) { + /** This filter is documented in php/connect/class-api.php */ + $use_original = apply_filters( 'cloudinary_use_original_image', true, $attachment_id ); + if ( $use_original && function_exists( 'wp_get_original_image_path' ) && wp_attachment_is_image( $attachment_id ) ) { + return wp_get_original_image_path( $attachment_id ); + } + + return get_attached_file( $attachment_id ); + } + /** * Get the Cloudinary delivery type. * diff --git a/php/connect/class-api.php b/php/connect/class-api.php index d3687603..606ccda8 100644 --- a/php/connect/class-api.php +++ b/php/connect/class-api.php @@ -560,12 +560,7 @@ public function upload( $attachment_id, $args, $headers = array(), $try_remote = } else { // We should have the file in args at this point, but if the transient was set, it will be defaulting here. if ( empty( $args['file'] ) ) { - if ( wp_attachment_is_image( $attachment_id ) ) { - $get_path_func = $use_original && function_exists( 'wp_get_original_image_path' ) ? 'wp_get_original_image_path' : 'get_attached_file'; - $args['file'] = call_user_func( $get_path_func, $attachment_id ); - } else { - $args['file'] = get_attached_file( $attachment_id ); - } + $args['file'] = $this->media->get_upload_file_path( $attachment_id ); } // Headers indicate chunked upload. if ( empty( $headers ) && file_exists( $args['file'] ) ) { diff --git a/php/sync/class-upload-sync.php b/php/sync/class-upload-sync.php index aabd6ffa..dfe5d6b3 100644 --- a/php/sync/class-upload-sync.php +++ b/php/sync/class-upload-sync.php @@ -391,8 +391,12 @@ function ( $is_synced, $post_id ) use ( $attachment_id ) { * Check whether a Cloudinary "existing" asset is likely this attachment's own local file. * * Used to tell apart an orphan left by this same attachment's previously interrupted upload - * (safe to overwrite) from an unrelated asset that happens to share the same derived public - * ID, e.g. WordPress reusing a filename across months (must not be overwritten). + * of the default (non "folder"/"cloud_name") sync type (safe to overwrite) from an unrelated + * asset that happens to share the same derived public ID, e.g. WordPress reusing a filename + * across months (must not be overwritten). Only called once a public_id is unrecorded, so in + * practice this only ever runs for that default sync type; the other types always have one. + * + * @internal Reachable for testing; not intended to be called from outside this class. * * @param int $attachment_id The attachment ID. * @param array $result The Cloudinary upload result. @@ -401,14 +405,23 @@ function ( $is_synced, $post_id ) use ( $attachment_id ) { */ public function is_matching_existing_asset( $attachment_id, $result ) { if ( empty( $result['bytes'] ) ) { + Utils::log( + sprintf( 'Cloudinary upload result for attachment %d has no "bytes" field; treating as a non-matching asset.', $attachment_id ), + 'upload-sync-existing-asset-check' + ); + return false; } - $file = get_attached_file( $attachment_id ); + $file = $this->media->get_upload_file_path( $attachment_id ); if ( empty( $file ) || ! file_exists( $file ) ) { return false; } + if ( (int) filesize( $file ) !== (int) $result['bytes'] ) { + return false; + } - return (int) filesize( $file ) === (int) $result['bytes']; + // Bytes alone can coincide between unrelated files; confirm with the content hash when available. + return empty( $result['etag'] ) || md5_file( $file ) === $result['etag']; } /** diff --git a/tests/phpunit/tests/test-upload-sync.php b/tests/phpunit/tests/test-upload-sync.php index 40bd98a7..8d43bf8d 100644 --- a/tests/phpunit/tests/test-upload-sync.php +++ b/tests/phpunit/tests/test-upload-sync.php @@ -50,20 +50,25 @@ public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) { } /** - * Build an Upload_Sync instance. + * Build a fully wired Upload_Sync instance. * - * is_matching_existing_asset() only calls core get_attached_file()/filesize(), never touches - * $media/$sync/$connect, so the component doesn't need setup() to have wired those up. + * is_matching_existing_asset() reads the upload file path through $media, so setup() needs + * to have run to wire it -- the real Media component, already initialised by the plugin + * bootstrap, is reused rather than stubbed. * * @return Upload_Sync */ protected function get_upload_sync() { - return new Upload_Sync( \Cloudinary\get_plugin_instance() ); + $upload_sync = new Upload_Sync( \Cloudinary\get_plugin_instance() ); + $upload_sync->setup(); + + return $upload_sync; } /** * An existing asset whose byte size matches the local file is treated as this attachment's - * own orphaned upload, so it's safe to overwrite. + * own orphaned upload, so it's safe to overwrite. No etag in the result falls back to the + * byte comparison alone. * * @return void */ @@ -115,4 +120,76 @@ public function test_does_not_match_when_the_attachment_has_no_local_file() { $this->get_upload_sync()->is_matching_existing_asset( $post_id, $result ) ); } + + /** + * Matching bytes plus a matching etag (the MD5 of the stored asset) confirms the content + * itself, not just its size. + * + * @return void + */ + public function test_matches_when_bytes_and_etag_both_match() { + $result = array( + 'bytes' => self::$attachment_bytes, + 'etag' => md5_file( get_attached_file( self::$attachment_id ) ), + ); + + $this->assertTrue( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * A byte size that coincidentally matches an unrelated file must not be enough on its own + * once an etag is available to rule it out. + * + * @return void + */ + public function test_does_not_match_when_bytes_match_but_etag_differs() { + $result = array( + 'bytes' => self::$attachment_bytes, + 'etag' => 'not-the-real-hash', + ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * Cloudinary uploads the unscaled original for a "-scaled" image (the file WordPress + * attaches for images over big_image_size_threshold is a downsized copy, not what was + * actually sent), so the check must compare against that original, not the attached file. + * + * @return void + */ + public function test_matches_using_the_unscaled_original_for_a_scaled_image() { + $id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' ); + + $original_file = get_attached_file( $id ); + $scaled_file = dirname( $original_file ) . '/canola-scaled.jpg'; + + // Stand in for the "-scaled" file WordPress would attach: same starting bytes, padded + // so its size provably differs from the original left alongside it. + copy( $original_file, $scaled_file ); + file_put_contents( $scaled_file, file_get_contents( $scaled_file ) . 'padding' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents, WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents + update_attached_file( $id, $scaled_file ); + + $metadata = wp_get_attachment_metadata( $id ); + $metadata['original_image'] = wp_basename( $original_file ); + wp_update_attachment_metadata( $id, $metadata ); + + $original_bytes = filesize( $original_file ); + $scaled_bytes = filesize( $scaled_file ); + + $this->assertNotSame( $original_bytes, $scaled_bytes, 'Fixture files must differ in size for this test to be meaningful.' ); + + // Cloudinary was sent the original -- its bytes must be what's compared against. + $this->assertTrue( + $this->get_upload_sync()->is_matching_existing_asset( $id, array( 'bytes' => $original_bytes ) ) + ); + // The attached (scaled) file's size is not what was actually uploaded. + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( $id, array( 'bytes' => $scaled_bytes ) ) + ); + } } From 7a3c44348023bf1021280caa5239fd41a1e25433 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Tue, 1 Sep 2026 15:59:09 +0200 Subject: [PATCH 3/3] fix: close remaining gaps in the existing-asset overwrite check - vip:// paths: skip the etag/md5 hash and rely on byte size alone. Hashing a VIP stream-wrapper path pulls the whole object over the network, and a failed read returns false rather than throwing -- which the etag check would misread as a content mismatch, reintroducing the #1182 duplicate-per-cycle bug on VIP specifically. - Media::get_upload_file_path(): corrected @return to string|false, matching the underlying get_attached_file()/wp_get_original_image_path() core functions. - Byte/etag equality alone isn't ownership: two unrelated attachments can hold byte-identical files that derive the same public ID. Added is_solely_linked_to(), mirroring the guard Delete_Sync::delete_asset() already uses, so the overwrite path now also requires that no other attachment is already tracked as linked to the public ID before treating a collision as this attachment's own orphan. Extends the test suite with the ownership-conflict, vip://, and missing-public_id scenarios (29 tests). --- php/class-media.php | 2 +- php/sync/class-upload-sync.php | 39 +++++ tests/phpunit/tests/test-upload-sync.php | 183 +++++++++++++++++++++-- 3 files changed, 213 insertions(+), 11 deletions(-) diff --git a/php/class-media.php b/php/class-media.php index 3f4700c1..8a98dc47 100644 --- a/php/class-media.php +++ b/php/class-media.php @@ -504,7 +504,7 @@ function_exists( 'wp_get_original_image_path' ) * * @param int $attachment_id The attachment ID. * - * @return string + * @return string|false */ public function get_upload_file_path( $attachment_id ) { /** This filter is documented in php/connect/class-api.php */ diff --git a/php/sync/class-upload-sync.php b/php/sync/class-upload-sync.php index dfe5d6b3..aa8b7f9d 100644 --- a/php/sync/class-upload-sync.php +++ b/php/sync/class-upload-sync.php @@ -412,6 +412,12 @@ public function is_matching_existing_asset( $attachment_id, $result ) { return false; } + // Byte-identical content between two unrelated attachments isn't proof of ownership: the + // second overwrite would still clobber the first's context and advance its version. Only + // proceed if no other attachment already claims this public ID. + if ( ! $this->is_solely_linked_to( $attachment_id, empty( $result['public_id'] ) ? null : $result['public_id'] ) ) { + return false; + } $file = $this->media->get_upload_file_path( $attachment_id ); if ( empty( $file ) || ! file_exists( $file ) ) { return false; @@ -419,11 +425,44 @@ public function is_matching_existing_asset( $attachment_id, $result ) { if ( (int) filesize( $file ) !== (int) $result['bytes'] ) { return false; } + // Hashing a vip:// stream wrapper path pulls the whole object over the network; a failed + // read returns false rather than throwing, which would wrongly read as a mismatch. Bytes + // alone is the safer signal to rely on there. + if ( false !== strpos( $file, 'vip://' ) ) { + return true; + } // Bytes alone can coincide between unrelated files; confirm with the content hash when available. return empty( $result['etag'] ) || md5_file( $file ) === $result['etag']; } + /** + * Check that no other attachment is already tracked as linked to a public ID. + * + * Mirrors the ownership guard Delete_Sync::delete_asset() uses before destroying an asset. + * + * @param int $attachment_id The attachment ID. + * @param string|null $public_id The public ID to check. + * + * @return bool + */ + protected function is_solely_linked_to( $attachment_id, $public_id ) { + if ( empty( $public_id ) ) { + return false; + } + $linked = $this->media->get_linked_attachments( $public_id ); + if ( count( $linked ) > 1 ) { + // More than one attachment already shares this public ID. + return false; + } + if ( 1 === count( $linked ) && (int) $attachment_id !== (int) $linked[0] ) { + // Exactly one other attachment is already linked to it. + return false; + } + + return true; + } + /** * Update an assets context.. * diff --git a/tests/phpunit/tests/test-upload-sync.php b/tests/phpunit/tests/test-upload-sync.php index 8d43bf8d..2b81fcd2 100644 --- a/tests/phpunit/tests/test-upload-sync.php +++ b/tests/phpunit/tests/test-upload-sync.php @@ -6,7 +6,8 @@ * Cloudinary asset blocking an upload (existing: true) is safe to overwrite. It should only * be treated as this attachment's own orphaned upload -- not an unrelated asset that happens * to share the same derived public ID, e.g. WordPress reusing a filename across months (see - * GitHub issue #1241). + * GitHub issue #1241), and not an unrelated attachment that happens to hold byte-identical + * content and already owns that public ID. * * The rest of upload_asset() talks to the Cloudinary API over HTTP and is covered by the * Playwright suite in tests/e2e. @@ -65,6 +66,19 @@ protected function get_upload_sync() { return $upload_sync; } + /** + * Mark an attachment as linked to a public ID, the way a completed upload_asset() call + * would via its trackable postmeta key -- what get_linked_attachments() looks up. + * + * @param int $attachment_id The attachment ID. + * @param string $public_id The public ID. + * + * @return void + */ + protected function link_attachment_to_public_id( $attachment_id, $public_id ) { + update_post_meta( $attachment_id, '_' . md5( $public_id ), true ); + } + /** * An existing asset whose byte size matches the local file is treated as this attachment's * own orphaned upload, so it's safe to overwrite. No etag in the result falls back to the @@ -73,7 +87,10 @@ protected function get_upload_sync() { * @return void */ public function test_matches_when_existing_asset_bytes_equal_the_local_file() { - $result = array( 'bytes' => self::$attachment_bytes ); + $result = array( + 'bytes' => self::$attachment_bytes, + 'public_id' => 'canola', + ); $this->assertTrue( $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) @@ -87,7 +104,10 @@ public function test_matches_when_existing_asset_bytes_equal_the_local_file() { * @return void */ public function test_does_not_match_when_existing_asset_bytes_differ() { - $result = array( 'bytes' => self::$attachment_bytes + 1 ); + $result = array( + 'bytes' => self::$attachment_bytes + 1, + 'public_id' => 'canola', + ); $this->assertFalse( $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) @@ -106,6 +126,20 @@ public function test_does_not_match_when_result_has_no_bytes_field() { ); } + /** + * Without a `public_id` field, there's no way to check who else might already be linked to + * it, so it must not be overwritten either. + * + * @return void + */ + public function test_does_not_match_when_result_has_no_public_id_field() { + $result = array( 'bytes' => self::$attachment_bytes ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + /** * Without a local file to compare against, there's no basis for a match either. * @@ -114,7 +148,10 @@ public function test_does_not_match_when_result_has_no_bytes_field() { public function test_does_not_match_when_the_attachment_has_no_local_file() { $post_id = self::factory()->post->create( array( 'post_type' => 'attachment' ) ); - $result = array( 'bytes' => self::$attachment_bytes ); + $result = array( + 'bytes' => self::$attachment_bytes, + 'public_id' => 'canola', + ); $this->assertFalse( $this->get_upload_sync()->is_matching_existing_asset( $post_id, $result ) @@ -129,8 +166,9 @@ public function test_does_not_match_when_the_attachment_has_no_local_file() { */ public function test_matches_when_bytes_and_etag_both_match() { $result = array( - 'bytes' => self::$attachment_bytes, - 'etag' => md5_file( get_attached_file( self::$attachment_id ) ), + 'bytes' => self::$attachment_bytes, + 'etag' => md5_file( get_attached_file( self::$attachment_id ) ), + 'public_id' => 'canola', ); $this->assertTrue( @@ -146,8 +184,9 @@ public function test_matches_when_bytes_and_etag_both_match() { */ public function test_does_not_match_when_bytes_match_but_etag_differs() { $result = array( - 'bytes' => self::$attachment_bytes, - 'etag' => 'not-the-real-hash', + 'bytes' => self::$attachment_bytes, + 'etag' => 'not-the-real-hash', + 'public_id' => 'canola', ); $this->assertFalse( @@ -155,6 +194,48 @@ public function test_does_not_match_when_bytes_match_but_etag_differs() { ); } + /** + * Byte-identical content is not proof of ownership: if another attachment is already + * tracked as linked to this public ID, overwriting it would clobber that attachment's + * context and advance its version out from under it, even though the bytes line up. + * + * @return void + */ + public function test_does_not_match_when_another_attachment_already_owns_the_public_id() { + $other_id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' ); + $this->link_attachment_to_public_id( $other_id, 'shared-id' ); + + $result = array( + 'bytes' => self::$attachment_bytes, + 'etag' => md5_file( get_attached_file( self::$attachment_id ) ), + 'public_id' => 'shared-id', + ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * This attachment being the one already tracked as linked to the public ID is the PR #1182 + * scenario itself (a prior successful upload whose local public_id record was then lost) -- + * still safe to overwrite. + * + * @return void + */ + public function test_matches_when_this_attachment_is_the_only_one_linked_to_the_public_id() { + $this->link_attachment_to_public_id( self::$attachment_id, 'canola' ); + + $result = array( + 'bytes' => self::$attachment_bytes, + 'public_id' => 'canola', + ); + + $this->assertTrue( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + /** * Cloudinary uploads the unscaled original for a "-scaled" image (the file WordPress * attaches for images over big_image_size_threshold is a downsized copy, not what was @@ -185,11 +266,93 @@ public function test_matches_using_the_unscaled_original_for_a_scaled_image() { // Cloudinary was sent the original -- its bytes must be what's compared against. $this->assertTrue( - $this->get_upload_sync()->is_matching_existing_asset( $id, array( 'bytes' => $original_bytes ) ) + $this->get_upload_sync()->is_matching_existing_asset( $id, array( 'bytes' => $original_bytes, 'public_id' => 'canola-original' ) ) ); // The attached (scaled) file's size is not what was actually uploaded. $this->assertFalse( - $this->get_upload_sync()->is_matching_existing_asset( $id, array( 'bytes' => $scaled_bytes ) ) + $this->get_upload_sync()->is_matching_existing_asset( $id, array( 'bytes' => $scaled_bytes, 'public_id' => 'canola-original' ) ) ); } + + /** + * A vip:// stream wrapper path is resolved without hashing it: doing so would pull the + * whole object over the network, and a failed read (false from md5_file()) would wrongly + * read as a content mismatch. Byte size alone is what's checked there. + * + * @return void + */ + public function test_matches_on_a_vip_path_by_bytes_alone_even_with_a_wrong_etag() { + add_filter( 'cloudinary_use_original_image', '__return_false' ); + add_filter( 'get_attached_file', array( $this, 'filter_attached_file_to_vip_path' ), 10, 2 ); + + try { + $result = array( + 'bytes' => self::$attachment_bytes, + 'etag' => 'not-the-real-hash', + 'public_id' => 'canola', + ); + + $this->assertTrue( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } finally { + remove_filter( 'get_attached_file', array( $this, 'filter_attached_file_to_vip_path' ), 10 ); + remove_filter( 'cloudinary_use_original_image', '__return_false' ); + } + } + + /** + * Rewrites an attached file path onto a fake vip:// stream wrapper, keeping filesize() + * resolvable (a plain file underneath) while making the path itself look VIP-hosted. + * + * @param string $file The attached file path. + * @param int $attachment_id The attachment ID. + * + * @return string + */ + public function filter_attached_file_to_vip_path( $file, $attachment_id ) { + if ( (int) $attachment_id !== (int) self::$attachment_id ) { + return $file; + } + if ( ! in_array( 'vip', stream_get_wrappers(), true ) ) { + stream_wrapper_register( 'vip', 'Test_Upload_Sync_Vip_Stream_Wrapper' ); + } + Test_Upload_Sync_Vip_Stream_Wrapper::$real_path = $file; + + return 'vip://canola.jpg'; + } +} + +/** + * A minimal stream wrapper standing in for VIP's, backed by a real local file. + * + * Only url_stat() is implemented: it's all is_matching_existing_asset() needs for + * file_exists()/filesize() to resolve. md5_file() is deliberately never exercised through this + * path in the test -- that's the whole point of the vip:// short-circuit being tested. + */ +class Test_Upload_Sync_Vip_Stream_Wrapper { + + /** + * The stream context resource, set automatically by PHP; must be declared or its creation is + * a deprecated dynamic property under PHPUnit's convertDeprecationsToExceptions. + * + * @var resource|null + */ + public $context; + + /** + * The real, local file path this wrapper reads from. + * + * @var string + */ + public static $real_path; + + /** + * Stat the underlying real file, so file_exists()/filesize() resolve. + * + * @return array|false + */ + public function url_stat() { + return @stat( self::$real_path ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged + } }