WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 2 weeks ago

#47779 accepted enhancement

Add a filter to `get_item_schema()` to be able to extend the schema of existing properties

Reported by: luisherranz Owned by: kadamwhite
Milestone: 5.4 Priority: normal
Severity: trivial Version: 4.7
Component: REST API Keywords: has-patch dev-feedback needs-unit-tests
Focuses: rest-api Cc:
PR Number:

Description

Right now there's no way to properly extend the schema of existing fields with additional properties. For example, the content field could be extended with new properties (in addition to content.rendered and content.raw).

Adding a filter to WP_REST_Posts_Controller::get_item_schema() would help solve this problem.

Attachments (5)

class-wp-rest-posts-controller.diff (816 bytes) - added by luisherranz 5 months ago.
Added filter rest_{$this->post_type}_item_schema in class-wp-rest-posts-controller.php
47779.2.diff (1.4 KB) - added by luisherranz 2 months ago.
Emit a _doing_it_wrong warning if user tries to add new properties with this filter instead of register_rest_field.
47779.3.diff (3.0 KB) - added by luisherranz 2 months ago.
I've added the first test, but it's not working yet. Somehow the same code works perfectly fine in the regular site.
47779.3.2.diff (3.0 KB) - added by luisherranz 2 months ago.
I've added the first test, but it's not working yet. Somehow the same code works perfectly fine in the regular site.
47779.4.diff (3.4 KB) - added by luisherranz 5 weeks ago.
I fixed the test and added an assertion for the context. I also beautified the code.

Download all attachments as: .zip

Change History (18)

@luisherranz
5 months ago

Added filter rest_{$this->post_type}_item_schema in class-wp-rest-posts-controller.php

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


5 months ago

#2 @luisherranz
4 months ago

For more context about why this filter is needed, please take a look at this slack conversation:
https://wordpress.slack.com/archives/C02RQC26G/p1563397511313900

Pascal's question appeared because of this new feature of the AMP plugin:
https://github.com/ampproject/amp-wp/issues/1014
https://github.com/ampproject/amp-wp/pull/2827

#3 @TimothyBlynJacobs
4 months ago

I think we should consider not allowing people to add properties to the main object via this filter and enforce people to use the additional fields API for that case.

Something like comparing the keys of $schema['properties'] before and after the filter call and issuing a _doing_it_wrong.

#4 @wpscholar
4 months ago

  • Keywords has-patch dev-feedback needs-unit-tests added
  • Version set to 4.7

@luisherranz
2 months ago

Emit a _doing_it_wrong warning if user tries to add new properties with this filter instead of register_rest_field.

@luisherranz
2 months ago

I've added the first test, but it's not working yet. Somehow the same code works perfectly fine in the regular site.

@luisherranz
2 months ago

I've added the first test, but it's not working yet. Somehow the same code works perfectly fine in the regular site.

#5 @luisherranz
2 months ago

This filter works fine in the regular site:

function filter_post_item_schema( $schema ) {
        $schema['properties']['content']['properties']['new_prop'] = array(
                'description' => __( 'A new prop added with a the rest_post_item_schema filter.' ),
                'type'        => 'string',
                'context'     => array( 'view', 'edit' ),
        );
        return $schema;
}

add_filter( 'rest_post_item_schema', 'filter_post_item_schema' );

However, in the testing environment, the filter doesn't seem to work:

public function test_rest_post_type_item_schema_filter_change_property() {
                add_filter( 'rest_post_item_schema', array( $this, 'filter_post_item_schema' ) );
                
                $request    = new WP_REST_Request( 'OPTIONS', '/wp/v2/posts' );
                $response   = rest_get_server()->dispatch( $request );
                $data       = $response->get_data();
                $properties = $data['schema']['properties']['content']['properties'];
                
                remove_filter( 'rest_post_item_schema', array( $this, 'filter_post_item_schema' ) );
                
                $this->assertArrayHasKey( 'new_prop', $properties );
        }

...

public function filter_post_item_schema( $schema ) {
                $schema['properties']['content']['properties']['new_prop'] = array(
                        'description' => __( 'A new prop added with a the rest_post_item_schema filter.' ),
                        'type'        => 'string',
                        'context'     => array( 'view', 'edit' ),
                );
                return $schema;
        }

Any help is appreciated.

Once this is solved I'll add more use cases to the test.

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


2 months ago

#7 @luisherranz
2 months ago

Ok, I had some more time today to look into this and I found the problem.

Looking at the stack, it looks like get_item_schema is called on the rest_api_init hook, which is called on the WP_Test_REST_Posts_Controller->setUp method.

That means we would need to add the filter before the setUp, or do the setUp or the rest_api_init again. I don't like either of those.

I've tested adding the filter in the setUp just to check if the test is passing and the test is working:

public function setUp() {
  add_filter( 'rest_post_item_schema', array( $this, 'filter_post_item_schema' ) );
  parent::setUp(); // <- this is where the do_action( 'rest_api_ini' ) happens.
  // the rest of the setUp...

Where should I add the filter? Any idea?

#8 @swissspidy
7 weeks ago

Such a filter seems very crucial for enabling other contexts beyond view, edit, and embed, so +1 on adding one.

I might wanna add a new field for post objects using register_rest_field that only works in context foo, and at the same time also want to use some of the existing fields like id in that context.
This doesn't seem to be doable without this proposed filter, as id in the schema is hardcoded to the view, edit, and embed contexts and not available otherwise.

@kadamwhite Since you chimed in on https://github.com/ampproject/amp-wp/issues/1014 originally - do you perhaps have any insights here regarding adding and testing such a filter?

#9 @kadamwhite
7 weeks ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to kadamwhite
  • Status changed from new to accepted

I'd be in favor of this, I'd seen this ticket when it was filed but it dropped off my radar; milestoning for 5.4 to make sure we get to it!

#10 @luisherranz
6 weeks ago

Awesome :)

Do you have any idea about how to solve the test problem with rest_api_init?

I don't like neither of these solutions:

  • Add the filter before parent::setUp().
  • Do the setUp again in the test.
  • Do the rest_api_init again in the test.
  • Don't do setUp at initialization and move it inside each individual test.
  • Create a new file for filters that depend on rest_api_init.

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


6 weeks ago

@luisherranz
5 weeks ago

I fixed the test and added an assertion for the context. I also beautified the code.

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


5 weeks ago

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


2 weeks ago

Note: See TracTickets for help on using tickets.