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

Menu and menu item endpoints. #22

Merged
merged 14 commits into from Jun 17, 2019

Conversation

@spacedmonkey
Copy link
Collaborator

commented Jun 7, 2019

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
Copy link

left a comment

My thoughts giving a quick look.

Show resolved Hide resolved lib/class-wp-rest-menu-items-controller.php
Show resolved Hide resolved lib/class-wp-rest-menu-items-controller.php
Show resolved Hide resolved lib/class-wp-rest-menu-items-controller.php Outdated
Show resolved Hide resolved lib/class-wp-rest-menu-items-controller.php
Show resolved Hide resolved lib/class-wp-rest-menus-controller.php Outdated
@renatonascalves

This comment has been minimized.

Copy link

commented Jun 11, 2019

Also, documentation could use some love.

spacedmonkey added some commits Jun 11, 2019

@TimothyBJacobs
Copy link
Member

left a comment

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.

Show resolved Hide resolved lib/class-wp-rest-menus-controller.php Outdated
Show resolved Hide resolved lib/class-wp-rest-menus-controller.php
Show resolved Hide resolved lib/class-wp-rest-menu-items-controller.php Outdated
Show resolved Hide resolved lib/class-wp-rest-menu-items-controller.php Outdated
Show resolved Hide resolved lib/class-wp-rest-menu-items-controller.php Outdated
Show resolved Hide resolved lib/class-wp-rest-menu-items-controller.php Outdated
@spacedmonkey

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

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:

  • Pagination. Does pagination make sense here? Especially with no upper-limit? I think I'd always want to be able to fetch all menu items for a menu.
  • before/after. Querying by publish date doesn't seem to make sense here.
  • menus/menus_exclude. These being arrays seems to imply that a menu item can belong to multiple menus.

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 added some commits Jun 13, 2019

@spacedmonkey

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

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

@spacedmonkey spacedmonkey merged commit d199fbe into WP-API:master Jun 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@spacedmonkey spacedmonkey deleted the spacedmonkey:v2 branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.