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

PHP 8.3 Compatibility #194

Closed
1 task done
drazenbebic opened this issue Apr 19, 2024 · 4 comments · Fixed by #203
Closed
1 task done

PHP 8.3 Compatibility #194

drazenbebic opened this issue Apr 19, 2024 · 4 comments · Fixed by #203
Assignees
Labels
type:bug Something isn't working.
Milestone

Comments

@drazenbebic
Copy link

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

  • I agree to follow this project's Code of Conduct
@drazenbebic drazenbebic added the type:bug Something isn't working. label Apr 19, 2024
@jeffpaul jeffpaul added this to the Future Release milestone Apr 19, 2024
@jeffpaul jeffpaul moved this from Incoming to To Do in Open Source Practice Apr 19, 2024
@dkotter dkotter modified the milestones: Future Release, 2.3.0 Apr 19, 2024
@kirtangajjar
Copy link
Member

@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
Copy link
Member

@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:

$image_meta['sizes'] = array();

@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
Copy link
Author

@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 faisal-alvi moved this from To Do to Code Review in Open Source Practice Jun 14, 2024
@faisal-alvi faisal-alvi self-assigned this Jun 14, 2024
@faisal-alvi
Copy link
Member

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

@github-project-automation github-project-automation bot moved this from Code Review to Done in Open Source Practice Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants