WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 6 months ago

Last modified 3 months ago

#48785 closed defect (bug) (fixed)

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

Reported by: pento Owned by: 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 8 months ago.
#48785-improve.patch (534 bytes) - added by dhavalkasvala 8 months ago.
48785.diff (2.7 KB) - added by johnwatkins0 6 months 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 6 months ago.
Made unit test PHP5-compatible
48785.3.diff (3.4 KB) - added by johnwatkins0 6 months ago.
48785.4.diff (3.7 KB) - added by johnwatkins0 6 months ago.
48785.5.diff (3.9 KB) - added by birgire 6 months ago.

Download all attachments as: .zip

Change History (29)

#1 @dkarfa
8 months ago

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

#2 @TimothyBlynJacobs
8 months 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
8 months ago

  • Keywords has-patch added; needs-patch removed

#4 @TimothyBlynJacobs
8 months 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
8 months 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
8 months ago

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

Version 1, edited 8 months ago by dhavalkasvala (previous) (next) (diff)

#7 @TimothyBlynJacobs
8 months 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.


7 months ago

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


7 months ago

@johnwatkins0
6 months ago

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

#10 @johnwatkins0
6 months ago

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

@johnwatkins0
6 months ago

Made unit test PHP5-compatible

#11 @TimothyBlynJacobs
6 months 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?

#12 @johnwatkins0
6 months ago

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

#13 @TimothyBlynJacobs
6 months 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.

#14 @johnwatkins0
6 months ago

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

#15 @TimothyBlynJacobs
6 months ago

  • Keywords commit added

Thanks for refreshing @johnwatkins0, looks great!

#16 @birgire
6 months 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
6 months ago

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

@birgire
6 months ago

#18 in reply to: ↑ 17 @birgire
6 months 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 6 months ago by birgire (previous) (diff)

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


6 months ago

#20 @TimothyBlynJacobs
6 months 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
3 months 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
3 months 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.