Block moves to "Inactive widgets" after saving

[zieladam]

From ​https://github.com/WordPress/gutenberg/issues/33335

Description

After adding a block to widgets Footer sidebar and saving changes, the widget moves to Inactive widgets area.
This happens randomly. Can be tricky to reproduce.

Step-by-step reproduction instructions

• Be sure to use Twenty Twenty-One theme
• Go to Appearance > Widgets
• Add a block to Footer sidebar. In my last test "Gallery" block.
• Save changes
• Refresh

Expected behaviour

The widget should remain in Footer sidebar.

Actual behaviour

The widget is moved to Inactive widgets area.

Solves ​https://github.com/WordPress/gutenberg/issues/33335

Calling wp_get_sidebars_widgets() is required, because retrieve_widgets() runs some logic to move "hidden/lost" widgets to wp_inactive_widgets sidebar. It does so based on the global $sidebars_widgets. Normally this is not a problem, but when processing batch requests, $sidebars_widgets isn't properly updated by a previous call to

WP_REST_Widgets_Controller::create_item() – only global $_wp_sidebars_widgets has been changed. So, as far as retrieve_widgets() is concerned, the last created widget isn't assigned to any sidebar and so it is mistakenly moved to the wp_inactive_widgets sidebar.

This fix is a temporary one so that we may ship WordPress 5.8 without that bug. I will also propose another PR to address the fundamental root cause of the problem here. For more context, see ​https://github.com/WordPress/gutenberg/issues/33335

This is related to Trac #53657

[zieladam]

I added a fix that should be safe enough to land in 5.8 RC4. I will also publish a PR that doesn't just alleviates the symptoms, but addresses the root cause of this problem (see ​https://github.com/WordPress/gutenberg/issues/33335 for more details on that).

[desrosj]

Moving to 5.8 for consideration.

Really great find. Patch looks good to me.

[desrosj]

Thanks for taking the time to dive into this one @zieladam! I have some very minor nits related to the inline docs, but I'll adjust those before committing.

[desrosj]

In 51432:

Widgets: Prevent widgets unintentionally being moved to the inactive sidebar.

This fixes a bug where widgets are unintentionally moved to the wp_inactive_widgets sidebar when batch updates occur through the REST API.

When batch requests are processed, only $_wp_sidebars_widgets is updated by previous calls to WP_REST_Widgets_Controller::create_item(). $sidebars_widgets is not aware of the new widget’s intended location, and retrieve_widgets()` mistakenly flags the widget as inactive.

Calling wp_get_sidebars_widgets() before retrieve_widgets() ensures both global variables match and is intended as a temporary fix until the root cause of the problem can be fixed.

Props zieladam, htmgarcia, timothyblynjacobs.
Fixes #53657.

[desrosj]

Reopening for backport.

Thanks again @adamziel! Merged into Core in https://core.trac.wordpress.org/changeset/51432.

I've opened Trac-53660 to explore a more permanent solution. Let's iterate there, and open a new PR when the time comes.

[desrosj]

In 51439:

Widgets: Prevent widgets unintentionally being moved to the inactive sidebar.

This fixes a bug where widgets are unintentionally moved to the wp_inactive_widgets sidebar when batch updates occur through the REST API.

When batch requests are processed, only $_wp_sidebars_widgets is updated by previous calls to WP_REST_Widgets_Controller::create_item(). $sidebars_widgets is not aware of the new widget’s intended location, and retrieve_widgets() mistakenly flags the widget as inactive.

Calling wp_get_sidebars_widgets() before retrieve_widgets() ensures both global variables match and is intended as a temporary fix until the root cause of the problem can be fixed.

Props zieladam, htmgarcia, timothyblynjacobs.
Merges [51432] to the 5.8 branch.
Fixes #53657.

We use a function called retrieve_widgets to assign orphaned widgets to an inactive sidebar. The name suggests that it's a getter, yet the function actually updates the database. The name is misleading and it bit us in ​https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958 and https://core.trac.wordpress.org/ticket/53657.

This PR deprecates the old name and proposes a new one which communicates the intention in a clearer way: recover_lost_widgets.

Trac ticket:

[zieladam]

Now that this temporary fix has landed, I opened a ticket to discuss the larger problem.

I've seen the comment with "lost" in it, but I feel the recover_lost_widgets name is a bit too nice :) Too epic.
What are we doing, actually? It looks like a synchronization to me so maybe sync_registered_widgets ?

@draganescu it does a lot of things and it's hard to come up with a good name – sync_registered_widgets works for me just as well. All I'm after here is to stop suggesting this function only reads and returns something.

Thanks for renaming @adamziel. LGTM now!

This is kinda teetering on the line of ​refactoring for the sake of refactoring. If we do this rename, I think we should just soft deprecate the retrieve_widgets function so as not to cause unnecessary code churn that is annoying for downstream developers that support more than the latest version of WordPress to support.

This is kinda teetering on the line of refactoring for the sake of refactoring.

It has that feeling for sure. The thing is that name confused a bunch of people into thinking it's a read operation and contributed to introducing a few hard to detect bugs. Even worse, we need to call it in the GET endpoint handler – something I would like to address as one of the next steps here. For the sake of our future selves, I would rather make it obvious that it's a write.

If we do this rename, I think we should just soft deprecate the retrieve_widgets function so as not to cause unnecessary code churn that is annoying for downstream developers that support more than the latest version of WordPress to support.

That's a very good point and something I've missed here. Let's do it!

If we do this rename, I think we should just soft deprecate the retrieve_widgets function so as not to cause unnecessary code churn that is annoying for downstream developers that support more than the latest version of WordPress to support.

Soft deprecate as in don't call _deprecated_function and only do @deprecated annotation?

Yeah, that'd be my thinking.

@TimothyBJacobs done done! Would you confirm it's what you had in mind?

That looks great to me!

Merged with changeset https://core.trac.wordpress.org/changeset/51705.