search

WP-API / menus-endpoints

Feature plugin for Menu Endpoints
43 stars 13 forks source link

Menu and menu item endpoints. #22

Closed spacedmonkey closed 5 years ago

spacedmonkey commented 5 years ago

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.

renatonascalves commented 5 years ago

Also, documentation could use some love.

spacedmonkey commented 5 years ago

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, get_item, delete methods, by basically copying and pasting massive amounts of code. This didn't make sense so I want went this approach.

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.

TimothyBJacobs commented 5 years ago

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 WP_REST_Posts_Controller ( and friends ).

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 Controller

The 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.

{
  "id": 179,
  "description": "",
  "name": "All Pages",
  "slug": "all-pages",
  "taxonomy": "nav_menu", // We shouldn't expose this.
  "meta": [],
  "_links": {
    "self": [
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menus\/179"
      }
    ],
    "collection": [
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menus"
      }
    ],
    "about": [ // The taxonomy shouldn't be exposed here either.
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/taxonomies\/nav_menu"
      }
    ],
    "wp:post_type": [ // It'd be better to have a more obvious link relation here. Like wp:items
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menu-items?menus=179"
      }
    ],
    "curies": [
      {
        "name": "wp",
        "href": "https:\/\/api.w.org\/{rel}",
        "templated": true
      }
    ]
  }
}

Menu Items Controller

Some 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.

{
  "id": 1799,
  "title": { // If a custom title isn't set, the title for the menu item appears as blank
    "raw": "",
    "rendered": ""
  },
  "original_title": "Page Image Alignment",
  "status": "publish", // What does status mean for a menu item?
  "url": "http:\/\/wp-api.test\/about\/page-image-alignment\/",
  "attr_title": "",
  "description": "",
  "type": "post_type",
  "type_label": "Page", 
  "object": "page",
  "object_id": 1133,
 // I personally found parent and menu_item_parent a bit confusing. I think it makes more sense if the parent refers to the parent menu item. Discovering the object's original parent can be found off a link to the menu item's object.
  "parent": 1725,
  "menu_item_parent": 1770,
  "menu_order": 5,
  "target": "",
  "classes": [
    ""
  ],
  "xfn": [
    ""
  ],
  "meta": [],
  "_links": {
    "self": [
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menu-items\/1799"
      }
    ],
    "collection": [
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menu-items"
      }
    ],
    "about": [ // I'm not sure how much sense it makes to include this link as a description of the menu item controller.
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/types\/nav_menu_item"
      }
    ],
    "wp:term": [ // What would the use cases be for this link?
      {
        "taxonomy": "nav_menu",
        "embeddable": true,
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menus?post=1799"
      }
    ],
   // I think most if not all of the permission links don't really make sense in this context.
    "wp:action-publish": [
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menu-items\/1799"
      }
    ],
    "wp:action-unfiltered-html": [
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menu-items\/1799"
      }
    ],
    "wp:action-create-menus": [
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menu-items\/1799"
      }
    ],
    "wp:action-assign-menus": [
      {
        "href": "http:\/\/wp-api.test\/wp-json\/wp\/v2\/menu-items\/1799"
      }
    ],
    "curies": [
      {
        "name": "wp",
        "href": "https:\/\/api.w.org\/{rel}",
        "templated": true
      }
    ]
  }
}

From the collection params:

Additional Ergonomic Issues

What 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 batch controller at the moment, but even if we did, we'd still have the overhead of dispatching internal requests for each menu item.

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. /wp/v2/menus/{menu_id}/items/{item_id} would be more intuitive. Can a menu item belong to more than one menu? Can you have orphaned menu items? The current URL setup and linking somewhat implies that you can.

So the first version of this PR did have their own controller, but I have myself re-implementing, get_item, delete methods, by basically copying and pasting massive amounts of code. This didn't make sense so I want went this approach.

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.

spacedmonkey commented 5 years ago

@TimothyBJacobs I have make tweaks based on comments. I think we are looking really good. I think we are nearly there with a version 1 merge.

I have also made another PR for menu location #24 . Treating this as something different.

TimothyBJacobs commented 5 years ago

Agreed. I think this is sufficient to merge and iterate on.