Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#53660 closed defect (bug) (duplicate)

`$sidebar_widgets` out of sync during batch updates to widgets through the REST API

Reported by: zieladam's profile zieladam Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.8
Component: Widgets Keywords: needs-patch
Focuses: rest-api Cc:


Continuation of #53657.

From @adamziel on

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.

[51432] provided a temporary, "RC friendly" fix to the issue for 5.8. But a better, more permanent solution should be explored for a future release.

See also:

Change History (4)

#1 @desrosj
3 years ago

  • Reporter changed from desrosj to zieladam

Changing reporter to reflect who originally reported the issue.

#2 @zieladam
3 years ago

This very relevant conversation happened on .org slack recently:

Does anyone know why we have both $_wp_sidebars_widgets AND $sidebars_widgets global variables?

Great question @zieladam!
Core uses $_wp_sidebars_widgets private global as a cache of the sidebars_widgets option to initialize $sidebars_widgets global when not in the admin. (Note: The cached version can be modified in memory to switch from single to multi widget.)
$sidebars_widgets is the in memory working state of the widgets.

thank you @hellofromtonya! do you think we could refactor that to have just one? I just finished debugging and a big part of it was that, after updating things, we refreshed only $_wp_sidebars_widgets but not $sidebars_widgets. I will patch it up and it will just work, but I think we’re in a non-future-proof situation and may introduce similar errors again (edited)

Switching over now to look more deeply at the globals and managing their state.

Hey @zieladam , the stack of handlers that have been added for inactive widgets and global states concerns me. 2 weird issues drove the need to add these. While it fixes both of them, I agree with you that fundamentally something is off. I agree that we should explore the 2 globals.
Caution: the globals and the code around them have existed a long time in Core. For those folks who choose to use the Classic Widgets, changes made in these functions and globals will need care to ensure backwards compatibility.

#3 @desrosj
2 years ago

  • Milestone changed from 5.8.1 to 5.9

With 5.8.1 RC due out in about 24 hours, I'm going to punt this one. I'm choosing to punt to 5.9, but if a fix is ready, it can be considered for a 5.8.x minor.

#4 @zieladam
2 years ago

  • Milestone 5.9 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #53816.

The fix is attached to and discussed in so let's close this one as a duplicate – I got caught up in proliferation of tickets and PRs :-)

Note: See TracTickets for help on using tickets.