WordPress.org

Make WordPress Core

Opened 15 months ago

Closed 12 months ago

Last modified 12 months ago

#51020 closed defect (bug) (fixed)

The public route args schema drops valid JSON Schema properties

Reported by: raubvogel Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: dev-feedback has-patch has-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 15 months ago.
51020#.2.diff (3.6 KB) - added by raubvogel 15 months ago.
version 2

Download all attachments as: .zip

Change History (16)

#1 @TimothyBlynJacobs
15 months 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
15 months ago

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

@raubvogel
15 months ago

#3 @raubvogel
15 months 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 15 months ago by raubvogel (previous) (diff)

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

@TimothyBlynJacobs, ok, on my way …

@raubvogel
15 months ago

version 2

#6 @raubvogel
15 months 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
14 months ago

  • Keywords needs-testing added

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

#9 follow-up: @TimothyBlynJacobs
13 months ago

Hi @raubvogel!

Just wanted to check in with you. Are you still interested in trying to write Unit Tests for this patch? No worries if not.

#10 in reply to: ↑ 9 @raubvogel
13 months ago

Replying to TimothyBlynJacobs:

Hi @TimothyBlynJacobs!

I had a short look at the Unit Tests, but for now it would be nice if you could write them – sorry for that!

Can I do something else? Is the patch fine?

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


12 months ago

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

#12 follow-up: @TimothyBlynJacobs
12 months ago

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

In 49257:

REST API: Make sure all supported JSON Schema keywords are output in the index.

Previously, only a small subset of keywords were exposed which limited the utility of OPTIONS requests.

Props raubvogel, TimothyBlynJacobs.
Fixes #51020.

#13 in reply to: ↑ 12 @raubvogel
12 months ago

@TimothyBlynJacobs, thank your very much!

Note: See TracTickets for help on using tickets.