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

Fix post-format serialization due to invalid REST API response for id prop #31380

Closed
getdave opened this issue Apr 30, 2021 · 7 comments
Closed
Assignees
Labels
[Block] Navigation Affects the Navigation Block REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended

Comments

@getdave
Copy link
Contributor

getdave commented Apr 30, 2021

As part of #31004 it was discovered that adding a Post Format type link variation in the new Navigation editor screen (only) does not work - the item is saved with the wrong type or the request does not succeed.

Please note: the error is only manifesting when you create the Post Format core/navigation-link block's post_type variation in the Navigation Editor screen.

The reason for this is that when the block is serialized into a nav_menu_item for storage its object_id is not set to an integer. Rather instead it is set to the slug which represents the Post Format (eg: video, quote, standard....etc)

{
    // ...more menu item props here
    object_id: 'video', // this should be an ID reference to the post format object ID
    object: 'post_format'
}

This is incorrect. It should be an integer ID representing the ID of the post-format.

{
    // ...more menu item props here
    object_id: 123, // example: references a known post format
    object: 'post_format'
}

Where does the incorrect ID come from?

This error is actually coming from the REST API request to /v2/search itself which is fired when the user performs a search in the link UI to select a Post Format.

Try this in your browser:

http://localhost:8888/index.php?rest_route=%2Fwp%2Fv2%2Fsearch&search=&per_page=3&type=post-format&_locale=user

If you have Post Formats created (see below for how to do this) you will see something akin to the following in the response:

[
    {
        id: "image", // wrong - should be the ID of the post format
        title: "Image",
        url: "http://localhost:8888/?post_format=image",
        type: "post-format",
    },
    {
        id: "video", // wrong - should be the ID of the post format
        title: "Video",
        url: "http://localhost:8888/?post_format=video",
        type: "post-format",
    }
]

This id prop is then used throughout the block UI as an attribute and therefore when the Navigation Editor tries to serialize the blocks to menu items in order to be saved the object_id is set to the incorrect id prop resulting in the error.

Proposed solution

This looks like a REST API issue. However, it may be completely legitimate for the endpoint to return this data type for the ID property as the schema allows for integer or string.

Options:

  1. Fix endpoint to turn the ID of the post format.
  2. Register a new field for this endpoint for post formats only which provides a object_id prop for the post formats only. We would then have to map this property to id before using it in the editor.

As a first step I would like to discuss with @TimothyBJacobs and the REST API team to get their opinion.

Aside: creating some Post Formats

The best way to do this is:

  • Activate the Twenty_Thirteen_ (2013) Theme (or any Theme which supports Post Formats).
  • Create a Post.
  • In the right hand sidebar select a Post Format.
  • Publish your Post.

Originally posted by @gwwar in #31004 (comment)

@getdave getdave added [Block] Navigation Affects the Navigation Block [Package] Edit Navigation [Type] Bug An existing feature does not function as intended REST API Interaction Related to REST API labels Apr 30, 2021
@getdave getdave self-assigned this Apr 30, 2021
@getdave getdave changed the title Fix post-format serialization Fix post-format serialization due to invalid REST API response for id prop Apr 30, 2021
@TimothyBJacobs
Copy link
Member

This is an intentional design decision. Post formats do not have an id as part of their public API. Everything is identified by their slug.

@getdave
Copy link
Contributor Author

getdave commented May 4, 2021

@TimothyBJacobs Thanks for advising on this. I'm going to add some more context from our Slack convo:

it [post-formats] is backed by a taxonomy, but the taxonomy’s term id is really an implementation detail. It isn’t part of the API.

When the post formats search handler was first introduced, it created its own id of an incrementing integer for each post format because the search handler would discard non-numeric results.

@kathrynwp
Copy link

@getdave With the focus on the Navigation block in WP, returning to this to see if there might be any potential to get something moving soon on this issue, or not. Thanks!

@getdave
Copy link
Contributor Author

getdave commented Oct 4, 2022

@kathrynwp Thanks for flagging. Probably a relatively low priority item but nonetheless one we should be able to solve.

@TimothyBJacobs I think you've said before that you wouldn't be happy exposing the taxonomy's term ID as that's an implementation detail.

If that's still the case then we could look at manually providing a hardcoded ID for each Post Format on the client side. It seems like there's a limited set of formats so this could just be a map of Post Format slug to hardcoded ID. However it's non-optimal.

@TimothyBJacobs
Copy link
Member

Can the client be updated to accept a string or int for the object_id attribute instead?

@getdave
Copy link
Contributor Author

getdave commented Oct 11, 2022

The next step here is actually to work out whether this issue is currently manifesting itself in the Navigation block. If it's not then I think we have more pressing problems to deal with that will have a bigger impact on user experience.

Not saying it's not important but perhaps a lower priority. I'll make a note to see if I can replicate the problem. If not then I say we archive this Issue and it can be picked up in the future if/when this becomes a problem.

@Mamaduka
Copy link
Member

Closing the issue since the editor is no longer maintained and has been removed from the project.

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 REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants