Refresh sidebars widgets global after calling wp_set_sidebars_widgets

[adamziel]

Description

Before this PR, batch-saving widgets sometimes moves one of them to "Inactive widgets". The full, step-by-step problem description can be found here.

Tactically, this PR removes the retrieve_widget call that was placed in the API endpoints as a workaround for the following problem:

		 * 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.

The solution introduced in this PR is correctly refreshing the related global variable in wp_set_sidebar_widgets.

Test plan

• Confirm the attached unit test correctly checks for the regression.
• Confirm all the tests pass.
• If you'd like to do some manual testing – it's hard to trigger the problem from the UI. Instead, switch themes to Twenty twenty one, go to your widgets screen in wp-admin, run the script below in your devtools, and refresh the page – you should have exactly three widgets assigned to the sidebar and zero widgets in "Inactive widgets":

async function giveMeThreeWidgets(){
    
	const sidebars = ['sidebar-1', 'wp_inactive_widgets'];
	async function* getWidgetsIds() {
		for(const sidebar of sidebars) {
				const sidebarWidgets = await window.wp.apiFetch({
					path: `/index.php?rest_route=%2Fwp%2Fv2%2Fsidebars%2F${sidebar}&_locale=user`
				});
				yield* sidebarWidgets.widgets;
		}
	}
	async function toArray(asyncIterator){
		const arr = [];
		for await(const i of asyncIterator) arr.push(i);
		return arr;
	}

	const ids = await toArray(getWidgetsIds());
	return await window.wp.apiFetch( {
		path: "/index.php?rest_route=%2Fbatch%2Fv1&_locale=user", 
		"data": {
			"validation": "require-all-validate", "requests": [
				{ "path": "/wp/v2/widgets","body": {"id_base": "block","instance": { "raw": { "content": "<!-- wp:legacy-widget /-->" } },"sidebar": "sidebar-1"},					"method": "POST"				},
				{ "path": "/wp/v2/widgets", "body": {"id_base": "block", "instance": { "raw": { "content": "<!-- wp:gallery {\"ids\":[10,8,7],\"linkTo\":\"none\"} -->\n<figure class=\"wp-block-gallery columns-3 is-cropped\"><ul class=\"blocks-gallery-grid\"><li class=\"blocks-gallery-item\"><figure><img src=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc5-1024x576.jpg\" alt=\"\" data-id=\"10\" data-full-url=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc5.jpg\" data-link=\"http://localhost:8888/?attachment_id=10\" class=\"wp-image-10\"/></figure></li><li class=\"blocks-gallery-item\"><figure><img src=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc3-1024x576.jpg\" alt=\"\" data-id=\"8\" data-full-url=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc3.jpg\" data-link=\"http://localhost:8888/?attachment_id=8\" class=\"wp-image-8\"/></figure></li><li class=\"blocks-gallery-item\"><figure><img src=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc2-1024x640.jpg\" alt=\"\" data-id=\"7\" data-full-url=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc2.jpg\" data-link=\"http://localhost:8888/?attachment_id=7\" class=\"wp-image-7\"/></figure></li></ul></figure>\n<!-- /wp:gallery -->" }						}, "sidebar": "sidebar-1"					}, "method": "POST"				},
				{ "path": "/wp/v2/widgets","body": {"id_base": "block","instance": { "raw": { "content": "<!-- wp:gallery {\"ids\":[9,7],\"linkTo\":\"none\"} -->\n<figure class=\"wp-block-gallery columns-2 is-cropped\"><ul class=\"blocks-gallery-grid\"><li class=\"blocks-gallery-item\"><figure><img src=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc4-1024x576.jpg\" alt=\"\" data-id=\"9\" data-full-url=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc4.jpg\" data-link=\"http://localhost:8888/?attachment_id=9\" class=\"wp-image-9\"/></figure></li><li class=\"blocks-gallery-item\"><figure><img src=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc2-1024x640.jpg\" alt=\"\" data-id=\"7\" data-full-url=\"http://localhost:8888/wp-content/uploads/2021/07/L-Misc2.jpg\" data-link=\"http://localhost:8888/?attachment_id=7\" class=\"wp-image-7\"/></figure></li></ul></figure>\n<!-- /wp:gallery -->" } },"sidebar": "sidebar-1"},"method": "POST" },
				...ids.map( id => ( { "path": `/wp/v2/widgets/${ id }?force=true`, "method": "DELETE" } ) ),
			]
		},
		"method": "POST",
	} )
}
giveMeThreeWidgets()

Trac ticket: https://core.trac.wordpress.org/ticket/53816

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[adamziel]

@draganescu feedback addressed, would you mind re-reviewing please?

[draganescu]

This looks good now IMO

[hellofromtonya]

A few coding standard fixes needed. Then I'll do a testing with both the block and classic widgets.

[adamziel]

@hellofromtonya I committed your suggestions – thank you! There were also some conflicts that I resolved. This should be good to go!

[hellofromtonya]

Test Report

Env:

• OS: macOS Big Sur 11.5.2
• Browser: Chrome 92.0.4515.159 and Firefox 91.0.2
• Local testing: wp-env (npm/Docker)
• Theme: Twenty Twenty-One
• Plugins: Classic Widgets (activated during that part of the testing)

Testing Instructions

Step 1: Set up the environment:

• Pull the latest master
• Build and start

npm install
npm run build
npm run env:start
npm run env:install

• Log into back-end http://localhost:8889/wp-admin
• Activate Classic Widgets plugin
• Go to Appearance > Widgets

Step 2: Interact with widgets and observe: add new ones, reorder, and move some to Inactive.

Expectation:

• Add -> adds the new widget. When leaving and returning to the screen, widget is retained
• Reorder -> widgets are reordered and order is retained when leaving screen
• Make inactive -> moves the widget to the Inactive Widgets (no long in the Footer sidebar) and retains widget when leaving screen

Step 3: Interact with widgets in Customizer

Expectations:

• Add -> adds new widget and retains it when closing Customizer
• Reorder -> widgets are reordered and order is retained when closing Customizer
• Make inactive (delete) -> widget is removed from Customizer; when closing Customizer, widget appears in the Inactive Widgets area.

Step 4: Apply this PR and rebuild

npm run grunt patch:https://github.com/WordPress/wordpress-develop/pull/1525
npm run build

Step 5: Repeat Step 2 and 3 above

Expectations: same expectations as above

Results

All tests worked as expected ✅

No change when applying the patch ✅

[spacedmonkey]

Related: #1578

[adamziel]

@hellofromtonya @desrosj I believe all the feedback on this one is now addressed.

[desrosj]

I'm thinking that since #53811 has been punted to the 5.9 release, the same should be done for this one. Especially since the sync_registered_widgets() function is being discussed further in Core-53811 and this PR assumes that the function is present in Core.

I also noted inline that I'm apprehensive to make a change to a function's signature in a minor release unless it cannot be avoided.

[draganescu]

Are we still onboard with this? @desrosj #53811 landed in the mean time so this can move forward?

[adamziel]

Thanks for surfacing this PR @draganescu! I just applied all the required changes.

I'm thinking that since #53811 has been punted to the 5.9 release, the same should be done for this one. Especially since the sync_registered_widgets() function is being discussed further in Core-53811 and this PR assumes that the function is present in Core.

@desrosj I reverted to the old, retrieve_widgets name that we ended up continuing to use in core.

I also noted inline that I'm apprehensive to make a change to a function's signature in a minor release unless it cannot be avoided.

Snap, we've missed the 6.0 train with this one. Would you be willing to reconsider including this patch in 6.0.1, or do you want to wait to 6.1?

[adamziel]

The single failing test is unrelated to this PR:

Resolved .nvmrc as 14
Found in cache @ /opt/hostedtoolcache/node/14.19.3/x64
/opt/hostedtoolcache/node/14.19.3/x64/bin/npm config get cache
/home/runner/.npm
Error: getCacheEntry failed: Cache service responded with 503

[hellofromtonya]

Would you be willing to reconsider including this patch in 6.0.1, or do you want to wait to 6.1?

As this is an enhancement and part of a refactoring, it could not go into a minor, but is a candidate for a major such as 6.1. Minors are for bug fixes and security.

[adamziel]

@hellofromtonya does it need to be added to any list, or do we leave it here and it will get discovered when the time comes?

[hellofromtonya]

Great question @adamziel (and Hello 👋 ). The Trac ticket is already in the 6.1 milestone. And you've refreshed this PR and discussion. As it's in the milestone, it'll also be part of the 6.1 scrub/triage process once the release squad is formed.

Is the updated PR ready for feedback and test report(s)? If yes, some tips: (a) remove the needs-refresh keyword and add a comment in Trac that it's ready and (b) drop the PR into the core-test channel for wider testing and test report(s).

[costdev]

Thanks for the PR! This review makes some minor suggestions for consistency with Core docblocks and with the test suite.

[adamziel]

Thank you @costdev ! I just applied the changes you suggested

[costdev]

Thank you @costdev ! I just applied the changes you suggested

@adamziel Thanks! There's also the message parameters that need to be added to the assertions in the test. See my review comment on this (step 3).

All PHPUnit assertions, as well as all WordPress custom assertions, allow for a $message parameter to be passed. This message will be displayed when the assertion fails and can help immensely when debugging a test. This parameter should always be used if more than one assertion is used in a test method. Handbook Reference

[adamziel]

@costdev Ah good note! I just added these messages and committed your suggestion.

[costdev]

The PR looks great to me. I'll try to get this tested again and leave a report on the ticket. 🙂

[mukeshpanchal27]

Thanks @adamziel, Left nit-pick feedback.

[adamziel]

Thanks @mukeshpanchal27!