WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 11 days ago

#51020 new defect (bug)

The public route args schema drops valid JSON Schema properties

Reported by: raubvogel Owned by:
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: dev-feedback has-patch needs-unit-tests
Focuses: rest-api Cc:

Description

The minItems and maxItems documented at https://developer.wordpress.org/rest-api/extending-the-rest-api/schema/#minitems-and-maxitems are not shown in the argument schema.

Add argument like (note minItems and maxItems)

<?php
$args = ['f_marker_author_args'    => [
                                'description' => __( 'Filter.', PVGW_TEXT_DOMAIN ),
                                'required'    => false,
                                'type'        => 'array',
                                'minItems'    => 1,
                                'maxItems'    => 2,
                                'items'       => [ 'type' => 'integer' ],
                        ]]

to

<?php
register_rest_route( $this->namespace, '/' . $this->rest_base,
                        array(
                                // register the readable endpoint for collections
                                [
                                        'methods'             => WP_REST_Server::READABLE,
                                        'callback'            => [ $this, 'get_items' ],
                                        'permission_callback' => [ $this, 'get_items_permissions_check' ],
                                        'args'                => $args,
                                ],
                                // register schema callback
                                'schema' => [ $this, 'get_public_item_schema' ],
                        )
                );

If I do something like OPTIONS http://localhost/wordpress-beta/wp-json/pvgw/v1/markers, in part I only get

"f_marker_author_args": {
                    "required": false,
                    "description": "Filter.",
                    "type": "array",
                    "items": {
                        "type": "integer"
                    }
                },

The keys minItems and maxItems are missing.

Attachments (2)

51020#.diff (3.4 KB) - added by raubvogel 5 weeks ago.
51020#.2.diff (3.6 KB) - added by raubvogel 5 weeks ago.
version 2

Download all attachments as: .zip

Change History (10)

#1 @TimothyBlynJacobs
5 weeks ago

  • Milestone changed from Awaiting Review to 5.6
  • Summary changed from minItems/maxItems not shown in argument schema to The public route args schema drops valid JSON Schema properties
  • Version changed from trunk to 4.7

Thanks for the ticket @raubvogel!

This is because the REST Server only adds a small number of the JSON Schema properties to the args: https://github.com/WordPress/wordpress-develop/blob/8c378606ab14c2e426c719de231bf63b05d45420/src/wp-includes/rest-api/class-wp-rest-server.php#L1300

The full list of what should be allowed is in WP_REST_Controller::get_endpoint_args_for_item_schema(): https://github.com/WordPress/wordpress-develop/blob/8c378606ab14c2e426c719de231bf63b05d45420/src/wp-includes/rest-api/endpoints/class-wp-rest-controller.php#L632

We should probably update WP_REST_Server::get_data_for_route() to include that same list of valid property names.

Do you want to work on a patch for this?

#2 @raubvogel
5 weeks ago

@TimothyBlynJacobs, thank you! Yes, I will give it a try …

@raubvogel
5 weeks ago

#3 @raubvogel
5 weeks ago

  • Keywords has-patch added; needs-patch removed

@TimothyBlynJacobs, can you review the patch, please? I did some testing (with my Plugin) and it seems to work.

Last edited 5 weeks ago by raubvogel (previous) (diff)

#4 @TimothyBlynJacobs
5 weeks ago

Thanks for the patch @raubvogel, this looks good!

Instead of making this a method on WP_REST_Controller though, let's make this a standalone function: rest_get_allowed_schema_keywords.

Do you want to try writing unit tests for this?

#5 @raubvogel
5 weeks ago

@TimothyBlynJacobs, ok, on my way …

@raubvogel
5 weeks ago

version 2

#6 @raubvogel
5 weeks ago

@TimothyBlynJacobs, next patch is ready for review.

I guess for the unit tests, I will need some help (phpunit is working already). I never have done this for WordPress before.

#7 @raubvogel
2 weeks ago

  • Keywords needs-testing added

#8 @TimothyBlynJacobs
11 days ago

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

That's looking good @raubvogel! I'd recommend opening a Pull Request in WordPress/wordpress-develop. That will run unit tests as well as linting automatically.

In terms of writing the test. You'll want to add a test case to the existing collection of tests covering WP_REST_Server. These are in the tests/phpunit/tests/rest-api/rest-server.php file. An example of one of these tests is test_get_index. I would add another method after that one following a pattern something like this.

<?php
public function test_get_index_for_route() {
    $server = new WP_REST_Server();
    $server->register_route(
        'test-ns/v1',
        '/test',
        array(
            array(
                'methods'  => WP_REST_Server::READABLE,
                'callback' => '__return_true',
                'args'     => array(
                    // The list of args you want to verify.
                    'test' => array( 'type' => 'array' ),
                ),
            ),
        )
    );

    $request = new WP_REST_Request( 'GET', '/test-ns/v1' );
    $index   = $server->dispatch( $request );
    $data    = $index->get_data();
    $args    = $data['routes']['/test']['args'];

    $this->assertArrayHasKey( 'schemaKeyword', $args['test'] );
}

In the route registration you'd add the arguments that use the different schema keywords. Then verify that they are present in the output.

I hope that helps, let me know if you run into issues. You can ping me on Slack timothybjacobs.


Something we'll might want to consider is whether we should traverse the schema's properties and items and apply the same sanitization as we do on the top-level. It was decided not to do that for items in #38420. I think we'd probably stick with that decision unless that are compelling reasons we shouldn't.

Note: See TracTickets for help on using tickets.