Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

spacedmonkey
Copy link
Member

Trac ticket: https://core.trac.wordpress.org/ticket/53656
GB issue: WordPress/gutenberg#33420


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

$this->rest_base = 'autosaves';
$this->namespace = ! empty( $post_type_object->rest_namespace ) ? $post_type_object->rest_namespace : 'wp/v2';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these checks necessary? AFAICT, we set a default value for the namespace in the post type object, so it should never be empty, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

( Same for other instances of the same check )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry here, that there is post type object is created in a strange way and that somehow this property does not exist. Or that it is set to false for some reason. This is just a little hardnessing.

@TimothyBJacobs
Copy link
Member

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

@spacedmonkey
Copy link
Member Author

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

Function change name to rest_get_route_for_post_type_items.

@TimothyBJacobs
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

Update the patch based on feedback.

@TimothyBJacobs TimothyBJacobs force-pushed the try/post-namespace branch 2 times, most recently from 0b9b1c8 to d6b1631 Compare October 31, 2021 20:42
Use rest_get_route_for_post in for media.

Add rest_get_route_for_post_type function.

use show_in_rest.

Apply suggestions from code review

Update src/wp-includes/class-wp-post-type.php

More tweaks

Update fixtures.

Update spacing.

Improve tests.

Improve tests.

Change function name.

Update src/wp-includes/rest-api/endpoints/class-wp-rest-post-types-controller.php

Remove checks for class instances.

Use rest_get_route_for_post_type_items in more places

use in rest_get_route_for_post_type_items in rest_get_route_for_post
@TimothyBJacobs
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants