search

wp-media / imagify-plugin

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

Fatal error when delete webP image #815

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 delete webP image, we will have fatal error `[26-Feb-2024 08:19:43 UTC] PHP Fatal error: Uncaught TypeError: Imagify\Optimization\Process\AbstractProcess::delete_file(): Argument #1 ($next_gen_path) must be of type string, bool given, called in /home/mai/Local Sites/imagifynginx/app/public/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php on line 1451 and defined in /home/mai/Local Sites/imagifynginx/app/public/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php:1461 Stack trace:

0 /home/mai/Local Sites/imagifynginx/app/public/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php(1451): Imagify\Optimization\Process\AbstractProcess->delete_file(false)

1 /home/mai/Local Sites/imagifynginx/app/public/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php(1411): Imagify\Optimization\Process\AbstractProcess->delete_nextgen_file('/home/mai/Local...')

2 /home/mai/Local Sites/imagifynginx/app/public/wp-content/plugins/imagify-plugin/inc/common/attachments.php(41): Imagify\Optimization\Process\AbstractProcess->delete_nextgen_files()

3 /home/mai/Local Sites/imagifynginx/app/public/wp-includes/class-wp-hook.php(324): imagify_cleanup_after_media_deletion(Object(Imagify\Optimization\Process\WP))

4 /home/mai/Local Sites/imagifynginx/app/public/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters('', Array)

5 /home/mai/Local Sites/imagifynginx/app/public/wp-includes/plugin.php(517): WP_Hook->do_action(Array)

6 /home/mai/Local Sites/imagifynginx/app/public/wp-content/plugins/imagify-plugin/inc/functions/media.php(21): do_action('imagifydelete...', Object(Imagify\Optimization\Process\WP))

7 /home/mai/Local Sites/imagifynginx/app/public/wp-content/plugins/imagify-plugin/inc/common/attachments.php(20): imagify_trigger_delete_media_hook(Object(Imagify\Optimization\Process\WP))

8 /home/mai/Local Sites/imagifynginx/app/public/wp-includes/class-wp-hook.php(326): imagify_trigger_delete_attachment_hook(47)

9 /home/mai/Local Sites/imagifynginx/app/public/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array)

10 /home/mai/Local Sites/imagifynginx/app/public/wp-includes/plugin.php(517): WP_Hook->do_action(Array)

11 /home/mai/Local Sites/imagifynginx/app/public/wp-includes/post.php(6302): do_action('delete_attachme...', 47, Object(WP_Post))

12 /home/mai/Local Sites/imagifynginx/app/public/wp-admin/post.php(321): wp_delete_attachment(47, true)

13 {main}

thrown in /home/mai/Local Sites/imagifynginx/app/public/wp-content/plugins/imagify-plugin/classes/Optimization/Process/AbstractProcess.php on line 1461`

To Reproduce Steps to reproduce the behavior:

  1. Fresh install/activate of imagify avif branch
  2. upload webp image
  3. delete uploaded image
  4. See error

Expected behavior No error when delete webp image while imagify is active

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

Khadreal commented 3 months ago

The fatal error is caused by this line, we are now allowing webp or avif to be deleted directly, looks like we aren't factoring user uploading/deleting a webp/avif file into consideration here. Or maybe I'm missing something else here ?

cc @Miraeld

https://github.com/wp-media/imagify-plugin/blob/0103815adb503cc7ff0cc11cd195516bb6bee658/classes/Optimization/File.php#L786-L788

Miraeld commented 3 months ago

Umm, This function is based on an old one that was used by WebP: https://github.com/wp-media/imagify-plugin/blob/431a4ff2ca8d9668a840386c34365854726ec0b4/classes/Optimization/File.php#L756-L766

The thing is, who managed the deleting part in the previous feedback ? It might be easier for them to take a look at it, @wordpressfan @jeawhanlee or @Tabrisrp.

I've also noticed something that doesn't make sense in the code: https://github.com/wp-media/imagify-plugin/blob/4502844734755943c7f2d1444d77f51e5722e96a/classes/Optimization/Process/AbstractProcess.php#L1461-L1464

In here, we say that $next_gen_path will be a string, and right at the beginning of the function we check if it's false and in this case we return an WP_Error. Well, it will never go in that condition... But well, that's another subject.