wp-media / imagify-plugin

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

Fixes #833 Delete all next-gen versions on restore original image #835

Closed remyperona closed 8 months ago

remyperona commented 8 months ago

Description

Correctly delete all next-gen versions on restore original image by passing the second parameter of the methods to true

Fixes #833

Type of change

Checklists

Feature validation

Code style

Mai-Saad commented 8 months ago

related test plan https://wpmediaqa.testrail.io/index.php?/runs/view/825&group_by=cases:section_id&group_order=asc

jeawhanlee commented 8 months ago

Testing in progress...

jeawhanlee commented 8 months ago

Thank you for the PR @Tabrisrp. During execution of test plan, with webp versions available and avif enabled, whenreoptimize with lossless compression is clicked the webp versions are deleted and avif created, While on trunk webp versions is untouched, Do we need to fix this @piotrbak ?

piotrbak commented 8 months ago

Yes, that sounds like a regression. @Tabrisrp the lossless/smart compression is related to the image optimization only, not the generation of webp/avif, right?

remyperona commented 8 months ago

The lossless level also applies to the next-gen versions, so it makes sense to delete and re-create them too

piotrbak commented 8 months ago

@Tabrisrp Looks like @Mai-Saad and @jeawhanlee were correct here. Currently in 2.2 we're not touching the version that's not enabled.

Is this a big effort to redo this change?

remyperona commented 8 months ago

It would mean to go back to the previous state, so the issue this PR is fixing would not be fixed.

Re-optimizing process is first restore the original, then optimize again with new settings, so it uses the same underlying restore methods.

MathieuLamiot commented 8 months ago

@piotrbak how can we align on the expected behavior to move forward? Should we set up a quick call this afternoon or not needed?

piotrbak commented 8 months ago

@MathieuLamiot I think we're okay with what @Tabrisrp explained.

@Mai-Saad We'll need to update the TCs