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

/widgets/:id or /widgets/:id_base? #4

Open
NateWr opened this issue Jan 30, 2016 · 4 comments
Open

/widgets/:id or /widgets/:id_base? #4

NateWr opened this issue Jan 30, 2016 · 4 comments

Comments

@NateWr
Copy link
Contributor

NateWr commented Jan 30, 2016

The get_widget_endpoint branch implements (mostly) the base /widgets endpoint.

I was surprised to see the next route was /widgets/:id_base. I would expect that the endpoint would work like posts, where you retrieved a specific widget with /widgets/:id. Wouldn't that make more sense? We could implement id_base as a filter on the /widgets base just like sidebar.

(I appreciate that I wrote the initial endpoint registration. Just want to double-check that there wasn't some decisions about /widgets/:id_base that I can't remember).

@westonruter
Copy link
Member

The /widgets/:id_base request is another way to say /widgets/:type, and it would return all widgets of a given type. Widget IDs are different than post IDs in that the numeric part is actually just an array index for an array of instances of a given type. In other words, two different widgets can have the same numeric part, e.g. search-2 and text-2. So we can't identify a widget simply by the 2. In some ways, this situation is similar to how terms IDs were ambiguous prior to the most recent releases. So this is why I suggest /widgets/:id_base/:number to address a single widget, and /widgets/:id_base to reference the collection of widgets of a given type (which could also be referenced via /widgets?filter[type]=:id_base).

Ideally widgets would be eliminated from being stored in options altogether and they would have a full-on widget_instance post type (as implemented in feature plugin). When this happens, widget instances could indeed be referenced by a single numeric ID since this would be a globally auto-incremented post ID value, such as /widgets/123.

@NateWr
Copy link
Contributor Author

NateWr commented Jan 30, 2016

I thought that we would be "short-circuiting" this issue by using [id_base]-[array_index_key] as the id. I guess I should have used /widgets/:slug to describe what I intended, because each instance has a unique slug (eg - '/widgets/recent-posts-2`).

I am struggling to find the use-case for the /widgets/:typeendpoint, as I don't know when or why I would want a collection of a specific type. I understand that's how the instances are stored in the database. But it's not how the data is used anywhere in WordPress, as far as I am aware.

It seems more logical to me that :type would be a filter I could pass to the /widgets endpoint rather than an endpoint on it's own. And being able to query a single item with /widgets/<slug> seems closer to the pattern followed by other endpoints.

With the current patter, /widgets/:id_base/:number, I'd have to split the widget id string in the client before making the request, effectively making the id not usable in the client without some extra work.

Edit - sorry hit enter before finishing. Anyway, I will go ahead and issue a PR for the get_widget_endpoint branch for now. It implements the base /widgets and you'll see using the [id_base]-[array_index_key] as a slug seems like a sensible approach. But whatever is decided is fine. I just thought it seemed a bit at odds to me to have a /widgets/type/number endpoint.

@westonruter
Copy link
Member

@NateWr yeah, I think you're right. We don't need /widgets/:type after all, especially if #35669 gets implemented and widgets get stored in custom post type instead of options. Once that happens, a widget instances would be addressable by a single integer ID just like any other post, and /widgets/:id would work. In the mean time, we could still implement this but the format of :id would allow for a widget ID as it exists currently in Core, as id_base followed by number (/^(?P<id_base>.+)-(?P<number>\d+)/$). If the widget_instance post is added to Core, then this pattern can be changed to include the integer post ID: (/^(?:(?P<post_id>\d+)|(?P<id_base>.+)-(?P<number>\d+))/$). When a non-post_id request is made, the old widget ID can be looked up by post_name, assuming that this is where the old widget IDs get stored.

In both cases, listing all widgets of a given type can be done via /widgets?filter[type]=:id_base instead of via the /widgets/:type which I initially proposed.

👍

@NateWr
Copy link
Contributor Author

NateWr commented Feb 1, 2016

I think I've set this up as you suggest now. See WP-API/menus-endpoints#17

@kadamwhite kadamwhite transferred this issue from WP-API/menus-endpoints Aug 14, 2019
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

No branches or pull requests

2 participants