Add reference widgets to legacy widget blocks. Use ajax admin endpoint on this blocks. #15801

jorgefilipecosta · 2019-05-23T22:45:31Z

Description

This is a WIP it's working correctly but I still need to check the code to improve it.

During the widgets RFC, we decided that existing widgets will be referenced in the legacy widgets and the real instance should be updated in these cases.
This PR uses the ajax admin endpoint to implement this concept of referencing widgets in the legacy widgets block.
This mechanism is also used to implement callback widgets so this PR supersedes #14395 and allows us to avoid the hack $_POST = array_merge( $_POST, $instance_changes ); present in the other PR.
The rest endpoint is used to get the initial form of all widgets (the ajax admin does not seem to allow that), and it is used to provide the update logic in class widgets that don't have an instance. That endpoint should be read-only unless the widgets apply changes in methods that should not have side-effects.
The ajax admin is used to perform updates on legacy widgets that reference a real widget instance present in the database.

How has this been tested?

I pasted the following code in the block editor:

<!-- wp:legacy-widget {"widgetClass":"WP_Widget_Archives","identifier":"archives-12"} /-->

<!-- wp:legacy-widget {"widgetClass":"WP_Widget_Archives","identifier":"archives-11"} /-->

<!-- wp:legacy-widget {"widgetClass":"WP_Widget_Media_Audio","identifier":"media_audio-26"} /-->

(Update identifier to real widget instances that were create using the widgets screen).

I verified the legacy widget block works as expected and if I open the widgets screen the changes I did appear there.

I used "NattyWP feedburner widget". The widget comes with silesia theme that can be downloaded at wordpress.org/themes/silesia. The theme is not updated to WordPress 5.0 and some warning messages appear. For testing purposes, the widget can also be used standalone by pasting the code available themes.trac.wordpress.org/browser/silesia/1.0.6/include/widgets/feedburner.php in the functions.php of any other theme.

noisysocks · 2019-05-31T04:57:46Z

This is a WIP it's working correctly but I still need to check the code to improve it.

Is it still a WIP?

jorgefilipecosta · 2019-06-03T22:08:52Z

Is it still a WIP?

It suffered a huge rebase but is ready for a review.
With these changes, the legacy widgets should load and allow edits on the widget screen. Preview is not working because it is pending on #15635.

jorgefilipecosta · 2019-09-03T13:39:43Z

Hi @noisysocks,

I'm a little confused, how does this fix #16604? When I don't have any plugins installed and insert a Legacy Widget block, I still see an empty dropdown:

Previously we were showing "Block Areas" widget in legacy widgets. This PR fixes that. The empty dropdown was a bug and not it shows as expected a message saying there is no widget available.

I tried testing this PR with Image Widget, which was the first plugin I found when searching the directory for "widget".

I was able to insert a Legacy Widget block and select Image Widget, but then I was not able to interact with the widget at all. Clicking on Select Image just caused the page to reload.

There are some known problems where some third-party widgets don't work because of relying on a specific dom.
A good way to test legacy widgets is enabling the core widgets by setting the array set on https://github.com/WordPress/gutenberg//blob/a4ceeb0159b6dab27a68c122c9c86c4559258b87/lib/widgets.php#L94 to empty.
Core Widgets should work as expected as they rely on the expected widget events.

That said the Image widget seems to work on my tests:

Is there any error being logged during your tests?

jorgefilipecosta · 2019-09-03T13:44:10Z

It's really apparent that we need to show the title of the widget in the block. For example, you can't tell at all that this is a Search widget:

We show the currently selected widget in the inspector:

We are trying to follow what's happening in other blocks. If we have multiple paragraph blocks, it is not easy to tell which type of paragraph a block is. The user would only know by selecting the block and checking its name on the inspector.
But on widgets, the problem is bigger as most widgets look the same.
cc: @mapk

mapk · 2019-09-06T21:21:18Z

It's really apparent that we need to show the title of the widget in the block.

I'm torn on this one. I'd like to stay inline with how we treat other blocks. But in this case, it's not a block, it's a widget inside a block. Can we adopt the reusable block pattern of displaying a name? Maybe we don't include the "Edit" button in this case though.

jorgefilipecosta · 2019-10-24T17:18:02Z

Hi @noisysocks, @mapk,
The design of legacy widgets was updated and now it contains a title indication similar to reusable blocks.

It follows the logic for the title in the widgets screen can show just the type of widget as a title or type plus the specific instance title set by the user.

I think this PR should be ready.

noisysocks · 2019-10-28T04:59:33Z

A few issues I noticed while testing:

The Legacy Widget block needs need a white background so that it doesn't look strange when the editor CSS has a non-white background:
It's nice that we show the name of the widget now, but because we only do this when the Legacy Widget block is selected, things look pretty confusing when you first land at the Block Areas page:
When I make a change to a Legacy Widget block in the Block Areas page, the Update button in the top right remains disabled.

In the interest of unblocking this PR, this can all be fixed in follow-up PRs—this is all experimental after all! I'll review the code shortly 🙂

noisysocks

Fundamentally, I think things here are looking really good!

The code that renders a widget is pretty tough to follow (as it is in Core 😅), but I think we can aim to clean a lot of that up if/when this feature is merged into Core e.g. by splitting dynamic_sidebar() up.

Before we merge this, I'd really like to see us rethink the REST API design and possibly just ditching it altogether in favour of a regular AJAX action. Happy to chat about this—ping me!

Have you thought about adding E2E tests for widgets? (Can be done as a follow-up.)

noisysocks · 2019-11-04T02:41:33Z

lib/class-wp-rest-widget-updater-controller.php

@@ -34,13 +34,30 @@ public function __construct() {
 	public function register_routes() {
 			register_rest_route(
 				$this->namespace,
-				// Regex representing a PHP class extracted from http://php.net/manual/en/language.oop5.basic.php.
-				'/' . $this->rest_base . '/(?P<identifier>[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*)/',
+				'/' . $this->rest_base . '/',


I think we need to rethink this route and how to make it RESTful / align with the rest of the WordPress REST API.

There's three odd things:

The URL (/wp/v2/widgets) indicates that the route refers to a collection resource, but it's actually referring to a member resource. We should put the identifier of the resource in the URL (e.g. /wp/v2/widgets/:identifier). We could do this by either splitting the route into /wp/v2/class-widgets/:className and /wp/instance-widgets/:identifier, or by prefixing the identifier with the type of identifier, e.g. /wp/v2/widgets/<class|instance>:identifier.

It looks like, so long as plugins are not doing anything weird, that this route won't modify any state on the server. Is that true? If so, this route is safe and should be using GET instead of POST.

The name of the route is /wp/v2/widgets, but really this route returns a widget form given a specific widget. I'd suggest renaming to /wp/v2/widget-forms.

Alternatively, could we ditch REST and make this an AJAX RPC-style endpoint? It might be better a fit since this endpoint is so very highly specific to how WP Admin is implemented (in other words: third parties will likely not find this route useful), and it isn't very clear what the resource the endpoint refers to is.

Hi @noisysocks, I ended up applying another big change to this PR.
I noticed that if we call the existing ajax admin endpoint without any changes, I retrieve its edit form. So instead of reimplementing this logic for reference widgets, I'm just relying on the endpoint.

For the class widgets, we can not use the endpoint, because there is not a widget instance at all. I kept the rest endpoint but modified it as suggested to be clear of what resource we provide a widget-form.
It may be useful e.g: to build an alternative widget edit UI, we can use this endpoint to retrieve a widget edit form and we can use the server-side render endpoint to retrieve the widget frontend.
I could have made it an ajax endpoint but the code would be more complex, e.g: we get validation and permission checked by the rest API.

Regarding the POST vs GET, the problem with get is that URLs have a max character limit depending on the browser. A widget may have a text area and use lots of character so it seemed safer to use POST. Also, widgets may have legitimate reasons to have side effects e.g: file uploads. Using a get this widgets would break.

I could have made it an ajax endpoint but the code would be more complex, e.g: we get validation and permission checked by the rest API.

I still have a slight preference for using an AJAX endpoint instead of a REST API endpoint because of how implementation-specific this endpoint is compared to the rest (ha!) of the REST API. Happy to bite my tongue and have us iterate on this, though. I'm glad that the endpoint has been marked experimental 🙂

It might be good to loop in the REST API team at some point on this question.

Regarding the POST vs GET, the problem with get is that URLs have a max character limit depending on the browser. A widget may have a text area and use lots of character so it seemed safer to use POST. Also, widgets may have legitimate reasons to have side effects e.g: file uploads. Using a get this widgets would break.

Makes sense, thanks 👍

noisysocks · 2019-11-04T03:03:01Z

lib/load.php

@@ -20,6 +20,8 @@
 	}
 	if ( ! class_exists( 'WP_REST_Widget_Areas_Controller' ) ) {
 		require dirname( __FILE__ ) . '/class-experimental-wp-widget-blocks-manager.php';
+	}
+	if ( ! class_exists( 'WP_REST_Widget_Areas_Controller' ) ) {


These two ifs are the same.

noisysocks · 2019-11-04T03:03:30Z

lib/widgets.php

+				wp_nonce_field( 'save-sidebar-widgets', '_wpnonce_widgets', false ),
+				'</form>',
+			)
+		);


What's this for?

I added a comment. Basically for the widgets code to be able to call the ajax endpoint we need a nonce.

noisysocks · 2019-11-04T03:05:51Z

lib/widgets.php

 				isset( $widget_obj['callback'][0] ) &&
-				( $widget_obj['callback'][0] instanceof WP_Widget )
+				( $widget_obj['callback'][0] instanceof WP_Widget ) ) ||
+				strncmp( $widget_id, $block_widget_start, strlen( $block_widget_start ) ) === 0


It's more common (and, I think, easier to understand) to see substr used to check that a string begins with another string.

substr( $widget_id, 0, strlen( $block_widget_start ) ) === $block_widget_start

Performance is not very relevant in this case. But, substring first unconditionally iterates on the string to copy parts of it and the only in the end the comparison is done. The method I'm using does not copy any character and stops the comparison when the first char difference is found. So I normally use this code because it is more performant. I added a comment to say that this logic is a starts with.

Interesting! PHP needs a starts_with function 😛

noisysocks · 2019-11-04T03:11:39Z

packages/block-library/src/legacy-widget/index.php

+ * @return string Returns the rendered widget as a string.
+ */
+function render_widget_by_id( $id ) {
+	// Code extracted from src/wp-includes/widgets.php.


Let's mention which method this code comes from (dynamic_sidebar(), yeah?).

When this code is merged into Core, I'd like to see this part of dynamic_sidebar() split out into its own method. We'll need to remember to do this somehow e.g. a Trac ticket or a TODO comment here.

jorgefilipecosta · 2019-11-08T15:42:35Z

Hi @noisysocks, I applied a considerable reactor in this PR and addressed most of your suggestions. Please, let me know what are your thoughts on the PR now.

noisysocks

This is a big improvement—let's get it in so that it gets the testing and iteration that it deserves. Thanks for the hard work, Jorge! 🙂

noisysocks · 2019-11-11T02:09:29Z

packages/block-library/src/legacy-widget/edit/dom-manager.js

-						<input type="hidden" name="widget_number" className="widget_number" value={ widgetNumber } />
-						<input type="hidden" name="multi_number" className="multi_number" value="" />
-						<input type="hidden" name="add_new" className="add_new" value="" />
+						{ isReferenceWidget && ( <><input type="hidden" name="widget-id" className="widget-id" value={ id } />


nit: Let's put a newline in between the <> and <input>.

noisysocks · 2019-11-11T02:22:52Z

packages/block-library/src/legacy-widget/index.php

+				'idBase'       => array(
+					'type' => 'string',
+				),
+				'widgetNumber' => array(


nit: Is it redundant to prefix all of these attributes with widget? The block is called a legacy-widget, after all.

noisysocks · 2019-11-11T02:49:41Z

packages/block-library/src/legacy-widget/edit/handler.js

+			formData.append( 'widget-height', '200' );
+			formData.append( 'savewidgets', this.widgetNonce );
+			httpRequest.open( 'POST', window.ajaxurl );
+			const self = this;


Let's use a => function instead of a function() function below so that const self = this; isn't necessary.

…nabled.

…in endpoint instead

jorgefilipecosta requested review from aduth, ajitbohra, gziolo, notnownikki, Soean, talldan and youknowriad as code owners May 23, 2019 22:45

jorgefilipecosta added the [Feature] Widgets Screen The block-based screen that replaced widgets.php. label May 23, 2019

jorgefilipecosta mentioned this pull request May 23, 2019

Fix: Legacy widget: callback widgets with a edit form. #14395

Closed

jorgefilipecosta added the REST API Interaction Related to REST API label May 23, 2019

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch 3 times, most recently from 24127bd to f63259e Compare May 27, 2019 21:37

jorgefilipecosta changed the title ~~And reference widgets to legacy widget blocks. Use ajax admin endpoint on this blocks.~~ Add reference widgets to legacy widget blocks. Use ajax admin endpoint on this blocks. May 28, 2019

jorgefilipecosta mentioned this pull request May 29, 2019

Widget-block Screen in wp-admin #13204

Closed

jorgefilipecosta added the Needs Technical Feedback Needs testing from a developer perspective. label May 30, 2019

jorgefilipecosta mentioned this pull request May 30, 2019

Allow core widgets to be used in the legacy widgets block #15396

Merged

jorgefilipecosta mentioned this pull request Jun 3, 2019

Use iframes in legacy widget preview. #14643

Merged

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch from f63259e to 89c7546 Compare June 3, 2019 22:03

jorgefilipecosta added the [Priority] High Used to indicate top priority items that need quick attention label Jun 5, 2019

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch from 89c7546 to c936b9f Compare June 26, 2019 15:54

jorgefilipecosta requested a review from TimothyBJacobs as a code owner June 26, 2019 15:54

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch 3 times, most recently from 3932ba8 to 7618e8d Compare June 27, 2019 14:31

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch from 7618e8d to ceec72c Compare July 9, 2019 21:29

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch from ceec72c to 1d23913 Compare August 20, 2019 19:04

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch 2 times, most recently from b525de5 to a4ceeb0 Compare September 3, 2019 13:34

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch 2 times, most recently from 6af688d to 2cbc838 Compare October 24, 2019 17:11

noisysocks requested changes Nov 4, 2019

View reviewed changes

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch 8 times, most recently from d4d26dd to 4b15b74 Compare November 8, 2019 15:23

noisysocks approved these changes Nov 11, 2019

View reviewed changes

jorgefilipecosta added 3 commits November 11, 2019 10:53

Fix: End to end tests do not disable the experiments after they are e…

b116acb

…nabled.

Legacy widget in progress

889e699

Remove all reference widget logic from the endpoint and used ajax adm…

663ee62

…in endpoint instead

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch from 4b15b74 to b3b2254 Compare November 11, 2019 15:24

Minor code "style" fixes

e1e48ac

jorgefilipecosta force-pushed the add/legacy-widget-by-id branch from b3b2254 to e1e48ac Compare November 11, 2019 17:14

jorgefilipecosta merged commit ce75596 into master Nov 11, 2019

jorgefilipecosta deleted the add/legacy-widget-by-id branch November 11, 2019 17:58

youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reference widgets to legacy widget blocks. Use ajax admin endpoint on this blocks. #15801

Add reference widgets to legacy widget blocks. Use ajax admin endpoint on this blocks. #15801

jorgefilipecosta commented May 23, 2019 •

edited

Loading

noisysocks commented May 31, 2019

jorgefilipecosta commented Jun 3, 2019

jorgefilipecosta commented Sep 3, 2019

jorgefilipecosta commented Sep 3, 2019

mapk commented Sep 6, 2019

jorgefilipecosta commented Oct 24, 2019 •

edited

Loading

noisysocks commented Oct 28, 2019 •

edited

Loading

noisysocks left a comment •

edited

Loading

noisysocks Nov 4, 2019

jorgefilipecosta Nov 8, 2019

noisysocks Nov 11, 2019

noisysocks Nov 4, 2019

noisysocks Nov 4, 2019

jorgefilipecosta Nov 8, 2019

noisysocks Nov 4, 2019

jorgefilipecosta Nov 8, 2019

noisysocks Nov 11, 2019

noisysocks Nov 4, 2019

jorgefilipecosta commented Nov 8, 2019

noisysocks left a comment

noisysocks Nov 11, 2019

noisysocks Nov 11, 2019

noisysocks Nov 11, 2019

Add reference widgets to legacy widget blocks. Use ajax admin endpoint on this blocks. #15801

Add reference widgets to legacy widget blocks. Use ajax admin endpoint on this blocks. #15801

Conversation

jorgefilipecosta commented May 23, 2019 • edited Loading

Description

How has this been tested?

noisysocks commented May 31, 2019

jorgefilipecosta commented Jun 3, 2019

jorgefilipecosta commented Sep 3, 2019

jorgefilipecosta commented Sep 3, 2019

mapk commented Sep 6, 2019

jorgefilipecosta commented Oct 24, 2019 • edited Loading

noisysocks commented Oct 28, 2019 • edited Loading

noisysocks left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Nov 8, 2019

noisysocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented May 23, 2019 •

edited

Loading

jorgefilipecosta commented Oct 24, 2019 •

edited

Loading

noisysocks commented Oct 28, 2019 •

edited

Loading

noisysocks left a comment •

edited

Loading