Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#48785 closed defect (bug) (fixed)

REST API: WP_REST_Controller::get_public_item_schema() assumes that the schema has properties.

Reported by: pento's profile pento Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: good-first-bug has-patch has-unit-tests commit
Focuses: Cc:

Description

My understanding of JSON Schema is that the base type doesn't necessarily need to be an object, it can be any type, so it's possible to define an endpoint that returns a string:

array(
        '$schema'     => 'http://json-schema.org/draft-04/schema#',
        'title'       => 'foo',
        'type'        => 'string',
        'description' => __( 'This is my magical endpoint that just returns a string.' ),
);

Other methods on WP_REST_Controller seem to check that properties is defined on the schema, but ::get_public_item_schema() doesn't.

Attachments (7)

#48785.patch (530 bytes) - added by dhavalkasvala 4 years ago.
#48785-improve.patch (534 bytes) - added by dhavalkasvala 4 years ago.
48785.diff (2.7 KB) - added by johnwatkins0 4 years ago.
Includes the previously attached change with unit tests covering the change as well as get_public_item_schema generally
48785.2.diff (3.1 KB) - added by johnwatkins0 4 years ago.
Made unit test PHP5-compatible
48785.3.diff (3.4 KB) - added by johnwatkins0 4 years ago.
48785.4.diff (3.7 KB) - added by johnwatkins0 4 years ago.
48785.5.diff (3.9 KB) - added by birgire 4 years ago.

Download all attachments as: .zip

Change History (29)

#1 @dkarfa
4 years ago

Hi @pento
Yes, ::get_item_schema() returing the properties

#2 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Awaiting Review to 5.4

Yep, this would be good to fix. I've run into the issue as well.

#3 @dhavalkasvala
4 years ago

  • Keywords has-patch added; needs-patch removed

#4 @TimothyBlynJacobs
4 years ago

Thanks for working on a patch for this @dhavalkasvala! Do you want to try tackling a unit test for this? Coding standards wise, we should also have spaces around the parentheses.

#5 @TimothyBlynJacobs
4 years ago

Thanks for updating the patch @dhavalkasvala, this looks good! Do you want to work on a unit test for this? No worries if not.

#6 @dhavalkasvala
4 years ago

@TimothyBlynJacobs I want to work on unit test but don't how to do that right now.

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

#7 @TimothyBlynJacobs
4 years ago

No worries @dhavalkasvala! There is some documentation available on WordPress.org: https://make.wordpress.org/core/handbook/testing/automated-testing/writing-phpunit-tests/

I sent you a message on Slack as well. Feel free to ping me about getting started with it.

This ticket was mentioned in Slack in #core-restapi by pbiron. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


4 years ago

@johnwatkins0
4 years ago

Includes the previously attached change with unit tests covering the change as well as get_public_item_schema generally

#10 @johnwatkins0
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@johnwatkins0
4 years ago

Made unit test PHP5-compatible

#11 @TimothyBlynJacobs
4 years ago

Thanks for working on a test for this @johnwatkins0!

I think it'd be better if we created a new WP_REST_Test_Controller that accepted in its constructor a schema. That way we'd be able to reuse the controller for any schema related controller tests.

Perhaps something along the lines of WP_REST_Test_Configurable_Controller?

@johnwatkins0
4 years ago

#12 @johnwatkins0
4 years ago

Thanks, @TimothyBlynJacobs. I like that Idea. Updated in 48785.3.diff.

#13 @TimothyBlynJacobs
4 years ago

That looks great @johnwatkins0! Let's just split that test into two. One for checking against the string type, and another for the object type with properties.

@johnwatkins0
4 years ago

#14 @johnwatkins0
4 years ago

Separate tests makes sense. Updated in 48785.4.diff, @TimothyBlynJacobs .

#15 @TimothyBlynJacobs
4 years ago

  • Keywords commit added

Thanks for refreshing @johnwatkins0, looks great!

#16 @birgire
4 years ago

The restapi group test on 48785.4.diff seems to run successfully on my install.

In 48785.4.diff I noticed the:

assertTrue( array_key_exists( ... ) )
assertFalse( array_key_exists(  ... ) )

I think alternatively one could use:

assertArrayHasKey
assertArrayNotHasKey

I don't think we need to use __() in test method (no locale assertions).


#17 follow-up: @TimothyBlynJacobs
4 years ago

That’s a good point @birgire, do you want to add an updated patch?

@birgire
4 years ago

#18 in reply to: ↑ 17 @birgire
4 years ago

Replying to TimothyBlynJacobs:

That’s a good point @birgire, do you want to add an updated patch?

Sure, thanks, 48785.5.diff uses

assertArrayHasKey
assertArrayNotHasKey

and also:

assertEqualSetsWithIndex

It also removes __() function dependency.

And adds @since 5.4.0 in the new test class file (+ minor inline docs additions).

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

This ticket was mentioned in PR #160 on WordPress/wordpress-develop by TimothyBJacobs.


4 years ago
#19

#20 @TimothyBlynJacobs
4 years ago

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

In 47328:

REST API: Don't assume all item schemas have properties.

All schema types, not just objects, are permitted as the base type of a resource. A future patch could add validation support for those types, but this fix only prevents a PHP warning from being issued.

Props dhavalkasvala, johnwatkins0, birgire.
Fixes #48785.

#21 @ajaaskel
4 years ago

Would you please explain a little bit more. It's nice to hear "it's fixed" but it's not obvious for a system maintainer how to fix the thing. Where/how the fix is available?
I faced the problem when running search/replace at WP client software (a special tool for maintaining WordPress on command line). The purpose is to change the complete setup to a new domain. The tool does not change anything but gives up at that error.
A sample:

from_domain=domain1.com
to_domain=domain2.com

wp --path=/home/www/domain/"$our_domain"/html search-replace "$from_domain" "$our_domain"

PHP Warning: Invalid argument supplied for foreach() in /home/www/domain/domain2.com/html/wp-includes/rest-api/endpoints/class-wp-rest-controller.php on line 287
Warning: Invalid argument supplied for foreach() in /home/www/domain/domain2.com/html/wp-includes/rest-api/endpoints/class-wp-rest-controller.php on line 287

#22 @TimothyBlynJacobs
4 years ago

Hi @ajaaskel,

By "fixed" this means that the fix was included in the version of WordPress being worked on at the time. That is listed in the "Milestone:" section of the trac ticket.

So if you are running WordPress 5.4, you shouldn't see that warning any longer.

Note: See TracTickets for help on using tickets.