Legacy widget rendering endpoint

[adamziel]

Description

Follows up on: WordPress/wordpress-develop#1508

This adds a /wp/v2/widget-types/<widget type>/render endpoint which accepts POST data and returns the same output as the previous iframe-oriented GET endpoint.

This PR is for the Gutenberg plugin and not for core so that we the endpoint may be safely used in other Gutenberg PRs.

The polyfill approach draws on learnings from #33810 and may not be perfect – let's use this PR as a testing ground. One shortcoming is that I can't think of a good way to apply the following feedback from @noisysocks:

Did you consider incorporating this functionality into the existing /widget-types/:id/encode endpoint? /encode is an existing RPC-style endpoint that already accepts instance as an argument and returns preview in the response. The only difference is that /encode does not output all of the wrapping HTML including scripts and styles and whatnot. Perhaps it could, though. I'm not sure that we're using the preview from /encode anywhere. If it's possible to have fewer endpoints (especially RPC-style ones) then I'd prefer that! 😀

I like the idea of re-using the /encode endpoint, but I have no idea how to polyfill that. I'd rather avoid overriding the entire endpoint with an updated copy living in this repo – it could send us to a land of two-way synchronization between wordpress-develop and gutenberg which I think is much more complex than one-way backporting workflow.

How has this been tested?

1. Go to /wp-admin/widgets.php
2. Paste the following script into your devtools:

wp.apiFetch({
      "path": "/index.php?rest_route=%2Fwp%2Fv2%2Fwidget-types%2Ftext%2Frender&context=edit&per_page=100&_locale=user",
      "method": "POST",
      "data": {"id_base":"text","instance":{"encoded":"YTo0OntzOjU6InRpdGxlIjtzOjU6InRlc3QyIjtzOjQ6InRleHQiO3M6NToidGVzdDMiO3M6NjoiZmlsdGVyIjtiOjE7czo2OiJ2aXN1YWwiO2I6MTt9","hash":"6bd53d501ad76cfbc50ef417d78dc9f1","raw":{"title":"test2","text":"test3","filter":true,"visual":true}}}
})
.then(console.log.bind(console))
.catch(console.error.bind(console));

3. Confirm the response was successful and the widget got rendered as expected.

Alternatives considered

• ServerSideRender – we need to render a full HTML document with wp_head(), CSS links, maybe some additional scripts, and this is much more that I would like to return from a block rendering API.
• Updating the existing admin_init hook handler to work with POST requests – it's tricky and doesn't seem right, I don't think we should stretch hooks like that. This clearly is an API request so let's treat it as such.

[adamziel]

This is my way of reusing get_item_permissions_check. It should be safe because WP_REST_Widget_Types_Controller is either supplied by core or the Gutenberg plugin.

[adamziel]

I thought about extending WP_REST_Widget_Types_Controller but I don't like bringing over all that code just to be able to check permissions. Inheriting from WP_REST_Controller is more isolated.

[adamziel]

I can't specify version such as 5.8.1 because I can't be certain when this change gets backported. I guess this means that on every core release we'll have to move all the PHP code compat/wordpress-trunk to compat/wordpress-$CURRENT_VERSION

[noisysocks]

I like the idea of re-using the /encode endpoint, but I have no idea how to polyfill that. I'd rather avoid overriding the entire endpoint with an updated copy living in this repo – it could send us to a land of two-way synchronization between wordpress-develop and gutenberg which I think is much more complex than one-way backporting workflow.

Ugh, yeah 💀

register_rest_route() accepts an $override param so maybe we can define a new route handler for /encode which overrides the one defined by Core (in 5.8) or Gutenberg (in 5.7).

(Thinking out loud: With #33810, we could have GB_REST_Widget_Types_Controller live in lib/compat/5.9 and WP_REST_Widget_Types_Controller live in lib/compat/5.8.)

class GB_REST_Widget_Types_Controller extends WP_REST_Widget_Types_Controller {
	public function register_routes() {
		register_rest_route(
			$this->namespace,
			'/' . $this->rest_base . '/(?P<id>[a-zA-Z0-9_-]+)/encode',
			array(
				'args' => array(
					...
				),
				array(
					'methods'             => WP_REST_Server::CREATABLE,
					'permission_callback' => array( $this, 'get_item_permissions_check' ),
					'callback'            => array( $this, 'encode_form_data' ),
				),
			),
			/* override: */ true
		);
	}

	public function encode_form_data( $request ) {
		...
	}
}

[gziolo]

I like the idea of re-using the /encode endpoint, but I have no idea how to polyfill that. I'd rather avoid overriding the entire endpoint with an updated copy living in this repo – it could send us to a land of two-way synchronization between wordpress-develop and gutenberg which I think is much more complex than one-way backporting workflow.

It looks like @noisysocks has an idea of how to reuse the existing endpoint. I don't think that the level of complexity that the solution creates in Gutenberg should drive the final implementation. Even if we had to unregister the endpoint from the core and register its copy it is still better in the long term. I wouldn't worry that much about sync issues at this moment. In practice, I don't remember any case where we had to keep REST API endpoints in sync after they were released in WordPress core.

[anton-vlasenko]

The code looks good to me.
However, I can't review the polyfill thing (I haven't used them much).
The rest of the PR is fine.

[adamziel]

I just updated this PR to override the /encode endpoint @noisysocks. I still don't like this implementation because of the amount of duplicate code, but I'll trust you on this one @gziolo. The PR assumes that we'll merge the change in 5.9 at the moment after @noisysocks suggestion. At the same time this looks like a bugfix so why don't we do 5.8.1 instead?

[draganescu]

Isn't it a bit unexpected to render the widget using an encode endpoint? I would rather if we don't use the preview in encode to remove it from there and just have one place to render and another place to encode :)

[noisysocks]

I was wrong about the preview that comes back from /encode not being used. It's used by legacy-widget/edit/control.js to display the "No preview available" UI if the preview HTML contains only empty HTML containers. Including scripts and styles and whatnot in the preview HTML breaks this logic, which you can test by inserting a Legacy Widget block, selecting e.g. the Image widget, and then previewing without having selected an image.

We could try to fix the "is this HTML empty?" logic but, as @draganescu alludes to, it's probably better to maintain a distinction between these two kinds of preview: one is a simple dump of the widget HTML and the other is a contextualised render of the widget complete with scripts and styles.

So I think let's revert back to adding a seperate /preview endpoint which is used by the iframe. This should reduce the amount of code duplication too, not that that's a primary concern.

Sorry to mess you about!

---

I think that we definitely should try to land a fix for #33540 in 5.8.1 because I imagine that it is a semi-widespread issue. Lots of servers don't support very long URLs. #30049 is a less urgent issue as I don't imagine it's particularly widespread. Nice to have though.

But yes that raises an interesting question: do we add the compatibility shim to lib/compat/wordpress-5.8.1? We did not consider minor releases when discussing #33810. I suppose we should because there is not much that differs in our process on the Gutenberg side of things with respect to major versus minor releases. What do you think @gziolo?

[gziolo]

But yes that raises an interesting question: do we add the compatibility shim to lib/compat/wordpress-5.8.1? We did not consider minor releases when discussing #33810. I suppose we should because there is not much that differs in our process on the Gutenberg side of things with respect to major versus minor releases. What do you think @gziolo?

Yes, we didn't discuss minor WordPress releases in this context because it's a very rare case when so many PHP changes have to be added to the plugin and WP core. There is usually a list of JS-based bug fixes that are cherry-picked to the corresponding major release line. In this case, it's up to you what approach you will take. At the end of the day, it should impact whether we will have to remove one or two folders when the minimum supported version of WordPress is higher than 5.8.

[adamziel]

We could try to fix the "is this HTML empty?" logic but, as @draganescu alludes to, it's probably better to maintain a distinction between these two kinds of preview: one is a simple dump of the widget HTML and the other is a contextualised render of the widget complete with scripts and styles.

I saw that control.js use case and I was thinking about addressing it in just that way, but now that I think about it more it makes sense to just keep it as a separate thing.

Anyway, all good, we're back to /render now @noisysocks :-)

[noisysocks]

When I try to test the endpoint locally I always get a 400 error.

gutenberg (add/legacy-widget-rendering-api) % http -a admin:password POST http://wp-git-build.test/wp-json/wp/v2/widget-types/search/render
HTTP/1.1 400 Bad Request
Access-Control-Allow-Headers: Authorization, X-WP-Nonce, Content-Disposition, Content-MD5, Content-Type
Access-Control-Expose-Headers: X-WP-Total, X-WP-TotalPages, Link
Allow: POST
Cache-Control: no-cache, must-revalidate, max-age=0
Connection: close
Content-Length: 123
Content-Type: application/json; charset=UTF-8
Date: Fri, 27 Aug 2021 06:06:36 GMT
Expires: Wed, 11 Jan 1984 05:00:00 GMT
Link: <http://wp-git-build.test/wp-json/>; rel="https://api.w.org/"
Server: Apache/2.4.48 (Unix) PHP/8.0.9
X-Content-Type-Options: nosniff
X-Powered-By: PHP/8.0.9
X-Robots-Tag: noindex

{
    "code": "rest_missing_callback_param",
    "data": {
        "params": [
            "id_base"
        ],
        "status": 400
    },
    "message": "Missing parameter(s): id_base"
}

It looks like what's happening is that the param is called id in the $route but id_base in the 'params' array.

It should be id so that we're consistent with the base class.

https://github.com/WordPress/wordpress-develop/blob/524c2a6fd3d8818dd4789faffd4e18a1d458e320/src/wp-includes/rest-api/endpoints/class-wp-rest-widget-types-controller.php#L56

Other than that I think this looks good!

[adamziel]

@noisysocks Good catch, in my testing I always had id_base specified in POST body but you're right – it's perfectly fine to call the endpoint without any body. We should be good to go now. Oh, and don't mind the broken test – the problem is in trunk.

[noisysocks]

It works locally! 👍

I still think it's weird to return a Content-Type: application/json response with one field containing HTML instead of a Content-Type: text/html response, but... I guess that makes it work easier with existing tooling and forwards compatible.