Make WordPress Core

Opened 9 years ago

Closed 2 months ago

#35574 closed enhancement (wontfix)

Add REST API JSON schema information to WP_Widget

Reported by: westonruter's profile westonruter Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.8
Component: Widgets Keywords: needs-patch needs-unit-tests
Focuses: rest-api Cc:

Description (last modified by westonruter)

With the REST API, there is an emerging-established way in WordPress for describing a data structure, such as a widget instance.

One aspect of the REST API endpoint schema is the default values for given fields on a property. Widgets often have duplicated logic between the WP_Widget::widget(), WP_Widget::update(), and WP_Widget::form() methods for checking if a given $instance property has been set, and if not, supplying a default value. In some cases, these isset checks are not performed resulting in PHP notices if the methods are programmatically invoked with an empty array. With JSON Schema defined, we would provide $instance defaults upon which the current stored $instance can be merged.

Widgets in WordPress are assumed to be arrays, with the applied filters and function return values. With a schema defined, the data type of a widget instance would be guaranteed. Meta in the REST API is a challenge given that it may or may not contain serialized PHP. Widgets are stored in serialized PHP arrays in WP options (though it is possible to store them in posts, per #32474). Additionally, the instance arrays may also contain PHP objects for classes that cannot be cleanly serialized into JSON, and having a REST API JSON schema defined for a widget would guarantee that a widget instance can be represented in JSON. This would, in turn, allow widgets to be exposed as endpoints in the REST API, and it would allow widget instances to be completely manipulated with JavaScript (such as in the Customizer, as described in #33507).

By adding schema information to WP_Widget, we get a lot more than having default $instance data available. For one, the widget form can be automatically generated based on the schema if one is not defined (i.e. if noform is returned from the WP_Widget::form() method). (This may actually be focus of the Fields API.)

Attachments (1)

35574.wip.diff (8.4 KB) - added by westonruter 9 years ago.
WIP

Download all attachments as: .zip

Change History (34)

@westonruter
9 years ago

WIP

#1 @westonruter
9 years ago

  • Keywords needs-patch dev-feedback needs-unit-tests added

35574.wip.diff is a first stab. If this ticket has support from other devs, we can flush it out.

#2 @sebastian.pisula
9 years ago

Oo good idea ;)

#3 @jdgrimes
9 years ago

+1. I do something like this already for widgets in one of my plugins.

#4 @boonebgorges
9 years ago

+1 to something like this. If this came up for @westonruter in the context of adding new widgets in the Customizer, then I have run into the very problem before :)

#5 @westonruter
9 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Version set to 2.8

#6 @westonruter
9 years ago

  • Description modified (diff)
  • Summary changed from Add default instance information to WP_Widget to Add schema information to WP_Widget

#7 @westonruter
9 years ago

  • Summary changed from Add schema information to WP_Widget to Add REST API JSON schema information to WP_Widget

#8 @westonruter
9 years ago

  • Description modified (diff)

#9 @mrahmadawais
9 years ago

+1 for this one.

#10 @westonruter
9 years ago

Proposal to deprecate old-style single widgets: #35656

This ticket was mentioned in Slack in #core by westonruter. View the logs.


9 years ago

#12 @sc0ttkclark
9 years ago

I think this would be a good place for the Fields API to step into, given we're working on a prototype of Widget forms powered by the Fields API for their configurations.

https://make.wordpress.org/core/2016/03/14/fields-api-where-were-at/#comment-29529

#13 @westonruter
9 years ago

  • Milestone changed from Future Release to 4.6

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


9 years ago

#15 @westonruter
9 years ago

Feature plugin where I'm prototyping the inclusion of an instance schema into the WP_Widget class and using it for REST API endpoints, widget instance validation, and enabling the raw JSON to be returned to the Customizer without the PHP-serialized base64-encoded data needed for back-compat for widgets that may store non-JSON-representable data in instances: https://github.com/xwp/wp-js-widgets

Last edited 9 years ago by westonruter (previous) (diff)

This ticket was mentioned in Slack in #core by westonruter. View the logs.


9 years ago

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


9 years ago

#19 @chriscct7
9 years ago

  • Milestone changed from 4.6 to Future Release

Punting to Future Release.

#20 @westonruter
8 years ago

As I just commented on #33507:

I just released JS Widgets v0.2.0 which includes JS-driven adaptations of all the core widgets (aside from Links), as well as a Post Collection bonus widget. Another standalone widget plugin called Next Recent Posts Widget takes the concept of JS widgets a step further and uses client-side JS templating for rendering the widget as well, implementing instant updates to changes. In this widget, selective refresh used to obtain rendered REST API data, while the raw value is used until the rendered data is returned. So this means that when entering -- into the widget's title will result in -- appearing in the rendered widget while waiting for the server to return with that wptexturize applies. In this way it behaves similarly to #33738.

#21 @westonruter
8 years ago

Call for feedback: Please see https://github.com/xwp/wp-core-media-widgets/issues/51 and provide feedback in terms of near-term merge of media widgets.

See method as defined in WP_Widget_Media: https://github.com/xwp/wp-core-media-widgets/blob/4445e0bf78ad008af79f962b874237c67ce93968/wp-includes/widgets/class-wp-widget-media.php#L91-L120

And in the WP_Widget_Image subclass: https://github.com/xwp/wp-core-media-widgets/blob/4445e0bf78ad008af79f962b874237c67ce93968/wp-includes/widgets/class-wp-widget-image.php#L47-L136

And then see how the widget can make use of it in the update method so that the image widget subclass doesn't need to define its own update method, since the parent class reads from the schema and does all of the logic: https://github.com/xwp/wp-core-media-widgets/blob/4445e0bf78ad008af79f962b874237c67ce93968/wp-includes/widgets/class-wp-widget-media.php#L181-L225

#22 @westonruter
8 years ago

  • Focuses rest-api added

This ticket was mentioned in Slack in #core-restapi by westonruter. View the logs.


8 years ago

#24 @westonruter
8 years ago

Humm, maybe we should rename a WP_Widget::get_instance_schema() method to WP_Widget::get_item_schema() for parity with WP_REST_Controller::get_item_schema().

The return value for this function is essentially the schema for the properties: https://github.com/xwp/wp-core-media-widgets/blob/994d40f25467591773e76d111bfcc806b351fd40/wp-includes/widgets/class-wp-widget-media.php#L91-L120

In that case perhaps it should be WP_Widget::get_properties_schema()?

Or then again, is this being short-sighted by not including the level above properties, namely:

'$schema' => 'http://json-schema.org/schema#',
'title' => $this->id_base . '-widget',
'type' => 'object',
'properties' => array( /* existing array returned by get_instance_schema */ )

I guess the idea here is that eventually when we have a full-on WP_REST_Widgets_Controller, that these root-level schema properties would be supplied there and the WP_Widget::get_instance_schema() (or however it is named) would essentially be serving the same purpose as \WP_REST_Controller::get_additional_fields().

Thoughts from REST API team?

Last edited 8 years ago by westonruter (previous) (diff)

This ticket was mentioned in Slack in #core-restapi by westonruter. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-restapi by westonruter. View the logs.


8 years ago

#27 follow-up: @rmccue
8 years ago

IMO, if it returns something similar but not identical, it shouldn't be called the same thing. If it's not a full schema object, it should definitely be called something different. Potentially, WP_Widget could have the get_item_schema which adds those properties automatically, with properties => $this->get_schema_for_properties()

Also, I haven't mentioned this here previously, but I have in Slack: I am -1 on reusing JSON Schema directly for internal usage. Using JSON Schema ties the internal behaviour of WordPress to a single serialisation formation (JSON) which doesn't have the same semantics as internal PHP types. I would rather we design our own type system which matches our requirements, which has JSON Schema as an output format.

Last edited 8 years ago by rmccue (previous) (diff)

#28 in reply to: ↑ 27 @westonruter
8 years ago

Replying to rmccue:

Also, I haven't mentioned this here previously, but I have in Slack: I am -1 on reusing JSON Schema directly for internal usage. Using JSON Schema ties the internal behaviour of WordPress to a single serialisation formation (JSON) which doesn't have the same semantics as internal PHP types. I would rather we design our own type system which matches our requirements, which has JSON Schema as an output format.

Quoting a couple messages from Slack to follow up on this:

From @joehoyle:

My thoughts on this: we already do this, so I think it's actually a good pattern to follow. Given that we want all of this to be used on the front-end too (and expose the schema there for potential rendering, then I don't think this is "native only" thing. If anything else, I just see "JSON Schema" to be a standardaized naming format (and we should probably stop calling is JSON Schema, as noted above, we have modifications)

And from me:

It makes sense to me. If the REST API provides a standard interface, and this standard interface speaks JSON as its transport protocol, then it makes sense to limit state data in PHP to being representable as JSON. Otherwise, what would have to be done is to have some kind of transformation layer built into the system so that the internal PHP value can be converted to JS and back again. Incidentally, the customizer actually implements the PHP-to-JS value translation in WP_Customize_Setting::js_value(), with the reverse (JS-to-PHP) being essentially handled by WP_Customize_Setting::sanitize(). It's somewhat awkward and is seldom used, though it did turn out to be essential to being able to export widget instances since they aren't currently guaranteed to be JSON-serializable since widgets have no schema, so the JS value of a widget instance is the PHP-serialized string with an HMAC to prevent tampering. If we can move away from having this transformation layer, and only pass around JsonSerializable values, then it would be much simpler to work with.

#29 @westonruter
8 years ago

The media widget is going ahead with WP_Widget_Media::get_instance_schema() for now. If we introduce a WP_Widget::get_schema_for_properties() method later on the base class, then we can deprecate WP_Widget_Media::get_instance_schema() and make it a wrapper for WP_Widget::get_schema_for_properties().

See https://github.com/xwp/wp-core-media-widgets/pull/70

#30 follow-up: @wonderboymusic
8 years ago

why is this so coupled to the potential transport mechanism? your first approach was sufficient in its idea - setting default props for the widget instance.

seems like a Widget instance that can return its props passed to a REST decorator would avoid tight coupling

#31 in reply to: ↑ 30 @westonruter
8 years ago

Replying to wonderboymusic:

why is this so coupled to the potential transport mechanism?

Coupled to a potential transport mechanism meaning JSON? If the data is to be able to be manipulated by JavaScript, which it should eventually, then the widget instance data should be representable as JSON, so defining a JSON Schema would enforce that. If a widget instance should store more than what is JsonSerializable, then that would mean there would have to be conversion between the internal PHP representation and the external JSON representation, which I understand to be what you mean by having a REST decorator. If we need that additional level of abstraction of an internal-vs-external data representation, as I noted in the previous comment, but in my experience it is seldom needed.

If there was a need for some richer type (e.g. DateTime), a custom type could be used in the widget's schema, but the the REST controller (decorator) could be handle the conversion of the JSON representation to the PHP representation. Right?

your first approach was sufficient in its idea - setting default props for the widget instance.

Beyond having default props for a widget instance, it is useful to have a full schema which can then be read by the WP_Widget::update() method to automate the sanitization of instance changes.

#32 @sc0ttkclark
2 months ago

Since Widgets have been deprecated and we've switched over to Blocks, this ticket can probably be closed.

#33 @johnbillion
2 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.