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

Update REST API to handle slug refs #44975

Open
wants to merge 3 commits into
base: try/nav-block-use-slugs-for-ref
Choose a base branch
from

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Oct 14, 2022

What?

Sub PR of #42809 containing changes to REST API needed to handle slugs as references for Navigation posts.

Why?

Foundational work for using slugs as references instead of IDs in the Nav block.

How?

Updates the REST API with a new endpoint which handles string-based slugs specifically and then routes that along the same handlers established for int based IDs.

Also adds test coverage for the endpoint.

Testing Instructions

  • Check backwards compat - Nav block should still find menus by ID. Just create a Nav block with a menu and save then ensure that you can reload the page and the menu is found.
  • Run the tests:
npm run test:unit:php /var/www/html/wp-content/plugins/gutenberg/phpunit/class-gutenberg-rest-navigation-controller-test.php

Screenshots or screencast

@getdave getdave self-assigned this Oct 14, 2022
@getdave getdave added the [Block] Navigation Affects the Navigation Block label Oct 14, 2022
// Lists a single nav item based on the given id or slug.
register_rest_route(
$this->namespace,
'/' . $this->rest_base . '/((?P<id>[\d]+)|(?P<slug>[\w\-]+))',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both id and slug are needed in this request otherwise backwards compatibility is broken.

@getdave getdave marked this pull request as ready for review October 14, 2022 11:18
@getdave getdave requested a review from spacedmonkey as a code owner October 14, 2022 11:18
* See: https://github.com/WordPress/gutenberg/pull/43703.
*
* @param string $id the slug of the Navigation post.
* @return WP_Post|null
Copy link
Member

Choose a reason for hiding this comment

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

Wrong return type.

@spacedmonkey
Copy link
Member

Why can you using existing endpoints, like this?

https://spacedmonkey.com/wp-json/wp/v2/posts?slug=proposal-add-a-dominant-color-background-to-images

@getdave
Copy link
Contributor Author

getdave commented Oct 14, 2022

Why can you using existing endpoints, like this?

https://spacedmonkey.com/wp-json/wp/v2/posts?slug=proposal-add-a-dominant-color-background-to-images

I feel like I tried that (it was a while ago now). I will double check that in the main PR just in case before we look to merge this one.

@spacedmonkey
Copy link
Member

@getdave Works fine in my testing.

https://spacedmonkey.com/wp-json/wp/v2/navigation?slug=navigation-2

The same of the response is different. Any array with a single object over one object. But it works just fine.

@getdave
Copy link
Contributor Author

getdave commented Oct 17, 2022

The slug query returns a collection whereas an ID query returns a single post

https://spacedmonkey.com/wp-json/wp/v2/navigation/434

Gutenberg's Core data has:

  • getEntityRecord - get a single post by recordKey. This is typically an ID but can be anything as long as the API endpoint accepts it (e.g. https://spacedmonkey.com/wp-json/wp/v2/navigation/{my-slug-here}).
  • getEntityRecords - get a collection of posts. Can provide a query param to pass a query to the endpoint.

Conceptually what the block is trying to do is get the individual Navigation post assigned to it by a specific reference. This is different to fetching a collection of Navigation posts based on some query parameters.

This is why we went down the route of changing the endpoint to accept slug as the "reference". This also brings it more into line with the way the Template Parts endpoint works. For example - if you want a single template part you pass a slug as a reference as you get a single record back.

I'm diving back through the parent PR again because I"m sure I explored the route you are suggesting in more detail and it was decided against but I've not found the original conversation (yet).

@draganescu
Copy link
Contributor

I think the way the REST API works is correct: the only call to return a single item is when we query by key. In general the key in WP is some kind of ID. The slug is a general query that can return a collection of one item if the slug is unique. With the caveat that I am not deep into the effort here yet, I think the client data library should work with this not the other way around.

@spacedmonkey
Copy link
Member

The logic in javascript should be simple.

if is int
wp/v2/navigation/?include=234&per_page=1
else
wp/v2/navigation/?slug=slug&per_page=1

Then just check the response is empty and get the 0 value of response.

No need for a new endpoint.

@getdave
Copy link
Contributor Author

getdave commented Oct 17, 2022

I think the client data library should work with this not the other way around.

This needs to be explored in more detail but in principle I agree.

@spacedmonkey and I discussed this in DMs on WP Slack and we came to the conclusion that

  1. I should (re)explore the idea of fetching the 0th item from a collection query.
  2. Determine whether the Gutenberg Data Layer needs to adapt to handle this situation.

I'll report back with another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants