REST Autosaves Controller creating extra instance of a post type REST controller

[pat@…]

If I register a REST controller using the 'rest_controller_class' parameter on register_post_type I would expect that it's instantiated once per REST request. This has been the case prior to 5.0

In my constructors I often add a filter on "rest_prepare_{$post_type}" or "rest_{$post_type}_query" to influence the input/output. These filters are now being run twice per request.

The WP_REST_Autosaves_Controller is created one per post type, and it's constructor creates a new instance of the post_type's controller. This seems unneccessary and it breaks some existing implementations. Why not pass the post type REST controller in to the constructor?

[TimothyBlynJacobs]

I don't love the idea of these filters being registered in the class constructor, they could possible go in register_routes. That being said we should probably reuse the same instance anyways, and we have it available when instantiating the autosaves controller.

We should probably make the parameter optional as well and instantiate it if not passed to maintain BC.

@patrelentlesstechnologycom Do you want to work on a patch for this?

[kadamwhite]

Assigning to @TimothyBlynJacobs per weekly REST API team meeting discussion

[pat@…]

@TimothyBlynJacobs Hi Timothy, I'm sorry I didn't see this sooner. I see you've patched it already. I haven't yet contributed to the core, but I've been working with WP every day for 10 years. I use the REST API pretty heavily and would love to be a bit more involved in it's development. In particular it'd be great to have support for custom contexts beyond the basics ( list, view, embed, edit ). I've added support for this through my own implementations but would be interested in contributing this functionality if you guys are interested.

[TimothyBlynJacobs]

@patrelentlesstechnologycom No worries! We'd be happy to have you. We meet every Thursday at 18:00 UTC in slack #core-restapi. We're actively looking at improving and/or replacing context, so feedback about how you are using context and where it might fall short would be quite useful. It'd be best to share at an upcoming meeting, or on make.wordpress.org where there'll probably eventually be a post soliciting feedback.

​https://make.wordpress.org/chat/

[adamsilverstein]

45677.2.diff​ looks good, nice work @TimothyBlynJacobs! I'll give this a test.

[adamsilverstein]

Testing this locally, autosaves continue to work as expected. I wasn't able to reproduce the rest_prepare_{$post_type} getting called multiple times. @patrelentlesstechnologycom are you able to test this patch and verify it fixes your issue?

[pento]

Bumping to 5.3, as the patch needs more testing and review.

[TimothyBlynJacobs]

Related: #40383

[TimothyBlynJacobs]

Since we need a shared instance to support #40383, I decided to drop the constructor parameter for the parent post type controller and instead always fetch it through the static WP_REST_Posts_Controller::get_for_post_type.

[TimothyBlynJacobs]

Adds a check to make sure show_in_rest is true before returning a valid post type controller.

[TimothyBlynJacobs]

Uploaded a version of the patch that uses WP_Post_Type::get_rest_controller() as described in https://core.trac.wordpress.org/ticket/40383#comment:5.

Now that the REST API schema is cached on an instance property of the WP_REST_Controller, any changes to the registered fields would not be reflected when running unit tests since we'd reuse the same WP_REST_Controller instance.

Moving it onto the post type kill this cache since the post types are unregistered and registered in WP_UnitTestCase_Base::reset_post_types.

Alternatively, we could kill the static cache on WP_REST_Controller in tearDown on REST API tests.

[kadamwhite]

In 46272:

REST API: Introduce WP_Post_Type::get_rest_controller() caching method to prevent unnecessary REST controller construction.

Cache REST controller references on their associated post type object to prevent unnecessary controller re-instantiation, which previously caused "rest_prepare_{$post_type}" and "rest_{$post_type}_query" to run twice per request.

Props TimothyBlynJacobs, patrelentlesstechnologycom.
Fixes #45677.

[dlh]

My apologies for reopening a ticket, but I wanted to offer a couple janitorial comments for [46272] outlined in 45677.11.diff​:

• The private static $post_type_controllers property was added but doesn't appear to be used.

• Once the controller instance is set on a WP_Post_Type, can it be returned straightaway? Can it not be because that would allow someone to register a post type with an arbitrary rest_controller property?

[TimothyBlynJacobs]

Thanks for the catch @dlh!

The private static $post_type_controllers property was added but doesn't appear to be used.

This was used in a previous version of the patch and I missed cleaning it up. It can be safely removed.

Can it not be because that would allow someone to register a post type with an arbitrary rest_controller property?

Yeah that was my thinking. Though we still probably aren't strict enough as you could pass a rest_controller argument as long as the rest_controller_class was valid. We should probably enforce that rest_controller is an instanceof rest_controller_class and that the rest_controller_class is valid. We also don't want to return a controller instance if the post type is not show_in_rest.

[dlh]

I can work on a patch to add those checks, but could you clarify what you mean by validating the rest_controller_class?

Separately, by pure chance, a colleague found some code in the wild today that generates a fatal error after [46272]. Simplified, it is:

$pt_object = get_post_type_object( 'post' );
$pt_object->rewrite = array( ... );
register_post_type( 'post', (array) $pt_object );

It breaks with the introduction of the private controller property, which becomes \0WP_Post_Type\0rest_controller after being cast to (array) and can't be set using $this->$property_name in set_props().

The approach in that snippet is not ideal and so perhaps not in need of special handling now, but I mention it "for the record" with respect to backcompat.

[TimothyBlynJacobs]

I can work on a patch to add those checks

Awesome, thank you!

but could you clarify what you mean by validating the rest_controller_class?

We always want to make sure that ::get_rest_controller() is an instance of the rest_controller_class and the rest_controller_class subclasses WP_REST_Controller.

---

That's an interesting case. I'm fine with changing the accessibility to public. Especially given the extra validation.

[kadamwhite]

One open question @dlh has raised is whether we'd ever be likely to introduce additional non-public properties to WP_Post_Type or related long-standing classes which currently only have public properties. Noting that question here in case any other committers with historical perspective on the matter can weigh in.

[dlh]

45677.12.diff​ has some updates based on the discussion above.

• Remove the extra private static $post_type_controllers.
• Make $rest_controller public.
• Validate that $this->rest_controller instanceof $class before returning the controller, and add a couple related tests.

[TimothyBlynJacobs]

Thanks for the patch @dlh! This looks good to me.

[kadamwhite]

In 46435:

REST API: Ensure rest_controller instantiates the post type's declared REST controller class.

Ensures that the ::get_rest_controller() method will always return an instanceof the expected controller class, or null.
Removes unused private static property $post_type_controllers.

Props dlh, TimothyBlynJacobs.
Fixes #45677.