From 17a5e3226594a475aac5c03d471598e3a03a4637 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Mon, 13 Jun 2022 11:00:25 +0200 Subject: [PATCH 01/11] Refresh sidebars widgets global after calling wp_set_sidebars_widgets --- .../class-wp-rest-widgets-controller.php | 22 +----- src/wp-includes/widgets.php | 29 ++++++-- .../rest-api/rest-widgets-controller.php | 73 +++++++++++++++++++ 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php index 079f18cc1f908..4834c56933c7a 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php @@ -292,16 +292,9 @@ public function update_item( $request ) { global $wp_widget_factory; /* - * retrieve_widgets() contains logic to move "hidden" or "lost" widgets to the - * wp_inactive_widgets sidebar based on the contents of the $sidebars_widgets global. - * - * When batch requests are processed, this global is not properly updated by previous - * calls, resulting in widgets incorrectly being moved to the wp_inactive_widgets - * sidebar. - * - * See https://core.trac.wordpress.org/ticket/53657. + * The name retrieve_widgets() is somewhat misleading. It doesn't just "retrieve". It also + * moves any "hidden" or "lost" widgets to the wp_inactive_widgets sidebar. */ - wp_get_sidebars_widgets(); $this->retrieve_widgets(); $widget_id = $request['id']; @@ -367,16 +360,9 @@ public function delete_item( $request ) { global $wp_widget_factory, $wp_registered_widget_updates; /* - * retrieve_widgets() contains logic to move "hidden" or "lost" widgets to the - * wp_inactive_widgets sidebar based on the contents of the $sidebars_widgets global. - * - * When batch requests are processed, this global is not properly updated by previous - * calls, resulting in widgets incorrectly being moved to the wp_inactive_widgets - * sidebar. - * - * See https://core.trac.wordpress.org/ticket/53657. + * The name retrieve_widgets() is somewhat misleading. It doesn't just "retrieve". It also + * moves any "hidden" or "lost" widgets to the wp_inactive_widgets sidebar. */ - wp_get_sidebars_widgets(); $this->retrieve_widgets(); $widget_id = $request['id']; diff --git a/src/wp-includes/widgets.php b/src/wp-includes/widgets.php index 639c59afa1140..db835773aa3a5 100644 --- a/src/wp-includes/widgets.php +++ b/src/wp-includes/widgets.php @@ -1075,19 +1075,29 @@ function wp_get_sidebar( $id ) { * @access private * * @global array $_wp_sidebars_widgets - * @param array $sidebars_widgets Sidebar widgets and their settings. + * @global array $sidebars_widgets + * + * @param array $new_sidebars_widgets Sidebar widgets and their settings. + * @param boolean $refresh_global_sidebars_widgets Optional. Whether to update $sidebars_widgets + * global. Default true. */ -function wp_set_sidebars_widgets( $sidebars_widgets ) { - global $_wp_sidebars_widgets; +function wp_set_sidebars_widgets( $new_sidebars_widgets, $refresh_global_sidebars_widgets = true ) { + global $_wp_sidebars_widgets, $sidebars_widgets; // Clear cached value used in wp_get_sidebars_widgets(). $_wp_sidebars_widgets = null; - if ( ! isset( $sidebars_widgets['array_version'] ) ) { - $sidebars_widgets['array_version'] = 3; + if ( ! isset( $new_sidebars_widgets['array_version'] ) ) { + $new_sidebars_widgets['array_version'] = 3; } - update_option( 'sidebars_widgets', $sidebars_widgets ); + update_option( 'sidebars_widgets', $new_sidebars_widgets ); + + // Refresh the $sidebars_widgets global. + if ( $refresh_global_sidebars_widgets ) { + $sidebars_widgets = wp_get_sidebars_widgets(); + sync_registered_widgets( true ); + } } /** @@ -1353,8 +1363,11 @@ function retrieve_widgets( $theme_changed = false ) { $sidebars_widgets['wp_inactive_widgets'] = array_merge( $lost_widgets, (array) $sidebars_widgets['wp_inactive_widgets'] ); if ( 'customize' !== $theme_changed ) { - // Update the widgets settings in the database. - wp_set_sidebars_widgets( $sidebars_widgets ); + /* + * The second parameter is false to avoid recursion: refreshing the global + * $sidebars_widgets entails calling retrieve_widgets(). + */ + wp_set_sidebars_widgets( $sidebars_widgets, false ); } return $sidebars_widgets; diff --git a/tests/phpunit/tests/rest-api/rest-widgets-controller.php b/tests/phpunit/tests/rest-api/rest-widgets-controller.php index a548ab79e1ad2..ab19be86ba077 100644 --- a/tests/phpunit/tests/rest-api/rest-widgets-controller.php +++ b/tests/phpunit/tests/rest-api/rest-widgets-controller.php @@ -926,6 +926,79 @@ public function test_create_item_second_instance() { ); } + /** + * @ticket 53816 + */ + public function test_create_and_delete() { + $this->setup_widget( + 'text', + 1, + array( + 'text' => 'Custom text test', + ) + ); + $this->setup_sidebar( + 'sidebar-1', + array( + 'name' => 'Test sidebar', + ), + array( 'text-1' ) + ); + + // Create a new text widget. + $request = new WP_REST_Request( 'POST', '/wp/v2/widgets' ); + $request->set_body_params( + array( + 'id_base' => 'text', + 'sidebar' => 'sidebar-1', + 'instance' => array( + 'encoded' => base64_encode( + serialize( + array( + 'text' => 'Updated text test', + ) + ) + ), + 'hash' => wp_hash( + serialize( + array( + 'text' => 'Updated text test', + ) + ) + ), + ), + ) + ); + rest_get_server()->dispatch( $request ); + + // Delete an old text widget (not the one we just created). + $request = new WP_REST_Request( 'DELETE', '/wp/v2/widgets/text-1' ); + $request->set_query_params( array( 'force' => true ) ); + rest_do_request( $request ); + + // Get a list of all widgets. + $request = new WP_REST_Request( 'GET', '/wp/v2/widgets' ); + $response = rest_get_server()->dispatch( $request ); + $data = $response->get_data(); + $data = $this->remove_links( $data ); + + // Confirm that we deleted exactly the widget that we wanted, and + // no other one. This tests against a regression in running multiple + // request handlers during the same run. See the following comment for more details: + // https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958 + $this->assertCount( 1, $data ); + $this->assertSame( 'text-2', $data[0]['id'] ); + $this->assertSame( 'sidebar-1', $data[0]['sidebar'] ); + $this->assertSameSetsWithIndex( + array( + 'text' => 'Updated text test', + 'title' => '', + 'filter' => false, + ), + $data[0]['instance']['raw'] + ); + } + /** * @ticket 41683 */ From f672c0e88ed9c6f8d31ac418bcc37fb3c24ffab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Mon, 13 Jun 2022 13:43:42 +0200 Subject: [PATCH 02/11] Fix unit tests --- src/wp-includes/widgets.php | 6 +++++- tests/phpunit/tests/rest-api/rest-widgets-controller.php | 9 +++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/wp-includes/widgets.php b/src/wp-includes/widgets.php index db835773aa3a5..a4eb1ae514a5e 100644 --- a/src/wp-includes/widgets.php +++ b/src/wp-includes/widgets.php @@ -1096,7 +1096,11 @@ function wp_set_sidebars_widgets( $new_sidebars_widgets, $refresh_global_sidebar // Refresh the $sidebars_widgets global. if ( $refresh_global_sidebars_widgets ) { $sidebars_widgets = wp_get_sidebars_widgets(); - sync_registered_widgets( true ); + /* + * The name retrieve_widgets() is somewhat misleading. It doesn't just "retrieve". It also + * moves any "hidden" or "lost" widgets to the wp_inactive_widgets sidebar. + */ + retrieve_widgets( true ); } } diff --git a/tests/phpunit/tests/rest-api/rest-widgets-controller.php b/tests/phpunit/tests/rest-api/rest-widgets-controller.php index ab19be86ba077..4ce7ada4abc34 100644 --- a/tests/phpunit/tests/rest-api/rest-widgets-controller.php +++ b/tests/phpunit/tests/rest-api/rest-widgets-controller.php @@ -977,10 +977,11 @@ public function test_create_and_delete() { rest_do_request( $request ); // Get a list of all widgets. - $request = new WP_REST_Request( 'GET', '/wp/v2/widgets' ); - $response = rest_get_server()->dispatch( $request ); - $data = $response->get_data(); - $data = $this->remove_links( $data ); + $request = new WP_REST_Request( 'GET', '/wp/v2/widgets' ); + $request['context'] = 'edit'; + $response = rest_get_server()->dispatch( $request ); + $data = $response->get_data(); + $data = $this->remove_links( $data ); // Confirm that we deleted exactly the widget that we wanted, and // no other one. This tests against a regression in running multiple From db1a2bddab2786a56e8661b510275d79b1fadf76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Mon, 13 Jun 2022 13:48:14 +0200 Subject: [PATCH 03/11] Add the @since annotation --- src/wp-includes/widgets.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wp-includes/widgets.php b/src/wp-includes/widgets.php index a4eb1ae514a5e..f1190012f23b3 100644 --- a/src/wp-includes/widgets.php +++ b/src/wp-includes/widgets.php @@ -1072,6 +1072,7 @@ function wp_get_sidebar( $id ) { * Set the sidebar widget option to update sidebars. * * @since 2.2.0 + * @since 6.0.1 Added the optional $refresh_global_sidebars_widgets argument. * @access private * * @global array $_wp_sidebars_widgets From 31c8c223682ac12553a278bb05f7e40e60736324 Mon Sep 17 00:00:00 2001 From: Adam Zielinski Date: Wed, 15 Jun 2022 17:07:12 +0200 Subject: [PATCH 04/11] Update src/wp-includes/widgets.php Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com> --- src/wp-includes/widgets.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wp-includes/widgets.php b/src/wp-includes/widgets.php index f1190012f23b3..c3f35f02b7fbb 100644 --- a/src/wp-includes/widgets.php +++ b/src/wp-includes/widgets.php @@ -1079,7 +1079,7 @@ function wp_get_sidebar( $id ) { * @global array $sidebars_widgets * * @param array $new_sidebars_widgets Sidebar widgets and their settings. - * @param boolean $refresh_global_sidebars_widgets Optional. Whether to update $sidebars_widgets + * @param bool $refresh_global_sidebars_widgets Optional. Whether to update $sidebars_widgets. * global. Default true. */ function wp_set_sidebars_widgets( $new_sidebars_widgets, $refresh_global_sidebars_widgets = true ) { From 5d9bff2c62dbc29f601ad96ea81fd86e255cbd7c Mon Sep 17 00:00:00 2001 From: Adam Zielinski Date: Wed, 15 Jun 2022 17:07:41 +0200 Subject: [PATCH 05/11] Update tests/phpunit/tests/rest-api/rest-widgets-controller.php Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com> --- tests/phpunit/tests/rest-api/rest-widgets-controller.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/phpunit/tests/rest-api/rest-widgets-controller.php b/tests/phpunit/tests/rest-api/rest-widgets-controller.php index 4ce7ada4abc34..a5491c77278ce 100644 --- a/tests/phpunit/tests/rest-api/rest-widgets-controller.php +++ b/tests/phpunit/tests/rest-api/rest-widgets-controller.php @@ -927,7 +927,14 @@ public function test_create_item_second_instance() { } /** + * Tests that running multiple request handlers (create and delete) deletes the intended widget. + * + * See this comment for more details: + * https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958 + * * @ticket 53816 + * @covers WP_REST_Widgets_Controller::create_item + * @covers WP_REST_Widgets_Controller::delete_item */ public function test_create_and_delete() { $this->setup_widget( From 4f88fd87eb130fd0355e39c342cb42bf808158b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Wed, 15 Jun 2022 17:08:32 +0200 Subject: [PATCH 06/11] Use a block comment instead of inline comments --- .../phpunit/tests/rest-api/rest-widgets-controller.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-widgets-controller.php b/tests/phpunit/tests/rest-api/rest-widgets-controller.php index 4ce7ada4abc34..6c926f2507d73 100644 --- a/tests/phpunit/tests/rest-api/rest-widgets-controller.php +++ b/tests/phpunit/tests/rest-api/rest-widgets-controller.php @@ -983,10 +983,12 @@ public function test_create_and_delete() { $data = $response->get_data(); $data = $this->remove_links( $data ); - // Confirm that we deleted exactly the widget that we wanted, and - // no other one. This tests against a regression in running multiple - // request handlers during the same run. See the following comment for more details: - // https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958 + /* + * Confirm that we deleted exactly the widget that we wanted, and + * no other one. This tests against a regression in running multiple + * request handlers during the same run. See the following comment for more details: + * https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958 + */ $this->assertCount( 1, $data ); $this->assertSame( 'text-2', $data[0]['id'] ); $this->assertSame( 'sidebar-1', $data[0]['sidebar'] ); From 5fa7a025f81859f1be6db5807a6789b486b3eb53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Thu, 16 Jun 2022 11:53:29 +0200 Subject: [PATCH 07/11] Reflow text in the comment block to restart CI checks --- .../rest-api/endpoints/class-wp-rest-widgets-controller.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php b/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php index 4834c56933c7a..50ba5552b6e4e 100644 --- a/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php +++ b/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php @@ -292,8 +292,8 @@ public function update_item( $request ) { global $wp_widget_factory; /* - * The name retrieve_widgets() is somewhat misleading. It doesn't just "retrieve". It also - * moves any "hidden" or "lost" widgets to the wp_inactive_widgets sidebar. + * The name retrieve_widgets() is somewhat misleading. It doesn't just "retrieve". It + * also moves any "hidden" or "lost" widgets to the wp_inactive_widgets sidebar. */ $this->retrieve_widgets(); From baa6ee52593fe50f8868684c7fd2f97492f0ac71 Mon Sep 17 00:00:00 2001 From: Adam Zielinski Date: Mon, 20 Jun 2022 11:20:50 +0200 Subject: [PATCH 08/11] Update src/wp-includes/widgets.php Needed some realignment for the params after changing boolean to bool. Removed the . after $sidebars_widgets as this sentence continues with the word "global.". Co-authored-by: Colin Stewart <79332690+costdev@users.noreply.github.com> --- src/wp-includes/widgets.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wp-includes/widgets.php b/src/wp-includes/widgets.php index c3f35f02b7fbb..2a33f923e43d8 100644 --- a/src/wp-includes/widgets.php +++ b/src/wp-includes/widgets.php @@ -1078,9 +1078,9 @@ function wp_get_sidebar( $id ) { * @global array $_wp_sidebars_widgets * @global array $sidebars_widgets * - * @param array $new_sidebars_widgets Sidebar widgets and their settings. - * @param bool $refresh_global_sidebars_widgets Optional. Whether to update $sidebars_widgets. - * global. Default true. + * @param array $new_sidebars_widgets Sidebar widgets and their settings. + * @param bool $refresh_global_sidebars_widgets Optional. Whether to update $sidebars_widgets + * global. Default true. */ function wp_set_sidebars_widgets( $new_sidebars_widgets, $refresh_global_sidebars_widgets = true ) { global $_wp_sidebars_widgets, $sidebars_widgets; From 289e463640f13aab0426a5a37b31d2c7d173fc09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Zieli=C5=84ski?= Date: Mon, 20 Jun 2022 11:24:35 +0200 Subject: [PATCH 09/11] Add assertions messages --- tests/phpunit/tests/rest-api/rest-widgets-controller.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/phpunit/tests/rest-api/rest-widgets-controller.php b/tests/phpunit/tests/rest-api/rest-widgets-controller.php index 33f45170898c7..74d7663dd1681 100644 --- a/tests/phpunit/tests/rest-api/rest-widgets-controller.php +++ b/tests/phpunit/tests/rest-api/rest-widgets-controller.php @@ -996,16 +996,17 @@ public function test_create_and_delete() { * request handlers during the same run. See the following comment for more details: * https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958 */ - $this->assertCount( 1, $data ); - $this->assertSame( 'text-2', $data[0]['id'] ); - $this->assertSame( 'sidebar-1', $data[0]['sidebar'] ); + $this->assertCount( 1, $data, 'The text-1 widget was not deleted' ); + $this->assertSame( 'text-2', $data[0]['id'], 'The text-2 widget was not preserved' ); + $this->assertSame( 'sidebar-1', $data[0]['sidebar'], 'The text-2 widget is no longer assigned to sidebar-1' ); $this->assertSameSetsWithIndex( array( 'text' => 'Updated text test', 'title' => '', 'filter' => false, ), - $data[0]['instance']['raw'] + $data[0]['instance']['raw'], + 'The content of the text-2 widget changed' ); } From 1542a5bf10ef7c36f5d90f5bc38c488b085a6632 Mon Sep 17 00:00:00 2001 From: Jb Audras Date: Wed, 18 Jan 2023 00:47:10 +0100 Subject: [PATCH 10/11] Update since mention --- src/wp-includes/widgets.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wp-includes/widgets.php b/src/wp-includes/widgets.php index 2a33f923e43d8..86cb9f15bcd3f 100644 --- a/src/wp-includes/widgets.php +++ b/src/wp-includes/widgets.php @@ -1072,7 +1072,7 @@ function wp_get_sidebar( $id ) { * Set the sidebar widget option to update sidebars. * * @since 2.2.0 - * @since 6.0.1 Added the optional $refresh_global_sidebars_widgets argument. + * @since 6.2.0 Added the optional $refresh_global_sidebars_widgets argument. * @access private * * @global array $_wp_sidebars_widgets From f1b71abd360c9b84af2d5d7d0c7c4b7e2e082d7d Mon Sep 17 00:00:00 2001 From: Adam Zielinski Date: Fri, 3 Feb 2023 13:23:15 +0100 Subject: [PATCH 11/11] Update src/wp-includes/widgets.php Co-authored-by: Mukesh Panchal --- src/wp-includes/widgets.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wp-includes/widgets.php b/src/wp-includes/widgets.php index 86cb9f15bcd3f..54662b9b561d9 100644 --- a/src/wp-includes/widgets.php +++ b/src/wp-includes/widgets.php @@ -1072,7 +1072,7 @@ function wp_get_sidebar( $id ) { * Set the sidebar widget option to update sidebars. * * @since 2.2.0 - * @since 6.2.0 Added the optional $refresh_global_sidebars_widgets argument. + * @since 6.2.0 Added the optional `$refresh_global_sidebars_widgets` parameter. * @access private * * @global array $_wp_sidebars_widgets