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
Copy link
Member Author

@TimothyBJacobs TimothyBJacobs commented Oct 8, 2020

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

@spacedmonkey spacedmonkey commented Oct 8, 2020

@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

@TimothyBJacobs TimothyBJacobs commented Oct 8, 2020

@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' => '',

This comment has been minimized.

@spacedmonkey

spacedmonkey Oct 8, 2020
Member

Add a unit test for namespaced widget class.

@TimothyBJacobs
Copy link
Member Author

@TimothyBJacobs TimothyBJacobs commented Oct 8, 2020

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

@spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Oct 8, 2020

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

@TimothyBJacobs TimothyBJacobs commented Oct 9, 2020

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

@spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Oct 9, 2020

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

@TimothyBJacobs
Copy link
Member Author

@TimothyBJacobs TimothyBJacobs commented Oct 9, 2020

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

@TimothyBJacobs TimothyBJacobs force-pushed the TimothyBJacobs:add/widgets-endpoint branch from 3c4dbd3 to 97d5df3 Oct 14, 2020
@TimothyBJacobs
Copy link
Member Author

@TimothyBJacobs TimothyBJacobs commented Oct 14, 2020

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

@noisysocks
Copy link
Member

@noisysocks noisysocks commented Oct 14, 2020

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

@TimothyBJacobs TimothyBJacobs commented Oct 14, 2020

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

Tested the widgets editor again and nothing breaks 👍

@TimothyBJacobs TimothyBJacobs merged commit 9d8d9e5 into WordPress:master Oct 14, 2020
13 of 14 checks passed
13 of 14 checks passed
@github-actions
Cancel Previous Runs Cancel Previous Runs
Details
@github-actions
Check Check
Details
@github-actions
build
Details
@github-actions
Admin - 1
Details
@github-actions
Compare performance with master
Details
@github-actions
test (gutenberg-editor-gallery) test (gutenberg-editor-gallery)
Details
@github-actions
test (gutenberg-editor-gallery)
Details
@github-actions
JavaScript
Details
@github-actions
PHP PHP
Details
@github-actions
Admin - 2
Details
@github-actions
Admin - 3
Details
@github-actions
Mobile
Details
@github-actions
Admin - 4
Details
Block-based Widgets Editor automation moved this from PRs in progress to Done Oct 14, 2020
@TimothyBJacobs
Copy link
Member Author

@TimothyBJacobs TimothyBJacobs commented Oct 14, 2020

Thanks for testing @noisysocks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment