Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fatal error when delete webP image #815

Closed
Mai-Saad opened this issue Feb 26, 2024 · 2 comments · Fixed by #819
Closed

Fatal error when delete webP image #815

Mai-Saad opened this issue Feb 26, 2024 · 2 comments · Fixed by #819
Assignees
Labels
AVIF Avif branch/feature priority: medium Issues which are important, but no one will go out of business. severity: critical type: bug
Milestone

Comments

@Mai-Saad
Copy link

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

  • Made sure you’re on the latest version => commit 0103815
  • Used the search feature to ensure that the bug hasn’t been reported before

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('imagify_delete_...', 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

  • No error when delete any type of image from media library (including large/small image, webp/avif/png/jpg..etc)
@Mai-Saad Mai-Saad added type: bug severity: critical priority: medium Issues which are important, but no one will go out of business. AVIF Avif branch/feature labels Feb 26, 2024
@Khadreal Khadreal self-assigned this Feb 26, 2024
@piotrbak piotrbak added this to the 2.2 milestone Feb 26, 2024
@Khadreal
Copy link
Contributor

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

if ( $this->is_webp() || $this->is_avif() ) {
return false;
}

@Miraeld
Copy link
Contributor

Miraeld commented Feb 26, 2024

Umm,
This function is based on an old one that was used by WebP:

public function get_path_to_webp() {
if ( ! $this->is_image() ) {
return false;
}
if ( $this->is_webp() ) {
return false;
}
return imagify_path_to_webp( $this->path );
}

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:

protected function delete_file( string $next_gen_path ) {
if ( ! $next_gen_path ) {
return new WP_Error( 'no_$next_gen_path', __( 'Could not get the path to the Next-Gen format file.', 'imagify' ) );
}

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AVIF Avif branch/feature priority: medium Issues which are important, but no one will go out of business. severity: critical type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants