WP-API / menus-endpoints

Feature plugin for Menu Endpoints
43 stars 13 forks source link

Update endpoint structure #17

Closed NateWr closed 5 years ago

NateWr commented 8 years ago

Address #11 and #12.

westonruter commented 8 years ago

I'm finally circling back around on this.

Given the dependencies on having JSON schema for instances (#35574) and for iterating on widgets to allow a fully JS-driven UI (#33507), I've started a separate feature JS Widgets plugin to implement and experiment with these changes.

Here's a REST API request for a single widget instance:

http://src.wordpress-develop.dev/wp-json/js-widgets/v1/widgets/text/10?context=edit
{
  "widget_number": 10,
  "id_base": "text",
  "raw": {
    "title": "Lorem Ipsum",
    "text": "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam pulvinar, erat nec facilisis blandit, lectus metus facilisis felis, ut euismod diam diam et nisi.",
    "filter": false
  },
  "rendered": "<li id=\"text-10\" class=\"widget widget_text\"><h2 class=\"widgettitle\">Lorem Ipsum</h2>\n\t\t\t<div class=\"textwidget\">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam pulvinar, erat nec facilisis blandit, lectus metus facilisis felis, ut euismod diam diam et nisi.</div>\n\t\t</li>\n",
  "_links": {
    "self": [
      {
        "href": "http://src.wordpress-develop.dev/wp-json/js-widgets/v1/widgets/text/10"
      }
    ],
    "collection": [
      {
        "href": "http://src.wordpress-develop.dev/wp-json/js-widgets/v1/widgets/text"
      }
    ]
  }
}

The raw is present when the edit context is requested. The rendered is bit is obviously the output of the widget() method call, and it's something I'm experimenting with. For rendered it to really be flexible, the endpoint request should accept a request_uri and sidebar_id param so that the rendered output would be able to have the right page and sidebar context. Nevertheless, the plugin also supports the widget renderer to return a data array instead of echoing markup directly. This would then allow for the rendered property to include the dataset needed to render the widget on the frontend using JS templates.

Aside: Take a look at the impacts for accessing and manipulating the widget instances in the Customizer. It's now straight JSON without any serialized/encoded data which were required for back-compat with PHP-driven widgets. Compare old structure for representing a widget instance with the new structure in JS Widgets:

customize__wordpress_develop_ _just_another_wordpress_site_
joehoyle commented 8 years ago

Nice! Liking the look of this. The raw / rendered pair is maybe not the best naming due to title.raw and title.rendered that we have on posts which means something a little different (pre filter and post filtered, essentially). I'd lose the raw top level altogether and changed rendered to html (perhaps), looking something like:

{
  "widget_number": 10,
  "id_base": "text",
  "title": "Lorem Ipsum",
  "text": "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam pulvinar, erat nec facilisis blandit, lectus metus facilisis felis, ut euismod diam diam et nisi.",
  "filter": false,
  "html": "<li id=\"text-10\" class=\"widget widget_text\"><h2 class=\"widgettitle\">Lorem Ipsum</h2>\n\t\t\t<div class=\"textwidget\">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam pulvinar, erat nec facilisis blandit, lectus metus facilisis felis, ut euismod diam diam et nisi.</div>\n\t\t</li>\n",
  "_links": {
    "self": [
      {
        "href": "http://src.wordpress-develop.dev/wp-json/js-widgets/v1/widgets/text/10"
      }
    ],
    "collection": [
      {
        "href": "http://src.wordpress-develop.dev/wp-json/js-widgets/v1/widgets/text"
      }
    ]
  }
}

Also, not sure if I understand what id_base means? is that the "type" of widget? if so, I thank could be named better, I'm guessing also widget_number is the instance number of that "type" of widget? IMO one of these things needs to be called id, it would seem that is widget_number if we are treating each widget "type" as it's own distinct object type. If on the other hand IDs can span types, maybe we have the id field as $type::$number or something.

westonruter commented 8 years ago

@joehoyle thanks!

The raw / rendered pair is maybe not the best naming due to title.raw and title.rendered that we have on posts which means something a little different (pre filter and post filtered, essentially).

My thought on the use of raw vs rendered here is that the raw is what is edited (in edit context) wheres the rendered is what is actually sent to the client (view and embed context). The raw may have sensitive information (such as a secret URL or API key) wheres the rendered would have this information processed. In terms of widgets being used as the backend for JS frontend components (using frontend templating), the raw/rendered pair makes more sense, as the raw instance data becomes transformed to rendered data for rendering. For example, consider a Recent Posts widget that has title and count properties in the raw object, where the rendered object could be the same but with filters applied on title and a new property posts that contains the post objects for the template to render.

Anyway, that's the thought process behind that. It's perhaps poor form to overload the one rendered property to be able to serve back either an HTML string or an object.

I'd lose the raw top level altogether.

Having the instance data stored as as sub-properties is necessary in order to handle cases where a widget may want to use property names that conflict with root level properties, in my example widget_number, id_base, and rendered, or even _links.

Also, not sure if I understand what id_base means? is that the "type" of widget? if so, I thank could be named better. I'm guessing also widget_number is the instance number of that "type" of widget? IMO one of these things needs to be called id, it would seem that is widget_number if we are treating each widget "type" as it's own distinct object type. If on the other hand IDs can span types, maybe we have the id field as $type::$number or something.

Yes. This is re-using some of the naming for widgets in Core, where a widget ID looks like text-123, consisting of the id_base (here text) and the widget number (here 123). It's similar to the taxonomy-term addressing that was required before term splits. Since the REST API generally is trying to jettison legacy names (e.g. use content instead of post_content), I agree that type is better than id_base, as that's what it means anyway. The widget number is more tricky since widgets of different types can have the same widget number, since it is just an array index. We could be a bit progressive and just show the widget number as an id, anticipating that widgets will eventually be stored as posts. Otherwise, the id would need to be a string/number compound like text-123.

joehoyle commented 8 years ago

My thought on the use of raw vs rendered here is that the raw is what is edited (in edit context) wheres the rendered is what is actually sent to the client (view and embed context). The raw may have sensitive information (such as a secret URL or API key) ......

Ok, I understand that, however the main thing for me is that it's consistent with other objects in the rest api, and in posts it's title: { raw: string, rendered: string }, so I'd be down if we had an object-per-key, however I don't think this will satisfy your argument, as that means that 1: you can't do the fallback to rendered html like you are, and 2: it is subject to the top level naming issue.

Right now, I don't think the rendered should mean both "all the widget html" and "a value for display purposes". The full rendering of the widget HTML is going to want to be included for both widgets that send data for the REST API and ones that don't, so it should be a permanent part of the response, along with the "schema" additional fields that a Modern Nice Widget will be defining for use in the REST API.

For the naming conflict with the top level keys, I don't see that as an issue, this is already the case elsewhere with defining additional fields for posts and things like that. I'm primarily concerned for the client using the API, and to them there's no distinction between type and title so it doesn't make sense to put all those "developer defined" fields under some kind of data or fields key (raw in your proposal). IMO the fields are first-class properties of the Widget object in the API, and therefore should be exposes as such.

danielbachhuber commented 8 years ago

Crazy idea: What if the rendered HTML always came through on the sidebar resource, and never on the widget resource? Sidebars have no database interaction, and thus aren't editable.

westonruter commented 8 years ago

@joehoyle:

Ok, I understand that, however the main thing for me is that it's consistent with other objects in the rest api, and in posts it's title: { raw: string, rendered: string }, so I'd be down if we had an object-per-key, however I don't think this will satisfy your argument, as that means that 1: you can't do the fallback to rendered html like you are, and 2: it is subject to the top level naming issue.

So instead of raw how about content to store the instance data, as was originally proposed above: https://github.com/WP-API/wp-api-menus-widgets-endpoints/pull/17#discussion_r51599728

Right now, I don't think the rendered should mean both "all the widget html" and "a value for display purposes". The full rendering of the widget HTML is going to want to be included for both widgets that send data for the REST API and ones that don't, so it should be a permanent part of the response, along with the "schema" additional fields that a Modern Nice Widget will be defining for use in the REST API.

Yes, agreed. The server-side rendered HTML for a widget and the exported data for front-end rendering should be separate fields.

For the naming conflict with the top level keys, I don't see that as an issue, this is already the case elsewhere with defining additional fields for posts and things like that. I'm primarily concerned for the client using the API, and to them there's no distinction between type and title so it doesn't make sense to put all those "developer defined" fields under some kind of data or fields key (raw in your proposal). IMO the fields are first-class properties of the Widget object in the API, and therefore should be exposes as such.

OK, I think I agree with you. In terms of catching widgets that define instance properties that conflict with widget resource properties, the widget could raise an error if id, type, or _links are defined in the widget instance schema (\WP_JS_Widget::get_instance_schema()).

So in the case of a Nice Modern Widget (aka JS Widget) which wants to output processed data for JS components to make us of on the frontend: you're suggesting that it should process the raw instance data and amend the returned REST resource with additional dynamic readonly properties for the frontend?

As an example to talk this through: consider a widget that lists entries from an external protected feed. Let's say the widget has has instance properties title, feed_url, and api_key. Only the title is public, where feed_url and api_key should only be displayed in a edit context. When the widget instance is prepare_item_for_response'ed, external data could be fetched and amended to the resource (e.g. add_additional_fields_to_object) as a dynamic readonly feed_items property, while the feed_url and api_key would be filtered out of the response.

Does this align with you are thinking?

westonruter commented 8 years ago

@danielbachhuber:

Crazy idea: What if the rendered HTML always came through on the sidebar resource, and never on the widget resource? Sidebars have no database interaction, and thus aren't editable.

That's a good point. A widget doesn't have any intrinsic knowledge of the sidebar args so that's why sidebar_id would have to be supplied to render a widget on a widget resource. If the sidebar was the resource instead, it would have the sidebar args available but then the widget to render would need to be passed as an argument. Or if the widget_id arg is omitted (or widget_ids arg), then it could render out all of the widgets assigned to that sidebar in their assigned order. Then on the widget resource it could include _links for the sidebar resources to render the widget as HTML. You're right about the sidebar resource being read-only, and likewise the widgets that are rendered as HTML off of this resource should likewise be readonly. So having another sidebar resource makes sense.

Does this sound right?

danielbachhuber commented 8 years ago

If the sidebar was the resource instead, it would have the sidebar args available but then the widget to render would need to be passed as an argument.

I'd actually think we should have two resources:

A sidebar would have a widgets attribute with an array of widget ids, and a rendered attribute to display the rendered HTML.

A widget resource would represent solely the pure underlying data — and shouldn't be exposed in an unauthorized request.

westonruter commented 8 years ago

@danielbachhuber yes, I also meant that there should be two resources: Sidebar and Widget. There could be routes like:

/widgetscollection of all widget types registered, linking to their respective collections.
/widgets/textcollection of all widget resources for the text type.
/widgets/text/123a single widget resource, a widget could define public properties and dynamic readonly fields that would be consumed by a JS front-end component. The _links could include references to the sidebar resource(s) that a widget is assigned to, along with a link to the rendering of the single widget the sidebar (see below).
/sidebarsa collection listing the registered sidebars
/sidebars/sidebar-1collection all widgets rendered as HTML, with _links for widget resources for data.

Containing a `widgets` property as an ordered array of widget IDs. This resource can be edited.
/sidebars/sidebar-1/widgetsa collection of all listing of all rendered widgets HTML, with _links for widget resources for data.
/sidebars/sidebar-1/widgets/text-123the one text widget rendered into this sidebar, again with _links containing a link to the widget resource.

(The text/123 route vs the text-123 widget ID is a bit messy.)

One thing not addressed here is that a sidebar is not necessarily read-only in WordPress, depending on how you look at the sidebars_widgets option. A sidebar resource could be modeled to contain an ordered collection of widget resources that it contains, and in this case, it would not be read only. Otherwise, the widget/sidebar relationship could be modeled on the widget resource itself, but I don't know if this is ideal either because sidebars change depending on the theme being active. If it was on the widget resource, then the associated sidebar(s) and the widget's position in each would need to be stored, similar to how nav menu items are stored internally.

westonruter commented 8 years ago

I missed this line in your comment:

A sidebar would have a widgets attribute with an array of widget ids, and a rendered attribute to display the rendered HTML.

But I think I said mostly the same thing, although I suppose I introduced a different resource type of rendered_widget which is what you'd get a collection of at /sidebars/sidebar-1/widgets and a single resource of at /sidebars/sidebar-1/widgets/text-123. I wasn't clear on how the list of widgets would be updated for a given sidebar, but a widgets attribute on a sidebar containing a list of widget IDs would certainly work.

danielbachhuber commented 8 years ago

One thing not addressed here is that a sidebar is not necessarily read-only in WordPress, depending on how you look at the sidebars_widgets option. A sidebar resource could be modeled to contain an ordered collection of widget resources that it contains, and in this case, it would not be read only.

Oh, right. This is correct. The Sidebar resource should have a widgets attribute, which would be an array of the Widget ids.

Generally, it would be useful to use API Blueprint or similar to produce a design document for these resources, so discussion isn't yet closely-coupled to the code itself.

But I think I said mostly the same thing, although I suppose I introduced a different resource type of rendered_widget which is what you'd get a collection of at /sidebars/sidebar-1/widgets and a single resource of at /sidebars/sidebar-1/widgets/text-123.

We originally ventured down a similar path with Categories and Tags of Posts, and ended up going with categories and tags as arrays of ids on a post https://github.com/WP-API/WP-API/issues/2032

The raw / rendered pair is maybe not the best naming due to title.raw and title.rendered that we have on posts which means something a little different (pre filter and post filtered, essentially)

Going back to this comment, I actually now regret title, guid and others as objects on the resource. It makes many aspects of working with the data more complex (e.g. schema validation needs to traverse into the object) for no advantages that I'm aware of.

If it's not too late to change it, I think Resources should be kept as flat as possible.

westonruter commented 8 years ago

If a Text widget has an id_base of text according to the WP_Widget, and this is shown in the endpoint as /widgets/text, how should this relate to the type returned in the actual widget resource? I'm wary of there being conflicts with other registered REST resources. Should the type (and the schema title) get prefixed with widget_ (e.g. widget_text) to avoid naming collisions?

danielbachhuber commented 8 years ago

Should the type (and the schema title) get prefixed with widget_ (e.g. widget_text) to avoid naming collisions?

Yep, could and probably should, although I'd call it text-widget to make it more human-readable.

Also, you should read and weigh in on https://make.wordpress.org/core/2016/05/03/a-data-schema-for-meta/

westonruter commented 8 years ago

I changed the type and added support for a widget to define rendered readonly rest fields:

{
  "id": "text-10",
  "type": "text-widget",
  "title": "Hello... world!",
  "text": "Paraphraph one\n\nparagraph two\n\n\"paragraph\" three!",
  "filter": true,
  "title_rendered": "Hello&#8230; world!",
  "text_rendered": "<p>Paraphraph one</p>\n<p>paragraph two</p>\n<p>\"paragraph\" three!</p>\n",
  "_links": {
    "self": [
      {
        "href": "http://src.wordpress-develop.dev/wp-json/js-widgets/v1/widgets/text/10"
      }
    ],
    "collection": [
      {
        "href": "http://src.wordpress-develop.dev/wp-json/js-widgets/v1/widgets/text"
      }
    ]
  }
}
kadamwhite commented 5 years ago

Per #35 we have separated widgets code out into a new repository wp-api/widgets-endpoints; there's been some great discussion above, so if anybody involved wishes to pursue this work further I'd encourage you to re-open this on the new repository. I do caveat that suggestion with the warning that based on recent developments in Gutenberg, the widgets piece of this work may never land in core, so it may not be the most productive project to work on. Thank you all for your contributions nevertheless!