Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#49116 closed enhancement (fixed)

Add Links to the REST version of a Resource in the header of the page

Reported by: dshanske's profile dshanske Owned by: dshanske's profile dshanske
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: needs-unit-tests dev-feedback has-dev-note
Focuses: rest-api Cc:

Description

Proposing as suggested by @timothybjacobs in Slack, that on each page, a link in the header of the HTML file to the REST route for that resource.

So, an author page would link to the user endpoint for that user. A post to the rest route to that specific post, etc.

The rel on the link to the REST API endpoint is rel="https://api.w.org/". This should probably be a rel="alternate" which indicates an alternative representation of the page, with type "application/json" as it is technically a json representation of the page, but this may need more discussion.

Attachments (2)

49116.diff (3.7 KB) - added by dshanske 4 years ago.
49116.2.diff (5.4 KB) - added by dshanske 4 years ago.

Download all attachments as: .zip

Change History (35)

#1 @TimothyBlynJacobs
5 years ago

I would agree that alternate makes sense for the relation. https://html.spec.whatwg.org/multipage/links.html#rel-alternate

#2 @dshanske
4 years ago

@TimothyBlynJacobs Do we want to try to get this in for the next release?

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


4 years ago

#5 @dshanske
4 years ago

Been looking at this again, and the problem with this is that mime type application/json is too generic. It doesn't indicate this is a rest return. And the api.w.org alternate compound rel I was looking at was ignored by many of the third-party parsers I tried.

So, the best solution appears to be to register a vendor specific mime type for the rest api. https://www.iana.org/form/media-types. I would suggest application/vnd.wordpress+json or application/vnd.wp+json. Then we can simply identify a wordpress rest api resource link. This may require a separate issue to discuss.

#6 @dshanske
4 years ago

  • Milestone changed from Awaiting Review to 5.5

#7 @TimothyBlynJacobs
4 years ago

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

How are things going on this front @dshanske? I think the main work to be done here is doing the actual mapping. We can figure out what content-type or rel to use at a later point.

#8 @dshanske
4 years ago

@TimothyBlynJacobs The proof of concept in the form of a plugin covered all the resources I could think of. I could repackage it as a patch.

#9 @TimothyBlynJacobs
4 years ago

Yeah let's convert this to a patch. The things that stand out for me.

  1. We need to handle singular non-posts, right now we are just pushing any singular posts to /posts.
  2. Instead of being specific to categories and tags, we should handle any taxonomy.

#10 @dshanske
4 years ago

@TimothyBlynJacobs Been thinking about this and it ends up being a bit more complicated. You can, from a post type, for example, get the rest controller...but what is the best way to know what the resource is based on the post? I'm trying to figure out a more generic methodology.

Are we safe just supporting the built-in controllers, and then adding a filter and putting the onus on people creating custom endpoints to handle that themselves?

This is probably a good place for me to start. https://developer.wordpress.org/reference/classes/wp_rest_posts_controller/prepare_links/

#11 @TimothyBlynJacobs
4 years ago

@dshanske I would look at WP_REST_Post_Search_Handler::detect_rest_item_route. We essentially only support using the built-in class.

I would be in support of introducing something like: rest_get_route_for_post( WP_Post ): string. Importantly only returning the route, it would still need to be passed to rest_url to retrieve the full url. That could have a filter on it to support custom controllers. Similar methods could also be introduced for terms.

@dshanske
4 years ago

#12 @dshanske
4 years ago

@TimothyBlynJacobs Gave it a shot if you want to comment.

#13 @tfrommen
4 years ago

This is an interesting ticket, and it's also very similar to something I wanted to do for the longest time: have a "See in REST API" node in the admin bar that would link to the according REST API request both when editing a resource or when viewing it on the frontend. I imagined it making use of get_queried_object(_id)and conditionals, just like you, so having functions like rest_get_route_for_(post|term) would certainly help.

That said, here are some minor nitpicks on the last patch, @dshanske:

  • You might want to remove that error_log on L1849. ;)
  • In PHP, the instanceof keyword is all lowercase (L1839).
  • Using is_attachment is unnecessary as is_singular will cover that, too.

Also, why did you pass in application/json via esc_attr instead of just hard-coded right in the placeholder string (L1790)?

Looking forward to having this in Core. Thanks! :)

#14 @dshanske
4 years ago

The error_log is my fault...I forgot to take it out after using it to track something.

I am still not 100% sure that application/json is the right mime type, see above. So it's possible this might change...still, you are right, I could simply hardcode it.

I will update the patch...want to see if anyone has any other ideas.

#15 @TimothyBlynJacobs
4 years ago

This is looking great, thanks @dshanske!

I agree with @tfrommen on the patch feedback. I think we should also make sure rest_get_route_for_post and rest_get_route_for_term have the same signature, that is both only accept a WP_Post|WP_Term object.

For the post type controller, we can also use the get_rest_controller method which has some additional checks around the class.

For taxonomies, lets be safe and make sure the taxonomy has show_in_rest enabled.

#16 @TimothyBlynJacobs
4 years ago

To @tfrommen's idea, I think we should change have a rest_get_current_resource_link that would return the correct URL for the current object. And then use that function when calling rest_head_link in rest_add_resource_link. That way things are more reusable if someone wants to use that data in a different way, like a admin menu node.

@dshanske
4 years ago

#17 @dshanske
4 years ago

  • Keywords needs-patch removed

@TimothyBlynJacobs @tfrommen I tried to incorporate the suggestions made. In order to do so, I added a get_rest_controller to WP_Taxonomy to match the one introduced in 5.3 to WP_Post_Type, as I thought this was more consistent than the alternatives.

So, this has the advantage of improving the return. The get_rest_controller function returns the Post Controller if there is no controller explicitly but show_in_rest is set to true...which I'm mirroring now in the WP_Taxonomy class.

#18 @TimothyBlynJacobs
4 years ago

Could you convert this to a PR @dshanske?

#19 @dshanske
4 years ago

I usually do PRs as most of my contributions are to Github projects. I haven't done a PR to a WordPress trac ticket, but I'll figure it out.

This ticket was mentioned in PR #333 on WordPress/wordpress-develop by dshanske.


4 years ago
#20

  • Keywords has-patch added

Posts, terms, and other URLs on the site should have head links to the REST API representation of the data contained.

Trac ticket: https://core.trac.wordpress.org/ticket/49116

#21 @dshanske
4 years ago

  • Keywords has-patch removed

@TimothyBlynJacobs Switched it to a PR.

dshanske commented on PR #333:


4 years ago
#22

As you can see, I tested the original version, but did not test the changes here. I wanted to get some feedback on the design before I did some brief tests.

TimothyBJacobs commented on PR #333:


4 years ago
#23

We'll want test coverage on the WP_Taxonomy::get_rest_controller method. See WP_Test_REST_Posts_Controller for an example. We'll want to utilize it in create_initial_rest_routes as well.

TimothyBJacobs commented on PR #333:


4 years ago
#24

It looks like unit tests are now passing. There are a couple of linting errors: https://travis-ci.com/github/WordPress/wordpress-develop/jobs/348833828#L1725

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


4 years ago

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


4 years ago

dshanske commented on PR #333:


4 years ago
#27

I'm trying to see what is missing here. Been caught on other things for a few days.

TimothyBJacobs commented on PR #333:


4 years ago
#28

I think we have everything at this point. Going to push some minor changes then commit.

TimothyBJacobs commented on PR #333:


4 years ago
#29

I've made some coding standards changes, added some more tests, and combined the link output in the existing functions.

@tfrommen, @dshanske if you want to do some final look overs.

dshanske commented on PR #333:


4 years ago
#30

I think we're going to need a dev note on this in the 5.5 Field Guide.

#31 @TimothyBlynJacobs
4 years ago

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

In 48273:

REST API: Link to the REST route for the currently queried resource.

This allows for programatically determining the REST version of the current page. The links also aid human discovery of the REST API in general.

Props dshanske, tfrommen, TimothyBlynJacobs.
Fixes #49116.

TimothyBJacobs commented on PR #333:


4 years ago
#32

Yeah we'll need to mention it in the overall dev note for the REST API.

Note: See TracTickets for help on using tickets.