Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50833 closed defect (bug) (fixed)

[PHP 8] GD resources are GdImage object instances

Reported by: ayeshrajans's profile ayeshrajans Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: php8 has-patch has-dev-note
Focuses: coding-standards Cc:

Description

In PHP 8, GD extension uses GdImage class objects instead of resources.

https://php.watch/versions/8.0/gdimage

This is a preemptive ticket aiming WordPress 5.6.

Without these changes, WordPress early-fails to process images because it is being defensive with multiple is_resource() checks which will no longer return true in PHP 8.0.

Change History (15)

#1 @SergeyBiryukov
4 years ago

  • Component changed from General to Media
  • Milestone changed from Awaiting Review to 5.6

#2 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#3 @jrf
4 years ago

@ayeshrajans Thanks for initiating this.

I've reviewed the attached patch and it mostly looks good & ready to commit.

Five remarks, though each very small:

  1. The new is_gd_image() function in src/wp-admin/includes/image.php checks against is_resource(), which is the same as was previously done inline, but I wonder if this check should be enhanced with an additional check against get_resource_type()`. Based on the PHP docs, the applicable resource type should be gd.
  1. Along the same lines as 1, IMO a test should be added passing a non-GdImage resource to the is_gd_image() function to document the behaviour of the function in that situation.
  1. The function description for the new is_gd_image() function needs work as it can be misinterpreted as is. Alternatively, a good description for the @return tag could take away the confusion. It currently can be interpreted as if true would, for instance, mean it is a resource and false a GDImage object.

    Returns whether the provided argument is of a resource, or a PHP 8 GdImage object.
    @return bool

  1. Nitpick: the parameter description alignment in the docblock of the save() function in src/wp-includes/class-wp-image-editor-gd.php is off. (4 spaces after longest param, should be 1 space). Also: the parameter description for the $image parameter of the is_gd_image() function is missing and AFAIK @ticket tags should only be used in the tests, not in the docblocks in src. Oh and the @since tag should have a complete version number, i.e. 5.6.0.
  1. Only two of the four testcases need for the GD extension to be loaded, but the whole test is now being skipped if GD is not available. Consider using a data provider for the test cases, where one of the passed parameters could be whether GD is needed or not. If left as is, consider using a @requires annotation instead of the condition in the test itself.

#4 @jrf
4 years ago

  • Focuses coding-standards added
  • Keywords has-patch added; needs-patch removed

#5 @ayeshrajans
4 years ago

Thanks a lot for this thorough review and detailed reply @jrf.

You are absolutely right the function description was not very good, so I rewrote the doc block to add more clarity and meaning, and to work on the points you mentioned in the comment above.

  • is_gd_image() now checks the type of the resource as well.
  • Added a new test to check for truthy values for is_gd_image(), and it @requires extendion gd.
  • There is an additional test with a fopen resource (closed after the test).

(Patch attached in next comment).

#6 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 48798:

Code Modernization: Introduce is_gd_image() to check for PHP 8 GdImage object instances.

In PHP 8, the GD extension uses GdImage objects instead of resources for its underlying data structures.

This updates the existing is_resource() calls for image resources in core to accomodate for GdImage instances as well.

Props ayeshrajans, jrf.
Fixes #50833.

#7 follow-up: @SergeyBiryukov
4 years ago

Since some media functions can technically be called on front end, I'm wondering if is_gd_image() should be available on front end too. Didn't want that consideration to block the initial commit though.

Not reopening the ticket for now, just wanted to note that this may need to be done in case it comes up later.

#8 @SergeyBiryukov
4 years ago

In 48799:

Docs: Fix typo in the $image parameter description in is_gd_image().

Follow-up to [48798].

See #50833.

#9 follow-up: @jrf
4 years ago

@SergeyBiryukov Thanks for committing this.

Since some media functions can technically be called on front end, I'm wondering if is_gd_image() should be available on front end too.

Good question and one I will freely admit I hadn't considered when reviewing.

Thinking it over now, I don't think it is necessary unless/until the page/post editing on the front-end would be implemented.

Just plain showing or moving images generally won't need this is_gd_image() function, as the GD extension isn't needed for that. It is only image creation and/or editing which needs GD and therefore will need this function.

Does that make sense ? Am I overlooking something ?

On another note - should we add the needs-dev-note tag to this ticket ? This new function should get a mention in the PHP 8 dev-note for WP 5.6.

Last edited 4 years ago by jrf (previous) (diff)

#10 in reply to: ↑ 9 @SergeyBiryukov
4 years ago

  • Keywords needs-dev-note added

Replying to jrf:

Thinking it over now, I don't think it is necessary unless/until the page/post editing on the front-end would be implemented.

Just plain showing or moving images generally won't need this is_gd_image() function, as the GD extension isn't needed for that. It is only image creation and/or editing which needs GD and therefore will need this function.

Thanks! I came to the same conclusion and chose not to reopen the ticket at this time.

The only relevant instance in core seems to be WP_REST_Attachments_Controller::edit_media_item(), but it already requires wp-admin/includes/image.php, where is_gd_image() is defined.

It's hard to tell if any plugins that implement front-end editing might need the function too, I guess we can wait and see if there are any reports during the dev cycle.

On another note - should we add the needs-dev-note tag to this ticket ? This new function should get a mention in the PHP 8 dev-note for WP 5.6.

Yes, we should :) Thanks!

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#11 in reply to: ↑ 7 @SergeyBiryukov
4 years ago

Replying to SergeyBiryukov:

Since some media functions can technically be called on front end, I'm wondering if is_gd_image() should be available on front end too.
...
It's hard to tell if any plugins that implement front-end editing might need the function too, I guess we can wait and see if there are any reports during the dev cycle.

Follow-up: #51174

#12 @SergeyBiryukov
4 years ago

In 48905:

Media: Make the is_gd_image() function available on front end.

This avoids a fatal error if a plugin calls image creation or editing functions like wp_imagecreatetruecolor() outside of the admin.

Follow-up to [48798]

Props BackuPs.
Fixes #51174. See #50833.

#13 @daisyo
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.