WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 3 weeks ago

Last modified 7 days ago

#53657 closed defect (bug) (fixed)

Block moves to "Inactive widgets" after saving

Reported by: zieladam Owned by: desrosj
Milestone: 5.8 Priority: high
Severity: normal Version: 5.8
Component: Widgets Keywords: has-patch commit fixed-major dev-reviewed has-unit-tests
Focuses: administration, rest-api Cc:

Description

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.

Attachments (1)

Zrzut ekranu 2021-07-14 o 16.32.41.png (1.2 MB) - added by zieladam 3 weeks ago.

Download all attachments as: .zip

Change History (14)

This ticket was mentioned in PR #1498 on WordPress/wordpress-develop by adamziel.


3 weeks ago

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

#2 @zieladam
3 weeks ago

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

#3 @desrosj
3 weeks ago

  • Component changed from General to Widgets
  • Focuses administration added
  • Milestone changed from Awaiting Review to 5.8

Moving to 5.8 for consideration.

#4 @prbot
3 weeks ago

TimothyBJacobs commented on PR #1498:

Really great find. Patch looks good to me.

#5 @desrosj
3 weeks ago

  • Focuses rest-api added
  • Keywords commit added

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.

#6 @desrosj
3 weeks ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

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.

#7 @desrosj
3 weeks ago

  • Keywords dev-feedback fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#8 @prbot
3 weeks ago

desrosj commented on PR #1498:

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.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 weeks ago

#10 @TimothyBlynJacobs
3 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

#11 @desrosj
3 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

This ticket was mentioned in PR #1524 on WordPress/wordpress-develop by adamziel.


7 days ago

  • Keywords has-unit-tests added

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:

#13 @zieladam
7 days ago

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

Note: See TracTickets for help on using tickets.