Make WordPress Core

Opened 21 months ago

Closed 17 months ago

Last modified 16 months ago

#53656 closed defect (bug) (fixed)

Not possible to get a post type's REST API namespace if modified

Reported by: swissspidy's profile swissspidy Owned by: spacedmonkey's profile spacedmonkey
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description (last modified by swissspidy)

It's possible to supply a custom rest_controller_class to register_post_type(). A post type's controller class can be retrieved using WP_Post_Type::get_rest_controller(). Same goes for taxonomies & WP_Taxonomy.

Now, such a custom controller can potentially change the namespace and rest_base for the post type, like so:

<?php
class My_Controller extends WP_REST_Posts_Controller {
        public function __construct() {
                parent::__construct();
                $this->namespace = 'foo/v1';
        }
}

register_post_type('bar', array( 'rest_controller_class' => My_Controller::class ) );

However, there's now no way to get the post type's REST API base URL (wp-json/foo/v1/bar.

For example, /wp-json/wp/v2/types/bar will link to /wp-json/wp/v2/bar because the wp/v2 namespace is hardcoded in WP_REST_Post_Types_Controller.

Is there a way to get a post type's updated namespace and use that in WP_REST_Post_Types_Controller and other places?

Hardcoding everything to wp/v2 seems a bit limiting.

Or is the namespace perhaps not intended to be modified like this?

Change History (23)

#1 @TimothyBlynJacobs
21 months ago

  • Milestone changed from Awaiting Review to Future Release
  • Version set to 4.7

This is part of the reasoning behind introducing the rest_get_route_for_post functions, https://make.wordpress.org/core/2020/07/22/rest-api-changes-in-wordpress-5-5/. We don't have a collection version of this, but I think we could introduce one.

#2 @desrosj
21 months ago

  • Keywords needs-patch added

#3 follow-up: @swissspidy
19 months ago

@TimothyBlynJacobs even rest_get_route_for_post() has this same problem, as the wp/v2 namespace is hardcoded there. But at least there's a filter.

To fix this, WP_REST_Controller needs a getter to access $namespace and $rest_base. Then functions like rest_get_route_for_post() or a potential rest_get_route_for_post_type() could get the REST controller for the entity and get the route via those getters.

#4 in reply to: ↑ 3 @TimothyBlynJacobs
18 months ago

Replying to swissspidy:

To fix this, WP_REST_Controller needs a getter to access $namespace and $rest_base. Then functions like rest_get_route_for_post() or a potential rest_get_route_for_post_type() could get the REST controller for the entity and get the route via those getters.

IMO instead of exposing a getter to access this info, we should follow the rest_base paradigm and add a rest_namespace argument when registering the post type. I don't think we should be reaching into the controller instance.

#5 @swissspidy
18 months ago

That also works I suppose, and would be consistent with rest_base

#6 @swissspidy
18 months ago

  • Description modified (diff)

#7 @swissspidy
18 months ago

Related Gutenberg ticket, just posting for context: https://github.com/WordPress/gutenberg/issues/33420

This ticket was mentioned in PR #1749 on WordPress/wordpress-develop by spacedmonkey.


18 months ago
#8

  • Keywords has-patch added; needs-patch removed

#9 @spacedmonkey
18 months ago

  • Focuses rest-api added
  • Keywords needs-unit-tests added
  • Milestone changed from Future Release to 5.9
  • Owner set to spacedmonkey
  • Status changed from new to assigned

I have created a PR / patch for this ticket at #1749.

This PR adds the following

  • This adds a new rest_namespace param to register_post_type / WP_Post_Type.
  • Updates REST API controller to read from rest_namespace property of the WP_Post_Type object in all the REST API controllers.
  • Add field to Post types controller.
  • Update logic in rest_get_route_for_post function

The patch does have unit tests yet. Can you take a look @swissspidy @TimothyBlynJacobs and see if you are happy with the approach, I will add unit tests and move this forward.

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


18 months ago

#11 @spacedmonkey
18 months ago

This ticket was discussed in this week's REST API core meeting.

It was decided that the patch is sound but needs to be updated with unit tests.

A follow up ticket was created to apply the same changes, the taxonomies. See #54267 for more details.

#12 @spacedmonkey
17 months ago

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

I have added unit tests to my patch.

TimothyBJacobs commented on PR #1749:


17 months ago
#13

Tests look great, thanks @spacedmonkey! Just a question about the naming.

spacedmonkey commented on PR #1749:


17 months ago
#14

Tests look great, thanks @spacedmonkey! Just a question about the naming.

Function change name to rest_get_route_for_post_type_items.

#15 @spacedmonkey
17 months ago

As #54267 now has a patch as well, once this ticket is merge, we should also merge #54267 at the same time / close behind it.

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


17 months ago

TimothyBJacobs commented on PR #1749:


17 months ago
#17

So upon further review, we are going to need to remove the specific controller class checks from this method as well as the rest_get_route_for_post method. Otherwise, anyone who has a custom controller, even if they have registered the routes as Core expects, as we are now using these methods for beyond generating informative links.

spacedmonkey commented on PR #1749:


17 months ago
#18

So upon further review, we are going to need to remove the specific controller class checks from this method as well as the rest_get_route_for_post method. Otherwise, anyone who has a custom controller, even if they have registered the routes as Core expects, as we are now using these methods for beyond generating informative links.

I noticed this myself. I was going to note it the dev blog after release.

I wasn't 100% why the check was in the there in the first place, felt like over kill to me. I also don't like that you have to spin up a instance of the class to do this check. Much cleaner to just use the show_in_rest param.

spacedmonkey commented on PR #1749:


17 months ago
#19

Update the patch based on feedback.

#20 @TimothyBlynJacobs
17 months ago

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

In 51962:

REST API: Support custom namespaces for custom post types.

While a custom post type can define a custom route by using the rest_base argument, a namespace of wp/v2 was assumed. This commit introduces support for a rest_namespace argument.

A new rest_get_route_for_post_type_items function has been introduced and the rest_get_route_for_post function updated to facilitate getting the correct route for custom post types.

While the WordPress Core Block Editor bootstrap code has been updated to use these API functions, for maximum compatibility sticking with the default wp/v2 namespace is recommended until the API functions see wider use.

Props spacedmonkey, swissspidy.
Fixes #53656.

#23 @spacedmonkey
16 months ago

In 52363:

REST API: Ensure that the parent link, uses the rest_get_route_for_post function.

In [51962] the rest_get_route_for_post function was implemented in all places where link to a post's REST API endpoint is needed. However in this commit, the up link, which links to the parent post of the current object, did not use this function.

Props Spacedmonkey, SergeyBiryukov.
Fixes #53656.

Note: See TracTickets for help on using tickets.