WordPress.org

Make WordPress Core

Opened 5 weeks ago

Closed 6 days ago

Last modified 6 days ago

#51389 closed defect (bug) (fixed)

REST API: WP_REST_Meta_Fields::default_additional_properties_to_false assumes that the schema has properties.

Reported by: austin880625 Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.9
Component: REST API Keywords: good-first-bug needs-patch
Focuses: Cc:

Description

This function simply traverses through $schema object and doesn't intend to do any type check on schema. Hence it may need to check existence of properties before traverse into it.

It affect the REST API response for post that has meta with object type, the warning message PHP Warning: Invalid argument supplied for foreach() in ... will corrupt the json output if the error_reporting configuration is low enough.

The problem can be reproduced with the following meta registration in a plugin:

<?php
function test_register_post_meta() {
    register_post_meta('post', 'test_key', array(
        'type' => 'object',
        'show_in_rest' => true,
    )); 
}
add_action('init', 'test_register_post_meta');
?>

#48785 might be related. They add test for that case, but I'm not sure what behavior should I test over in my case(for eliminating an warning without affecting the final result).

Attachments (3)

51389.patch (905 bytes) - added by austin880625 5 weeks ago.
51389.2.patch (1.8 KB) - added by austin880625 7 days ago.
51389.3.patch (1.2 KB) - added by austin880625 6 days ago.

Download all attachments as: .zip

Change History (15)

@austin880625
5 weeks ago

#1 @TimothyBlynJacobs
4 weeks ago

Thanks for the patch @austin880625 and welcome to trac!

We could take a look at switching out that method to use the new rest_default_additional_properties_to_false introduced in WordPress 5.5.

#2 @TimothyBlynJacobs
4 weeks ago

  • Milestone changed from Awaiting Review to 5.6
  • Version changed from trunk to 4.9

#3 @TimothyBlynJacobs
7 days ago

  • Keywords good-first-bug needs-patch added; has-patch removed

@austin880625
7 days ago

#4 @austin880625
7 days ago

I changed the method used in get_registered_fields into rest_default_additional_properties_to_false , and since it seems to be the only method calling default_additional_properties_to_false , deleting the whole method also seems OK.

I have done npm run test:php once, the result remain the same after I made these changes. Remind me if I did anything wrong.

#5 @TimothyBlynJacobs
6 days ago

Thanks for the refreshed patch @austin880625! We still need to keep the method around since developers who are extending the class may be using the method. So instead of removing it, we should instead mark it as deprecated and use _deprecated_function.

For example: https://github.com/WordPress/wordpress-develop/blob/9eed4bc352c944ce75c3ff1615bc63b7162af88b/src/wp-includes/rest-api/search/class-wp-rest-post-search-handler.php#L191

@austin880625
6 days ago

#6 @austin880625
6 days ago

the method has been put back and the call to _deprecated_function is added. You may check if there's any other problem.

#7 @TimothyBlynJacobs
6 days ago

Thanks @austin880625! Can you open this as a Pull Request please, https://github.com/WordPress/wordpress-develop? It looks like there are some linting issues which a Pull Request will highlight.

This ticket was mentioned in PR #659 on WordPress/wordpress-develop by austin880625.


6 days ago

  • Keywords has-patch added; needs-patch removed

Signed-off-by: Austin Chang <austin880625@…>

Change the call to the default_additional_properties_to_false method into rest_default_addtional_properties_to_false introduced in 5.5

Trac ticket: https://core.trac.wordpress.org/ticket/51389

This ticket was mentioned in PR #660 on WordPress/wordpress-develop by austin880625.


6 days ago

Signed-off-by: Austin Chang <austin880625@…>

Change the call to the default_additional_properties_to_false method into rest_default_addtional_properties_to_false introduced in 5.5

Trac ticket: https://core.trac.wordpress.org/ticket/51389

#10 @austin880625
6 days ago

  • Keywords needs-patch added; has-patch removed

Ah, fixed the indentation issue.
Should I upload the patch that being displayed on the PR section again?

Last edited 6 days ago by austin880625 (previous) (diff)

#11 @TimothyBlynJacobs
6 days ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 49308:

REST API: Prevent PHP warning when metadata schema is missing properties.

This switches to the new rest_default_additional_properties_to_false() function which doesn't have this issue and deprecates the WP_REST_Meta_Fields::default_additional_properties_to_false() method.

Props austin880625.
Fixes #51389.

#12 @prbot
6 days ago

TimothyBJacobs commented on PR #660:

Merged in 3646dd2e9f8c80d924a6b6a691a5c1387e51c2c4. Thanks for the patch @austin880625!

Note: See TracTickets for help on using tickets.