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

jorgefilipecosta · 2019-03-12T20:10:38Z

This PR enhances the work done in #13511 to make sure we support call back widgets with a custom edit form.

Testing

As a test subject, I used "NattyWP feedburner widget". The widget comes with silesia theme that can be downloaded at https://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 https://themes.trac.wordpress.org/browser/silesia/1.0.6/include/widgets/feedburner.php in the functions.php of any other theme.

aduth · 2019-03-27T21:11:45Z

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

@@ -69,6 +69,9 @@ function register_block_core_legacy_widget() {
 'isCallbackWidget' => array(
 'type' => 'boolean',
 ),
+ 'hasEditForm' => array(


It seems odd to me to call this an attribute of the block, considering that it's of no consequence to the saved result or semantic meaning of a block's content. It seems like it's being used this way as a convenience?

aduth · 2019-03-27T21:13:28Z

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

-
- $widget = $request->get_param( 'identifier' );
-
+ private function isValidWidget( $identifier, $is_callback_widget ) {


Is there a reason for this not be named using snake_case ?

Use lowercase letters in variable, action/filter, and function names (never camelCase). Separate words via underscores.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions

Hi @aduth the name was updated the snake case.

aduth

I added the label "REST API Interaction" in hopes of drawing attention from those with REST API expertise who may be able to provide more feedback.

aduth · 2019-04-29T17:57:08Z

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

@@ -34,8 +34,7 @@ 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 . '/(?P<identifier>[\w-_]+)/',


Can you explain why this is changing?

Hi @aduth, previously the identifier was only the name of a PHP class. Now the identifier can be the name of a PHP class or the instance id of a widget (for callback widgets), so the regex needed this update to match instance ids.

aduth · 2019-04-29T18:01:31Z

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

+ */
+ public function compute_new_widget( $request ) {
+ $identifier = $request->get_param( 'identifier' );
+ $is_callback_widget = $request->get_param( 'is_callback_widget' );


I believe this should be included in args for the register_routes function?

I was not sure if args should only be used for arguments part of the route itself e.g: /(?P[\w-_]+)/. But in my tests, it seems args also work on arguments part of the post data, so I updated the code to follow your suggestion.

aduth · 2019-04-29T18:01:59Z

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

+ public function compute_new_widget( $request ) {
+ $identifier = $request->get_param( 'identifier' );
+ $is_callback_widget = $request->get_param( 'is_callback_widget' );
+ if ( null === $is_callback_widget ) {


Adding to the previous comment, if it was specified as an args as of type boolean, could we assign the default there such that we never expect it to be any value other than true or false, and not need to include this logic here?

aduth · 2019-04-29T18:02:15Z

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

+ /**
+ * Returns the new widget instance and the form that represents it.
+ *
+ * @since 5.2.0


These @since will need to be updated.

aduth · 2019-04-29T18:03:05Z

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

- $widget = $request->get_param( 'identifier' );
-
+ private function is_valid_widget( $identifier, $is_callback_widget ) {
+ if ( null === $identifier ) {


It seems like we should just be making these args as required ? Rather than trying to deal with validating the request format from the implementation.

…flag for the identifier. (+2 squashed commits) Squashed commits: [86539f0] Remove hasEditForm attribute [3045a2d] Fix: Legacy widget: allow to use the edit form of callback widgets with a custom form.

jorgefilipecosta · 2019-05-23T22:52:17Z

I'm closing this PR while we try #15801.

jorgefilipecosta requested review from aduth, ajitbohra, gziolo, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners March 12, 2019 20:10

jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Mar 12, 2019

jorgefilipecosta force-pushed the fix/legacy-call-back-widgets-with-forms branch 3 times, most recently from 1804beb to 9644b7b Compare March 12, 2019 21:57

jorgefilipecosta mentioned this pull request Mar 13, 2019

Try Legacy widget block #13511

Merged

jorgefilipecosta force-pushed the fix/legacy-call-back-widgets-with-forms branch from 9644b7b to 123bf60 Compare March 14, 2019 10:47

jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement. [Feature] Widgets Screen The block-based screen that replaced widgets.php. and removed [Status] In Progress Tracking issues with work in progress labels Mar 14, 2019

jorgefilipecosta mentioned this pull request Mar 18, 2019

Support storing blocks in sidebars #14251

Closed

7 tasks

jorgefilipecosta force-pushed the fix/legacy-call-back-widgets-with-forms branch from 123bf60 to c4c4e14 Compare March 22, 2019 18:59

jorgefilipecosta mentioned this pull request Mar 22, 2019

Add legacy widget to calendar transform #14586

Merged

aduth reviewed Mar 27, 2019

View reviewed changes

jorgefilipecosta force-pushed the fix/legacy-call-back-widgets-with-forms branch 3 times, most recently from 0aea34b to 21f4a7e Compare April 9, 2019 10:20

aduth added the REST API Interaction Related to REST API label Apr 29, 2019

aduth reviewed Apr 29, 2019

View reviewed changes

jorgefilipecosta force-pushed the fix/legacy-call-back-widgets-with-forms branch 3 times, most recently from 1e8fb26 to a29054b Compare May 1, 2019 14:43

jorgefilipecosta force-pushed the fix/legacy-call-back-widgets-with-forms branch 3 times, most recently from 14b7a92 to b86f9d4 Compare May 2, 2019 13:53

jorgefilipecosta force-pushed the fix/legacy-call-back-widgets-with-forms branch from b86f9d4 to 7b4ed34 Compare May 15, 2019 09:14

Added is_callback_widget to the controller arguments; Added required …

bba10fa

…flag for the identifier. (+2 squashed commits) Squashed commits: [86539f0] Remove hasEditForm attribute [3045a2d] Fix: Legacy widget: allow to use the edit form of callback widgets with a custom form.

jorgefilipecosta force-pushed the fix/legacy-call-back-widgets-with-forms branch from 7b4ed34 to bba10fa Compare May 23, 2019 14:48

jorgefilipecosta mentioned this pull request May 23, 2019

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

Merged

jorgefilipecosta closed this May 23, 2019

youknowriad deleted the fix/legacy-call-back-widgets-with-forms branch May 27, 2020 17:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jorgefilipecosta commented Mar 12, 2019

aduth Mar 27, 2019

aduth Mar 27, 2019

jorgefilipecosta Apr 8, 2019

aduth left a comment

aduth Apr 29, 2019

jorgefilipecosta May 1, 2019

aduth Apr 29, 2019

jorgefilipecosta May 1, 2019

aduth Apr 29, 2019

aduth Apr 29, 2019

aduth Apr 29, 2019

jorgefilipecosta commented May 23, 2019


		$widget = $request->get_param( 'identifier' );

		private function isValidWidget( $identifier, $is_callback_widget ) {

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

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

Conversation

jorgefilipecosta commented Mar 12, 2019

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth 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

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