search

10up / safe-svg

Enable SVG uploads and sanitize them to stop XML/SVG vulnerabilities in your WordPress website.
https://wordpress.org/plugins/safe-svg/
GNU General Public License v2.0
263 stars 31 forks source link

PHP 8.3 Compatibility #194

Closed drazenbebic closed 2 months ago

drazenbebic commented 4 months ago

Describe the bug

There is a small PHP warning coming from your plugin when upgrading to PHP 8.3:

Deprecated: Automatic conversion of false to array is deprecated in /wp-content/plugins/safe-svg/safe-svg.php on line 652

Steps to Reproduce

  1. Update PHP to 8.3
  2. Have the Safe SVG plugin installed & activated
  3. Visit any page in the frontend
  4. Check PHP logs

Screenshots, screen recording, code snippet

No response

Environment information

No response

WordPress information

No response

Code of Conduct

kirtangajjar commented 3 months ago

@faisal-alvi @dkotter I wasn't able to reproduce this on latest trunk or even on 2.2.4 with the given instructions. I upgraded to PHP 8.3 and visited page with no SVG, page with SVG and even editor with with and without SVG. I wasn't able to see that warning in the logs.

Can you folks confirm?

faisal-alvi commented 2 months ago

@kirtangajjar I tested and did not encounter the PHP warning either. I reviewed the code on line 652 as mentioned, and it doesn't seem to be the source of the issue. It appears the code has either been updated or maybe an older version was used during testing. However, I think the following line of. code could be the source for the warning:

https://github.com/10up/safe-svg/blob/ec4f598a109ff85660f318b175a8a952c18f9cbb/safe-svg.php#L543

@drazenbebic could you please share the plugin version used during testing? Alternatively, sharing the code around the line 652 of your safe-svg.php would also be helpful.

drazenbebic commented 2 months ago

@faisal-alvi I just checked, we're using the latest version (2.2.4). I even tried completely deleting the plugin and installing it again from the plugin store, but I still get the same warning.

I must add: The warning is now coming from line 691, not 652 anymore. Like you said, it's the line you mentioned.

Here's the code around line 691 ($image_meta['sizes'] = array(); being line 691):

public function disable_srcset( $image_meta, $size_array, $image_src, $attachment_id ) {
    if ( $attachment_id && 'image/svg+xml' === get_post_mime_type( $attachment_id ) ) {
        $image_meta['sizes'] = array();
    }

    return $image_meta;
}

I was debugging the issue a little bit and found out that sometimes the $image_meta parameter of the wp_get_attachment_metadata hook can be false, even though this is not documented in the Developer Documentation.

This happens because wp_get_attachment_image_srcset() internally calls wp_get_attachment_metadata() if no $metadata parameter is passed (which happens in our case). This function can return either false or array (this is also documented in the developer docs), and for whatever reason, it fails and returns false.

Then this false value is passed to the wp_calculate_image_srcset() function which expects the image meta to be an array, not a boolean value. This value is then passed to the wp_calculate_image_srcset_meta filter, which your plugin hooks into.

TLDR; $image_meta sometimes is false and not an array, which is why the warning is thrown.

This sounds more like a WordPress core bug than an issue with your plugin. I already found an alternative way for us to handle it. Not sure if you want to handle this on your side too?

faisal-alvi commented 2 months ago

@drazenbebic thank you for the details. I've raised PR #203, please confirm if it removes the warning.