WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 3 months 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 5 months ago.

Download all attachments as: .zip

Change History (23)

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


5 months 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
5 months 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
5 months 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
5 months ago

TimothyBJacobs commented on PR #1498:

Really great find. Patch looks good to me.

#5 @desrosj
5 months 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
5 months 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
5 months ago

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

Reopening for backport.

#8 @prbot
5 months 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.


5 months ago

#10 @TimothyBlynJacobs
5 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#11 @desrosj
5 months 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.


4 months 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
4 months ago

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

#14 @prbot
3 months ago

draganescu commented on PR #1524:

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 ?

#15 @prbot
3 months ago

adamziel commented on PR #1524:

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

#16 @prbot
3 months ago

draganescu commented on PR #1524:

Thanks for renaming @adamziel. LGTM now!

#17 @prbot
3 months ago

TimothyBJacobs commented on PR #1524:

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.

#18 @prbot
3 months ago

adamziel commented on PR #1524:

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!

#19 @prbot
3 months ago

TimothyBJacobs commented on PR #1524:

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.

#20 @prbot
3 months ago

adamziel commented on PR #1524:

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

#21 @prbot
3 months ago

TimothyBJacobs commented on PR #1524:

That looks great to me!

Note: See TracTickets for help on using tickets.