Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh $sidebars_widgets before calling retrieve_widgets in WP_REST_Widgets_Controller a safe, RC-friendly way #1498

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Jul 14, 2021

Solves WordPress/gutenberg#33335, https://core.trac.wordpress.org/ticket/53657

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 WordPress/gutenberg#33335

Test plan

  1. Go to /wp-admin/widgets.php without this PR applied
  2. Run the gist from https://gist.github.com/adamziel/857d18a9cc9a99ac6be9dc2714b699d4 in your browser's dev tools
  3. Wait a few seconds and confirm it says Broken state generated on attempt 1
  4. Apply this PR, refresh the page in your browser
  5. Run the same gist
  6. Confirm it keeps saying Valid state generated on attempt 1 and doesn't report problems regardless of how long you keep waiting
  7. Also, play with the widgets editor for a while and confirm nothing is broken with respect to updating and deleting widgets

adamziel added 2 commits July 14, 2021 16:21
…Widgets_Controller a safe, RC-friendly way.

This 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 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 moved to the wp_inactive_widgets sidebar.
@TimothyBJacobs
Copy link
Member

Really great find. Patch looks good to me.

@desrosj
Copy link
Contributor

desrosj commented Jul 14, 2021

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 desrosj closed this Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants