search

wp-media / imagify-plugin

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

Generate missing next-gen keeps loading in settings page without generating webP when disable avif #838

Closed Mai-Saad closed 3 months ago

Mai-Saad commented 3 months ago

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

Describe the bug When we disable avif, generate missing next-gen is automatically loading in settings page while nothing no webP created (and it can be -ve )

To Reproduce Steps to reproduce the behavior:

  1. Fresh install and activation of imagify on clean new site
  2. enable avif
  3. upload couple of images => avif is created
  4. disable avif => generate missing next-gen is loading with -2/0 and will keep loading

Expected behavior Either generate next-gen not automatically triggered in this case or webp is created. Please refer to @wp-media/productimagify to confirm expected

Screenshots screen-capture (87).webm

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

Miraeld commented 3 months ago

Reproduce the problem

To reproduce the issue, follow the step in the issue.

Identify the root cause

The issue is that, while saving the settings with AVIF disabled from an enabled state, we are bailing out of this condition https://github.com/wp-media/imagify-plugin/blob/5684433e2a7d126b312de47ed05d21022483374c/classes/Bulk/Bulk.php#L612-L615

As we bail-out, we don't set this transient https://github.com/wp-media/imagify-plugin/blob/5684433e2a7d126b312de47ed05d21022483374c/classes/Bulk/Bulk.php#L298 which leads to a negative number in the count => This is a reason we see a -2/0 if we have two images.

Scope a solution

To solve the issue, we could delete https://github.com/wp-media/imagify-plugin/blob/5684433e2a7d126b312de47ed05d21022483374c/classes/Bulk/Bulk.php#L612-L615 so either avif or webp would be generated depending on what we enable and save. As it would run through https://github.com/wp-media/imagify-plugin/blob/5684433e2a7d126b312de47ed05d21022483374c/classes/Bulk/Bulk.php#L237 we would set the transient and the count would be good.

Effort estimation:

XS

Is a refactor needed in that part of the codebase?

No

Tabrisrp commented 3 months ago

I think this is missing expectations:

the 1st activation of imagify while having images without webP, won't generate webP by default for those images. (we shall test this case as well with the fix here or may be add new GH)

On first activation there is nothing to do I believe? The bulk optimization is there for it.

@piotrbak can you review the above points?

piotrbak commented 3 months ago

@Tabrisrp thanks for additional cases.

When disabling convert to AVIF, should the currently process be stopped? The process of bulk optimization that was started before should not stop, it should move forward

Should we convert missing to WebP when disabling convert to AVIF? I'm not sure if I fully get the use case here. In general, we should generate webp for all images when disabling the AVIF generation.

However, I can imagine that you could've meant the situation when the bulk process was running and the AVIF was disabled in the middle. Then we could just finish generating webp for images that were still not processed. Then we'd have an UI button to generate missing ones, right?

Tabrisrp commented 3 months ago

We can't generate WebP (or AVIF) images for images that have not been optimized yet, it either happens:

So on first install, you have to either optimize images individually, or through the bulk optimization, to generate the next-gen images too.

Miraeld commented 3 months ago

With this grooming, it would generates WebP when you disabled AVIF. So it covers In general, we should generate webp for all images when disabling the AVIF generation.

piotrbak commented 3 months ago

It's expected behaviour then. What do you think @Tabrisrp ?

Tabrisrp commented 3 months ago

It covers this case yes. It won't generate anything on first install, but for me it's not possible to do it.

piotrbak commented 3 months ago

That sounds good to me then 🆗

Mai-Saad commented 3 months ago

It covers this case yes. It won't generate anything on first install, but for me it's not possible to do it.

Here is the case (however , we can agree that it's edge case) 1- activate 2.2+ 2- enable avif 3- upload image => image is optimized and avif created 4- deactivate/delete imagify 5- install and activate imagify => webp for that image isnot automatically generated , however there is generate link displayed so I think it's ok to keep it like that

piotrbak commented 3 months ago

That's fine @Mai-Saad