#50833 closed defect (bug) (fixed)
[PHP 8] GD resources are GdImage object instances
Reported by: | ayeshrajans | Owned by: | 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.
- Update
is_resource()
calls to accomodateGdImage
instances as well (https://php.watch/versions/8.0/gdimage#gdimage-is-resource) - Update PHPDoc comments to allow
resource|GdImage
in their type hints.
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.
Attachments (2)
Change History (15)
#1
@
4 years ago
- Component changed from General to Media
- Milestone changed from Awaiting Review to 5.6
#3
@
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:
- The new
is_gd_image()
function insrc/wp-admin/includes/image.php
checks againstis_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 begd
.
- 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.
- 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 iftrue
would, for instance, mean it is a resource andfalse
a GDImage object.Returns whether the provided argument is of a resource, or a PHP 8 GdImage object.
@return bool
- Nitpick: the parameter description alignment in the docblock of the
save()
function insrc/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 theis_gd_image()
function is missing and AFAIK@ticket
tags should only be used in the tests, not in the docblocks insrc
. Oh and the@since
tag should have a complete version number, i.e.5.6.0
.
- 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.
#5
@
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).
#7
follow-up:
↓ 11
@
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.
#9
follow-up:
↓ 10
@
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.
#10
in reply to:
↑ 9
@
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 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!
#11
in reply to:
↑ 7
@
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
#13
@
4 years ago
- Keywords has-dev-note added; needs-dev-note removed
Dev note published: https://make.wordpress.org/core/2020/11/23/wordpress-and-php-8-0/
Tests: https://travis-ci.com/github/Ayesh/wordpress-develop/builds/178167517