WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 10 months ago

Last modified 10 months ago

#51393 closed defect (bug) (invalid)

REST API: Inconsistent type for 'media_details' and 'sizes' in attachments controller

Reported by: SergeyBiryukov Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch
Focuses: Cc:

Description

Background: #50913, #50640.

While working on the tickets above, I've discovered an inconsistency in WP_REST_Attachments_Controller.

In the ::prepare_item_for_response() method, $data['media_details'] is treated as an array if not empty, but is initialized as an stdClass object if empty. Same for $data['media_details']['sizes'].

This was introduced along with the class itself in [38832].

This is currently causing two errors on PHP 8:

1) WP_Test_REST_Attachments_Controller::test_get_item_sizes
Error: Cannot use object of type stdClass as array

/var/www/tests/phpunit/tests/rest-api/rest-attachments-controller.php:607

2) WP_Test_REST_Attachments_Controller::test_get_item_sizes_with_no_url
Error: Cannot use object of type stdClass as array

/var/www/tests/phpunit/tests/rest-api/rest-attachments-controller.php:635

The errors are caused by $data['media_details']['sizes'] being initialized as an empty object, due to missing GD library in the PHP 8 build in the Docker image used for the tests. See comment:10:ticket:50640 for some more context.

The same errors can also be reproduced on PHP 7 or 5.6 if the GD library is not loaded.

The tests should probably be updated to check if $data['media_details']['sizes'] is not empty before proceeding, or to require the GD library, but the main issue remains.

Could we switch these properties to be initialized as an empty array instead of an object? See the attached patch.

Attachments (1)

51393.diff (2.2 KB) - added by SergeyBiryukov 10 months ago.

Download all attachments as: .zip

Change History (6)

#1 @SergeyBiryukov
10 months ago

  • Keywords php8 removed

Removing the keyword. Although this was discovered while working on PHP 8 support, it's not specific to PHP 8 after all, and can be reproduced on any PHP version if the GD library is not loaded.

#2 follow-up: @TimothyBlynJacobs
10 months ago

I _think_ this was implemented this way so that when the result is json_encoded it is encoded as an empty json object, instead of a json array. Ie {} instead of [].

Regardless, the correct schema type is an object. See https://developer.wordpress.org/rest-api/extending-the-rest-api/schema/#primitive-types for reference.

The tests should probably be updated to check if $data['media_details']['sizes'] is not empty before proceeding

I think this is the correct fix. Changing the data values in the controller would be a BC break.

Last edited 10 months ago by SergeyBiryukov (previous) (diff)

#3 in reply to: ↑ 2 @SergeyBiryukov
10 months ago

  • Milestone 5.6 deleted
  • Resolution set to invalid
  • Status changed from new to closed

Thanks for the clarification! This makes more sense to me now :)

Let's just fix the tests as part of #50913 or #50640 then, I agree with that being the correct fix here.

Last edited 10 months ago by SergeyBiryukov (previous) (diff)

#4 @SergeyBiryukov
10 months ago

In 49044:

Tests: Check if image sizes were successfully retrieved in some REST API attachments controller tests.

This outputs a proper message in case of failure, instead of an obscure PHP error further in the test.

Props TimothyBlynJacobs.
See #50913, #51393.

#5 @SergeyBiryukov
10 months ago

In 49046:

Tests: Correct the check for image sizes in some REST API attachments controller tests.

If the sizes data could not be retrieved, the controller returns an empty object instead of an array.

This makes sure that the value is in fact an array before proceeding, and outputs a proper message in case of failure, instead of an obscure PHP error further in the test.

Follow-up to [49044].

See #50913, #51393.

Note: See TracTickets for help on using tickets.