Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Legacy Widget add support for widgets with attributes that can't be serialised into JSON #28902

Closed
noisysocks opened this issue Feb 10, 2021 · 7 comments · Fixed by #29960
Closed
Assignees
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Customizer Ability to add and edit blocks in Customize → Widgets. [Feature] Widgets Screen The block-based screen that replaced widgets.php. REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended

Comments

@noisysocks
Copy link
Member

Description

The Legacy Widget block will choke when used with a widget that has an instance attribute which can't be serialised into JSON. WordPress supports storing any PHP value in a widget's instance attributes.

Step-by-step reproduction instructions

  1. Add the following code to a plugin file e.g. gutenberg.php:
class Date_Widget extends WP_Widget {
	protected $default_instance = array(
		'date' => null,
	);

	public function __construct() {
		$widget_ops  = array(
			'classname'                   => 'widget_date',
			'description'                 => __( 'A date.', 'gutenberg' ),
			'customize_selective_refresh' => true,
		);
		$control_ops = array(
			'width'  => 200,
			'height' => 350,
		);
		parent::__construct( 'date', __( 'Date', 'gutenberg' ), $widget_ops, $control_ops );
	}

	public function widget( $args, $instance ) {
		echo $args['before_widget'];
		if ( $instance['date'] ) {
			echo $instance['date']->format( 'c' );
		}
		echo $args['after_widget'];
	}

	public function update( $new_instance, $old_instance ) {
		$instance = array_merge( $this->default_instance, $old_instance );
		if ( $new_instance['date'] ) {
			$instance['date'] = DateTime::createFromFormat( 'Y-m-d', $new_instance['date'] );
		} else {
			$instance['date'] = null;
		}
		return $instance;
	}

	public function form( $instance ) {
		$instance = wp_parse_args( (array) $instance, $this->default_instance );
		?>
		<p>
			<label for="<?php echo $this->get_field_id( 'date' ); ?>">Date:</label>
			<input
				id="<?php echo $this->get_field_id( 'date' ); ?>"
				class="widefat"
				name="<?php echo $this->get_field_name( 'date' ); ?>"
				type="date"
				value="<?php echo $instance['date'] ? $instance['date']->format( 'Y-m-d' ) : ''; ?>"
			/>
		</p>
		<?php
	}
}
add_action(
	'widgets_init',
	function() {
		register_widget( 'Date_Widget' );
	}
);
  1. Navigate to Appearance → Widgets.

  2. Use the insert to add a Date legacy widget.

  3. Set a date and press Update.

  4. Refresh the page.

Expected behaviour

The Date legacy widget should be in the editor and display the date that was set.

Actual behaviour

The Date legacy widget is missing from the editor and there are network errors in DevTools.

Screenshots or screen recording (optional)

Kapture 2021-02-10 at 11 18 56

WordPress information

  • WordPress version: trunk
  • Gutenberg version: master
  • Are all plugins except Gutenberg deactivated? Yes
  • Are you using a default theme (e.g. Twenty Twenty-One)? Yes
@noisysocks noisysocks added [Type] Bug An existing feature does not function as intended REST API Interaction Related to REST API [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Customizer Ability to add and edit blocks in Customize → Widgets. labels Feb 10, 2021
@noisysocks noisysocks added this to Inbox in Block-based Widgets Editor via automation Feb 10, 2021
@noisysocks noisysocks moved this from Inbox to Needs dev in Block-based Widgets Editor Feb 10, 2021
@noisysocks
Copy link
Member Author

The Customizer addresses this by using base64 encoded strings instead of JSON to send a widget's attributes to the frontend. I think we should take a similar approach in the Legacy Widget block.

@noisysocks
Copy link
Member Author

noisysocks commented Feb 10, 2021

Here's roughly how I think the Legacy Widget block can work.

An empty Legacy Widget block contains just an ID.

<!-- wp:legacy-widget { "id": "custom_html" } /-->

After it is mounted, the block makes a request to /wp/v2/widget-types/:id/control:

GET /wp/v2/widget-types/custom_html/control

{
	"html": "<form>...</form>"
}

The HTML in html is used to render the block's Edit tab.

After the user makes some edits in the Edit tab, the block grabs the form data and POSTs it to the same endpoint.

POST /wp/v2/widget-types/custom_html/control

title=My+HTML&content=Custom+html+goes+here

{
	"html": "<form>...</form>",	
	"instance": {
		"encoded": "086ODoiRGF0ZVRpbWUiOjM6e3M...",
		"raw": {
			"title": "My HTML content",
			"content": "Custom html goes here"
		}
	}
}

The Edit tab is then replaced with the updated HTML in html, and instance.raw and instance.encoded are added to the block's attributes.

<!-- wp:legacy-widget { "id": "custom_html", "instance": { "encoded": "086ODoiRGF0ZVRpbWUiOjM6e3M...", "raw": { "title": "My HTML content", "content": "Custom html goes here" } } } /-->

When the block again wants to render the Edit tab in the future, it provides the encoded instance.

GET /wp/v2/widget-types/custom_html/control?instance=086ODoiRGF0ZVRpbWUiOjM6e3M...

{
	"html": "<form>...</form>",
	"instance": {
		"encoded": "086ODoiRGF0ZVRpbWUiOjM6e3M...",
		"raw": {
			"title": "My HTML content",
			"content": "Custom html goes here"
		}
	}
}

Finally, to render the Preview tab, the block provides the encoded instance to the iframe.

<iframe src="legacy-widget-preview.php?id=custom_html&instance=086ODoiRGF0ZVRpbWUiOjM6e3M..." />

Things to note:

  • instance.encoded is base64_encode( serialize( $instance ) ).
  • instance.raw is used by the block to implement block transforms from the Legacy Widget block to other block types.
  • If $instance cannot be JSON-serialized, only instance.encoded is set. instance.raw is always optional.
  • instance.raw / $instance may be an object (multi widgets) or a single value (non-multi widgets).
  • Customizer uses instance_hash_key to prevent tampering. We may want to do the same. See WP_Customize_Widgets::sanitize_widget_instance.

@TimothyBJacobs
Copy link
Member

This seems sensible to me. We need to make sure we apply the same hashing that the customizer does to avoid unserializing arbitrary input.

@draganescu draganescu changed the title Legacy Widget block does not support widgets with attributes that can't be serialised into JSON Legacy Widget add support for widgets with attributes that can't be serialised into JSON Feb 18, 2021
@noisysocks
Copy link
Member Author

The repro steps posted in the description now work but you can still see an error to do with incorrect serialisation if you click Preview on the Legacy Widget.

@noisysocks
Copy link
Member Author

Noting that need to remove the temporary hack added in #29365 as part of fixing this.

@noisysocks
Copy link
Member Author

noisysocks commented Mar 9, 2021

I've opened #29649 which contains the REST API changes needed for this.

I deviated a little from my plan outlined in #28902 (comment):

  • Instead of a separate /wp/v2/widget-types/:id/control endpoint which encodes form data into a serialised instance string, there is a ?preview=1 param to the regular /wp/v2/widgets endpoint. This reduces the amount of REST API complexity.
  • To obtain the form markup shown in the Legacy Widget block's Edit tab, the client sends an empty POST /wp/v2/widgets?preview=1 request with id_base=$idBase.
  • To obtain the serialised instance string stored in the Legacy Widget block's attributes, the client sends a POST /wp/v2/widgets?preview=1 request with the form data and id_base=$idBase.

edit: Did not end up doing this deviation. See #29649 (comment) 🙂

@noisysocks
Copy link
Member Author

I've opened #29960 which is a WIP PR containing the frontend changes needed for this.

@draganescu draganescu moved this from Needs dev to Issues in progress in Block-based Widgets Editor Mar 23, 2021
@draganescu draganescu linked a pull request Mar 23, 2021 that will close this issue
Block-based Widgets Editor automation moved this from Issues in progress to Done Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Legacy Widget Affects the Legacy Widget Block - used for displaying Classic Widgets [Feature] Widgets Customizer Ability to add and edit blocks in Customize → Widgets. [Feature] Widgets Screen The block-based screen that replaced widgets.php. REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants