WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 8 days ago

#45677 assigned defect (bug)

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 needs-testing
Focuses: rest-api Cc:

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 (2)

45677.diff (2.5 KB) - added by TimothyBlynJacobs 3 months ago.
45677.2.diff (2.4 KB) - added by TimothyBlynJacobs 3 months ago.

Download all attachments as: .zip

Change History (20)

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


6 months ago

#2 @desrosj
6 months ago

  • Version changed from 5.0.1 to 5.0

#3 @desrosj
6 months ago

  • Severity changed from major to normal

#4 @desrosj
6 months ago

  • Milestone changed from Awaiting Review to 5.2

#5 @TimothyBlynJacobs
6 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.


4 months ago

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


4 months ago

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


3 months ago

#9 @kadamwhite
3 months ago

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

Assigning to @TimothyBlynJacobs per weekly REST API team meeting discussion

#10 @pat@…
3 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
3 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
3 months ago

  • Keywords has-patch needs-testing added

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


3 months ago

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


3 months ago

#15 @adamsilverstein
3 months ago

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

#16 @adamsilverstein
3 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
3 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.


8 days ago

Note: See TracTickets for help on using tickets.