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

Add separate widgets endpoint #25958

Merged
merged 26 commits into from
Oct 14, 2020

Conversation

TimothyBJacobs
Copy link
Member

@TimothyBJacobs TimothyBJacobs commented Oct 8, 2020

Description

This separates the widgets handling from sidebars into a dedicated widgets controller.

Fixes #25232.

How has this been tested?

Automated tests in progress.

This will require changes to the client.

Types of changes

Breaking change.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@TimothyBJacobs TimothyBJacobs added [Priority] High Used to indicate top priority items that need quick attention Core REST API Task Task for Core REST API efforts REST API Interaction Related to REST API [Type] Breaking Change For PRs that introduce a change that will break existing functionality [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets labels Oct 8, 2020
@TimothyBJacobs TimothyBJacobs added this to the Gutenberg 9.2 milestone Oct 8, 2020
@TimothyBJacobs TimothyBJacobs self-assigned this Oct 8, 2020
@TimothyBJacobs
Copy link
Member Author

There is some more clean up I'd like to do, but I wanted to get this open so that the changes to the Gutenberg client can be discussed.

As I see it, the Client will now need to do a batch request to update all dirty widgets. Then if the order of widgets has changed, a PUT to the sidebar that contains the ordered list of widget IDs like { widgets: [ text-1, rss-1 ] }.

@spacedmonkey
Copy link
Member

@TimothyBJacobs I can't see this working in postman.

Don't we need to load the file here

gutenberg/lib/load.php

Lines 35 to 37 in c1390e0

if ( ! class_exists( 'WP_REST_Sidebars_Controller' ) ) {
require_once dirname( __FILE__ ) . '/class-wp-rest-sidebars-controller.php';
}

and setup the class here

gutenberg/lib/rest-api.php

Lines 191 to 198 in c1390e0

/**
* Registers the Sidebars REST API routes.
*/
function gutenberg_register_sidebars_endpoint() {
$sidebars = new WP_REST_Sidebars_Controller();
$sidebars->register_routes();
}
add_action( 'rest_api_init', 'gutenberg_register_sidebars_endpoint' );

If you can do this, I will continue my testing.

@TimothyBJacobs
Copy link
Member Author

@spacedmonkey yep its completely broken at the moment, sorry for the early ping. Working on a fix.

'id' => $widget_id,
'id_base' => '',
'sidebar' => $sidebar_id,
'widget_class' => '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a unit test for namespaced widget class.

@TimothyBJacobs
Copy link
Member Author

Huzzah, PHPunit is now passing! Will start on your suggestions @spacedmonkey in a bit.

@spacedmonkey
Copy link
Member

In my testing, a widget with namespace works.

5:
description: "A wibble form for your site."
id: "wibble1-2"
id_base: "wibble1"
name: "Wibble!"
number: 2
rendered: "<section id="wibble1-2" class="widget widget_wibble1"><h2 class="widget-title">TRfsd</h2></section>"
settings: {title: "TRfsd"}
sidebar: "sidebar-1"
widget_class: "WordPressWidgetBoilerplate\WordPress\WP_Widget_Wibble"
_links: {collection: Array(1), self: Array(1), wp:sidebar: Array(1), curies: Array(1)}
__proto__: Object

@TimothyBJacobs
Copy link
Member Author

@spacedmonkey looks like the tests need to be updated for the new fields.

@spacedmonkey
Copy link
Member

@TimothyBJacobs I am thinking about creating that widget_type api and moving some of the fields out into that.

@TimothyBJacobs
Copy link
Member Author

👍 let's get the tests passing again here first.

@TimothyBJacobs
Copy link
Member Author

The failing test is test_all_supported which looks to be failing on main as well.

@noisysocks
Copy link
Member

I checked out this PR and tested adding, removing and updating blocks and legacy widgets in the widgets editor. The only problem I could spot is that reference widgets (widgets registered without using WP_Widget) no longer can be saved.

You can test this by adding this code, going to Appearance → Widgets, inserting a Marquee Greeting into a widget area, then clicking Save.

wp_register_sidebar_widget(
	'marquee_greeting',
	'Marquee Greeting',
	function() {
		$greeting = get_option( 'marquee_greeting', 'Hello!' );
		printf( '<marquee>%s</marquee>', esc_html( $greeting ) );
	}
);

wp_register_widget_control(
	'marquee_greeting',
	'Marquee Greeting',
	function() {
		if ( isset( $_POST['marquee-greeting'] ) ) {
			update_option(
				'marquee_greeting',
				sanitize_text_field( $_POST['marquee-greeting'] )
			);
		}

		$greeting = get_option( 'marquee_greeting' );
		?>
		<p>
			<label for="marquee-greeting">Greeting:</label>
			<input
				id="marquee-greeting"
				class="widefat"
				name="marquee-greeting"
				type="text"
				value="<?= esc_attr( $greeting ) ?>"
				placeholder="Hello!"
			/>
		</p>
		<?php
	}
);

Reference widgets always have an id, so they end up being routed to the
update_item endpoint instead of the create_item endpoint. We now allow for
saving reference widgets that aren't yet assigned to a sidebar if they
include their desired sidebar in the request.
@TimothyBJacobs
Copy link
Member Author

Thanks for testing @noisysocks! I've pushed a fix. The issue was that reference widgets always have an ID, so it was detected as an update. However, the widget wasn't yet assigned to a sidebar which the update endpoint expects. I've fixed this to allow for reference widgets to pass the sidebar id to update and treat that as creation.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the widgets editor again and nothing breaks 👍

@TimothyBJacobs TimothyBJacobs merged commit 9d8d9e5 into WordPress:master Oct 14, 2020
@TimothyBJacobs
Copy link
Member Author

Thanks for testing @noisysocks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core REST API Task Task for Core REST API efforts [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Package] Edit Widgets /packages/edit-widgets [Priority] High Used to indicate top priority items that need quick attention REST API Interaction Related to REST API [Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a "/widgets" endpoint
4 participants