WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 6 weeks ago

#45677 closed defect (bug) (fixed)

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

Reported by: pat@… Owned by: TimothyBlynJacobs
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.0
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: rest-api Cc:
PR Number:

Description

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?

Attachments (12)

45677.diff (2.5 KB) - added by TimothyBlynJacobs 8 months ago.
45677.2.diff (2.4 KB) - added by TimothyBlynJacobs 8 months ago.
45677.3.diff (8.1 KB) - added by TimothyBlynJacobs 3 months ago.
45677.4.diff (7.3 KB) - added by TimothyBlynJacobs 3 months ago.
45677.5.diff (9.3 KB) - added by TimothyBlynJacobs 3 months ago.
45677.6.diff (11.3 KB) - added by TimothyBlynJacobs 3 months ago.
45677.7.diff (9.7 KB) - added by TimothyBlynJacobs 3 months ago.
45677.8.diff (9.8 KB) - added by TimothyBlynJacobs 3 months ago.
45677.9.diff (10.1 KB) - added by TimothyBlynJacobs 3 months ago.
45677.10.diff (8.9 KB) - added by TimothyBlynJacobs 2 months ago.
45677.11.diff (1.1 KB) - added by dlh 8 weeks ago.
45677.12.diff (2.5 KB) - added by dlh 7 weeks ago.

Download all attachments as: .zip

Change History (53)

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


10 months ago

#2 @desrosj
10 months ago

  • Version changed from 5.0.1 to 5.0

#3 @desrosj
10 months ago

  • Severity changed from major to normal

#4 @desrosj
10 months ago

  • Milestone changed from Awaiting Review to 5.2

#5 @TimothyBlynJacobs
10 months ago

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?

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


8 months ago

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


8 months ago

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


8 months ago

#9 @kadamwhite
8 months ago

  • Owner set to TimothyBlynJacobs
  • Status changed from new to assigned

Assigning to @TimothyBlynJacobs per weekly REST API team meeting discussion

#10 @pat@…
8 months ago

@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.

#11 @TimothyBlynJacobs
8 months ago

@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/

#12 @JeffPaul
7 months ago

  • Keywords has-patch needs-testing added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 months ago

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


7 months ago

#15 @adamsilverstein
7 months ago

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

#16 @adamsilverstein
7 months ago

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?

#17 @pento
7 months ago

  • Milestone changed from 5.2 to 5.3

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

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


5 months ago

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


4 months ago

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


3 months ago

#22 @TimothyBlynJacobs
3 months ago

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.

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


3 months ago

#24 @TimothyBlynJacobs
3 months ago

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

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

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


3 months ago

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


3 months ago

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


2 months ago

#28 @TimothyBlynJacobs
2 months ago

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.

#29 @kadamwhite
2 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

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


2 months ago

@dlh
8 weeks ago

#31 @dlh
8 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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?

#32 @TimothyBlynJacobs
8 weeks ago

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.

#33 @dlh
7 weeks ago

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.

#34 @TimothyBlynJacobs
7 weeks ago

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.

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


7 weeks ago

This ticket was mentioned in Slack in #core-committers by kadamwhite. View the logs.


7 weeks ago

#37 @kadamwhite
7 weeks ago

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
7 weeks ago

#38 @dlh
7 weeks ago

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.

#39 @TimothyBlynJacobs
7 weeks ago

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

#40 @kadamwhite
7 weeks ago

  • Keywords commit added

#41 @kadamwhite
6 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.