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 dd2d9100..dfe5d6b3 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,43 @@ 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 + * 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. + * + * @return bool + */ + 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 = $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; + } + + // Bytes alone can coincide between unrelated files; confirm with the content hash when available. + return empty( $result['etag'] ) || md5_file( $file ) === $result['etag']; + } + /** * 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..8d43bf8d --- /dev/null +++ b/tests/phpunit/tests/test-upload-sync.php @@ -0,0 +1,195 @@ +attachment->create_upload_object( $file ); + self::$attachment_bytes = filesize( get_attached_file( self::$attachment_id ) ); + } + + /** + * Build a fully wired Upload_Sync instance. + * + * 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() { + $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. No etag in the result falls back to the + * byte comparison alone. + * + * @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 ) + ); + } + + /** + * 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 ) ) + ); + } +}