search

wp-media / imagify-plugin

Speed up your website with lighter images without losing quality.
https://imagify.io
69 stars 24 forks source link

Reoptimize image in media library isnot working #790

Closed Mai-Saad closed 3 months ago

Mai-Saad commented 4 months ago

Before submitting an issue please check that you’ve completed the following steps:

Describe the bug When clicking reoptimize with lossless or smart compression, image+thumbnails are removed from the upload folder and not regenerated causing a warning [02-Feb-2024 06:48:10 UTC] PHP Warning: file_get_contents(/var/www/imagify.rocketlabsqa.ovh/htdocs/wp-content/uploads/2024/02/sea2.jpg): Failed to open stream: No such file or directory in /var/www/imagify.rocketlabsqa.ovh/htdocs/wp-includes/functions.php on line 3332 and status in media library is 'This media lacks the required metadata and cannot be optimized.'

To Reproduce Steps to reproduce the behavior:

  1. fresh activation of imagify
  2. upload image
  3. in the media library click 'reoptimize with lossless compression'

Expected behavior

Screenshots If applicable, add screenshots to help explain your problem.

Additional context Add any other context about the problem here.

Acceptance Criteria (for WP Media team use only) Clear instructions for developers, to be added before the grooming

wordpressfan commented 4 months ago

When we do the reoptimize process, we try to remove the next gen media files:

Start here: https://github.com/wp-media/imagify-plugin/blob/de6dc98d592a607b52baa89918d3d3c6b7ee8460/classes/Optimization/Process/AbstractProcess.php#L327

Then here:

https://github.com/wp-media/imagify-plugin/blob/de6dc98d592a607b52baa89918d3d3c6b7ee8460/classes/Optimization/Process/AbstractProcess.php#L1023

Then here:

https://github.com/wp-media/imagify-plugin/blob/de6dc98d592a607b52baa89918d3d3c6b7ee8460/classes/Optimization/Process/AbstractProcess.php#L1581

We try to get the next gen. file path but we pass the $this->format which in this case equals @imagify-avif OR @imagify-webp as in here:

https://github.com/wp-media/imagify-plugin/blob/de6dc98d592a607b52baa89918d3d3c6b7ee8460/classes/Optimization/Process/AbstractProcess.php#L1639

Then goes here:

https://github.com/wp-media/imagify-plugin/blob/de6dc98d592a607b52baa89918d3d3c6b7ee8460/classes/Optimization/File.php#L781-L791

calling the function imagify_path_to_next_gen with the previously mentioned values but we are expecting values like webp or avif

https://github.com/wp-media/imagify-plugin/blob/ec0bf151a9607b8a5e40daaf9c60a7d912700f2f/inc/functions/common.php#L201-L212

Which means that when reoptimize is triggered we are removing the main image file! not the next gen ones which is catastrophic!

What do u think here?

Miraeld commented 4 months ago

Ummml, what you are saying seems correct. Changing this function :

function imagify_path_to_next_gen( $path, string $format ) { 
    switch ( $format ) { 
        case 'webp': 
            $path = $path . '.webp'; 
            break; 
        case 'avif': 
            $path = $path . '.avif'; 
            break; 
    } 

    return $path; 
 }

Would probably fix one issue.

Tabrisrp commented 3 months ago

Good find, now that we have the root cause, let's find the best solution to fix it

wordpressfan commented 3 months ago

Changing the format inside switch cases in imagify_path_to_next_gen would be a solution yes, but will require small changes as this function is called in two places over the code, I'm OK with that solution.

Or we can add another case with the constants WEBP_SUFFIX and AVIF_SUFFIX beside webp and avif

Tabrisrp commented 3 months ago

I think the value passed should be fixed in the AbstractProcess class instead

Mai-Saad commented 3 months ago

No warning now and Files updated in upload folder. Note: while AVIF is off, now we arenot creating webP while if AVIF was on we create AVIF with reoptimize (will open separate GH about that if it was still there after merging https://github.com/wp-media/imagify-plugin/pull/809

Mai-Saad commented 3 months ago

Note: while AVIF is off, now we arenot creating webP while if AVIF was on we create AVIF with reoptimize

reoptimize_lossless while AVIF on reoptimize_lossless

reoptimize_lossless while AVIF off reoptimize_lossless_webp

reoptimize_smart while AVIF on reoptimize_smart

Mai-Saad commented 3 months ago