Avoid overwriting existing WebP files when creating WebP images

[mukeshpanchal27]

Summary

This PR skips the creation of duplicate WebP images if anyone uses an image file with the same name with an extension of jpg, jpe, or jpeg.

Please check more detail about the issue and the options for the solution here

As @felixarntz suggested we should go with 1st approach.

If you want to generate a WebP image for .png images then please add the below code snippet in your current themes functions.php

add_filter('webp_uploads_upload_image_mime_transforms', function () {

    return [
        'image/jpeg' =>  [ 'image/jpeg', 'image/webp' ],
        'image/png' =>  [ 'image/png', 'image/webp' ],
        'image/webp' =>  [ 'image/webp', 'image/jpeg' ],
    ];
});

Fixes #358

Images for testing

If you want to test the different images then please download the zip in which I added all image formats( jpg, jpe, jpeg and png ) - image.zip

Checklist

• PR has either [Focus] or Infrastructure label.
• PR has a [Type] label.
• PR has a milestone or the no milestone label.

[jjgrainger]

Looks goods

[mitogh]

Just one question, besides that things look good on my end.

I would encourage to add a test case replicating the current issue BTW.

[mitogh]

Do we want to try to create a different destination name instead? Like appending a number or a UUID instead, in the same way, WordPress handles it when the file already exists?

[felixarntz]

@mitogh Yes, this is the eventual plan, but for now adding this stop-gap to not overwrite/confuse images should be sufficient.

Let's work on a proper solution where both images can be kept in a separate PR.

[felixarntz]

@mukeshpanchal27 This PR only fixes one half of the bug - for when the full size is generated, but not for when the other sizes are generated. See my comment below, I think this should rather be fixed at the root of the problem, in webp_uploads_generate_additional_image_source(). Otherwise, it's easy to miss certain situations, one of them already missing here.

Could you then also add a test for the new condition in webp_uploads_generate_additional_image_source(), to cover the new clause that an existing image file would cause a WP_Error?

[felixarntz]

Instead of adding this check here, I think a cleaner solution would be to handle it within the webp_uploads_generate_additional_image_source() function which is the function where the file is actually generated. That would fix the problem at its root and it would ensure you don't have to add a condition around it everywhere you call that function.

The current implementation here is actually missing the scenario when a new image size is generated where that function is also called - right now there would still be a chance for the image to be overwritten. By adding the condition to webp_uploads_generate_additional_image_source() and returning a WP_Error in that case, we only need to make that change there and it will work everywhere without any further changes.

[mukeshpanchal27]

If we consider checking the file_exists in function webp_uploads_generate_additional_image_source() then we can check it after the editor generate_filename function because we will get all additional thumbnail size URLs after this code snippet.

if ( null === $destination_file_name ) {
	$extension             = explode( '|', $allowed_mimes[ $mime ] );
	$destination_file_name = $editor->generate_filename( null, null, $extension[0] );
}

As I troubleshoot the issue, we have two options here.

1. We can check file_exists after the code snippet so we get full and other thumbnail images.

    if ( null === $destination_file_name ) {
    	$extension             = explode( '|', $allowed_mimes[ $mime ] );
    	$destination_file_name = $editor->generate_filename( null, null, $extension[0] );
    }
   
    if ( file_exists( $destination_file_name ) ) {
    	return new WP_Error( 'webp_image_file_exist', __( 'The webP image already exists.', 'performance-lab' ) );
    }
   

In this approach there may be some unnecessary checking happens for the duplicate image in function webp_uploads_generate_additional_image_source()

2. We pass $destination_file_name in function webp_uploads_generate_image_size() for consistancy for thumbnail images so we can check file_exists at top in function webp_uploads_generate_additional_image_source()

    $filename    = pathinfo( $properties['file'], PATHINFO_FILENAME );
    $extension   = explode( '|', $allowed_mimes[ $targeted_mime ] );
    $destination = trailingslashit( $original_directory ) . "{$filename}.{$extension[0]}";
    $source = webp_uploads_generate_image_size( $attachment_id, $size_name, $mime, $destination );
   

If we consider this option then we don't need to call the generate_filename function to get the image name.

if ( null === $destination_file_name ) {
	$extension             = explode( '|', $allowed_mimes[ $mime ] );
	$destination_file_name = $editor->generate_filename( null, null, $extension[0] );
}

In this approach we have to update unit test, we use webp_uploads_generate_image_size() function in unit test.

If I miss anything then please let me know.

[felixarntz]

@mukeshpanchal27 I think we should go with your approach 1, always run the file_exists check right after we have the $destination_file_name set for sure. That way we only need to have the check in one place.

In this approach there may be some unnecessary checking happens for the duplicate image in function webp_uploads_generate_additional_image_source()

Can you clarify what you mean? I think there would only be a single file_exists check in each different situation, only in webp_uploads_generate_additional_image_source().

[mukeshpanchal27]

Can you clarify what you mean?

For duplicate image, webp_uploads_generate_additional_image_source() function check this https://github.com/mukeshpanchal27/performance/blob/fix/358-skip-duplicate-webp-image/modules/images/webp-uploads/helper.php#L85-L143 code that may be not require for already exist images.

PR updated with approach 1 and requested the review.

[felixarntz]

@mukeshpanchal27

For duplicate image, webp_uploads_generate_additional_image_source() function check this https://github.com/mukeshpanchal27/performance/blob/fix/358-skip-duplicate-webp-image/modules/images/webp-uploads/helper.php#L85-L143 code that may be not require for already exist images.

Right, I agree it's a bit wasteful that we have to run all that logic before the file_exists check, but unless $destination_file_name is already provided, some of that logic is critical to run before since it affects the file name (for example the resize() call).

I think the current implementation is good for now since it solves the problem. In WordPress core this will eventually be implemented differently anyway. I left one more tiny comment, but already approving this since it's good to go.