Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts giving a quick look.
Also, documentation could use some love. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a bit of inline feedback, there are some other things as well that I need to think more about. Particularly the structure of the menu item endpoint.
This requires overriding a substantial amount of the terms and posts controllers. What would this look like as dedicated controllers? I'm worried about the breadth of the API that we are exposing. We also end up exposing quite a bit of the implementation details of the storage system which we generally try to abstract in the REST API.
Thanks for your feedback so far @TimothyBJacobs So the first version of this PR did have their own controller, but I have myself re-implementing, As for abstraction, the nav terms, is pretty simple, so make sense to keep them as they with the controller. All you can do is a create / edit / delete the term, so the term controller worked fine for that. As for the menu item controller, I don't feel like it is clear from the outside it is a post. Most of the fields don't map and unless you look into core you wouldn't know. I do not want to abstract here, for the sake of it. There has to be a compelling reason to do so, which I haven't seen yet. This implementation works and works well, why fight the fact that these are terms / post under the hood. In the case, it is a benefit. |
To be clear. I'm not advocating another layer of abstraction. The REST API controller itself is the abstraction I'm talking about. One of the design goals of the REST API is to provide an abstraction over the inconsistent WordPress internals. This provides an interface that it easy for developers to learn without having to understand the internals of how WordPress works. Importantly, we can always add additional features to the routes at a future point. Removing features is essentially impossible. So once we commit to having a large API surface, we'll be stuck maintaining it. This isn't just the "public" API either ( ie the request and response format ) it's also the extension points of the I think there is also at least some interest in looking at alternate forms of menu storage. Some extended feedback from looking at the response and request bodies. Menus ControllerThe menus controller is probably the easiest one to adjust because the terms controller itself is quite simple. However, there are a number of things exposed that don't really make sense.
Menu Items ControllerSome of these things are bugs, but fixing them could get quite messy working within the existing Posts controller. Particularly when trying to make sure the filter and extension system works as expected for plugin developers.
From the collection params:
Additional Ergonomic IssuesWhat does it look like when a menu is reordered? It seems like this would require HTTP requests to each menu item to complete. We don't have a I think menu settings could cleanly be implemented into the menu controller. Locations could as well, perhaps with the different locations defined by the Schema. The URL structure is a bit confusing as well.
I agree wholesale copying would not be ideal. I think exploring extracting out the posts controller into more manageable bits would help alleviate this issue. As it is, the current controllers aren't really friendly to customization w/o copy and pasting and making changes. Perhaps traits can be explored here. I'm not a huge fan of them for various reasons, but I think in a WordPress context it makes a lot of sense. It'd allow us to introduce helpers with minimal BC impact and are fairly friendly to use. |
@TimothyBJacobs I have make tweaks based on comments. I think we are looking really good. I have also made another PR for menu location #24 . Treating this as something different. |
Agreed. I think this is sufficient to merge and iterate on. |
First pass of menu / menu item endpoints. After #21 , it was discussed that we need something a little more simple as a first pass at the PR.
Props to @wpscholar as I used this PR as inspiration.