Overview: Refactor the widgets read/write logic

[zieladam]

This is an overview/epic issue that serves as the place to have a discussion and also points to many smaller sub-issues.

Widgets-related logic in core got quite confusing over the years. As a result, we've dealt with problems such as ​Blocks moving to "Inactive widgets" after saving (temporarily solved by adding an unexpected ​wp_get_sidebars_widgets(); call).

There are a few problems there:

• We have multiple, closely related global variables ($sidebars_widgets+$_wp_sidebars_widgets, $wp_registered_widgets, $wp_registered_sidebars). If we update one, we should also update the others for consistency. Sometimes we don't and we run into ​undefined behaviors.
• We use a function called retrieve_widgets as a mean to fix any discrepancies in the stored sidebar-to-widget mapping. It's called retrieve, but it actually does some writing. This is a source of confusion in itself so I proposed renaming it.
• We ​we have to call retrieve_widgets in GET API endpoints which makes it read-write, not read-only. This breaks HTTP caching and is also a ​source of bugs.

I don't think we're able to address the proliferation of global variables – it seems like a major BC break. However, we should still be able to improve the retrieve_widgets situation.

Ideally it would:

☐ Be less monolithic and have a clear, encapsulated flow of logic (as suggested by @helloFromTonya)
☐ Have a name suggesting a write, e.g. `remap_widgets`
☐ ​Always be called **after** a write
☐ Always be called after a theme change
☐ Never be required to perform a read
☐ Never be called in GET request handlers

cc @TimothyBlynJacobs @noisysocks @andraganescu @talldanwp @hellofromTonya @desrosj

Trac ticket: https://core.trac.wordpress.org/ticket/53816

Trac ticket: https://core.trac.wordpress.org/ticket/53816
Trac ticket: https://core.trac.wordpress.org/ticket/53811

[hellofromTonya]

Thinking this refactoring work falls into the enhancement category to present an opportunity to rethink state management and complexities.

@draganescu feedback addressed, would you mind re-reviewing please?

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 ?

This looks good now IMO

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

[andraganescu]

I think this is a good quality of life change. Given we offer a deprecated back compatible function and that it's a straight forward rename only accompanied by a test, I think it's good to go.

We should add the devnote and any required doc entries for this change.

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.

[desrosj]

Removing the commit keyword as there is still some ongoing discussion on the PR.

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

[zieladam]

Devnote for posterity:

In #53816 the retrieve_widgets function was deprecated in favor of a new function with the same signature: sync_registered_widgets. The existing code relying on retrieve_widgets will continue to work without any PHP notices – this deprecation is merely a documentation change.

The old name retrieve_widgets suggested that the function merely reads a value. In reality, it updates the database. This discrepancy was confusing for developers. The new name reflects the actual behavior more accurately.

That looks great to me!

[zieladam]

Restoring the commit keyword (for both patches attached to this ticket)

[hellofromTonya]

Confusion. This ticket is an epic. The ticket associated with ​PR 1524 is #53811 (though it's also tagged to this epic ticket). Removing the commit keyword to move the PR's consideration to its specific ticket.

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

[hellofromTonya]

Some changes needed in ​PR 1525 for consistency and coding standards.

Marking that PR for testing for both block and classic widgets. Tomorrow before 5.8.1-RC, will do thorough testing and ask the Test Team to test it too.

@hellofromtonya I committed your suggestions – thank you! There were also some conflicts that I resolved. This should be good to go!

[zieladam]

#53660 was marked as a duplicate.

## Test Report

Env:

• OS: macOS Big Sur 11.5.2
• Browser: Chrome 92.0.4515.159 and Firefox 91.0.2
• Local testing: wp-env (npm/Docker)
• Theme: Twenty Twenty-One
• Plugins: Classic Widgets (activated during that part of the testing)

### Testing Instructions

Step 1: Set up the environment:

• Pull the latest master
• Build and start

npm install
npm run build
npm run env:start
npm run env:install

• Log into back-end ​http://localhost:8889/wp-admin
• Activate Classic Widgets plugin
• Go to Appearance > Widgets

Step 2: Interact with widgets and observe: add new ones, reorder, and move some to Inactive.

Expectation:

• Add -> adds the new widget. When leaving and returning to the screen, widget is retained
• Reorder -> widgets are reordered and order is retained when leaving screen
• Make inactive -> moves the widget to the Inactive Widgets (no long in the Footer sidebar) and retains widget when leaving screen

Step 3: Interact with widgets in Customizer

Expectations:

• Add -> adds new widget and retains it when closing Customizer
• Reorder -> widgets are reordered and order is retained when closing Customizer
• Make inactive (delete) -> widget is removed from Customizer; when closing Customizer, widget appears in the Inactive Widgets area.

Step 4: Apply this PR and rebuild

npm run grunt patch:https://github.com/WordPress/wordpress-develop/pull/1525
npm run build

Step 5: Repeat Step 2 and 3 above

Expectations: same expectations as above

### Results

All tests worked as expected ✅

No change when applying the patch ✅

[desrosj]

Going to punt this to 5.8.2 so that it can be considered at the same time as #53811.

Related: #1578

[spacedmonkey]

Related: #53915

@hellofromtonya @desrosj I believe all the feedback on this one is now addressed.

[desrosj]

Commented on the PR, but punting this one to 5.9 since it assumes sync_registered_widgets() exists in Core, and that is being discussed further in #53811.

[zieladam]

Making a note here that ​1578 introduces more retrieve_widgets calls and is worth looking into in case we stumble upon some ​non-deterministic saving issues. To be perfectly clear – it looks good and works great in my testing. This is just a contingency note.

[hellofromTonya]

Today is 5.9 Feature Freeze. As this needs a refresh and likely more eyes on it, moving to the next cycle. But the next milestone is not yet available to set. Boo. So setting it to Future Release (sorry about that). Once available, please move into 6.0.

[costdev]

With Beta 1 released yesterday, I'm moving this ticket to the 6.1 milestone.

Are we still onboard with this? @desrosj #53811 landed in the mean time so this can move forward?

I'm thinking that since #53811 has been punted to the 5.9 release, the same should be done for this one. Especially since the sync_registered_widgets() function is being discussed further in Core-53811 and this PR assumes that the function is present in Core.

I reverted to the old, retrieve_widgets name that we ended up continuing to use in core.

I also noted inline that I'm apprehensive to make a change to a function's signature in a minor release unless it cannot be avoided.

Snap, we've missed the 6.0 train with this one. Would you be willing to reconsider including this patch in 6.0.1, or do you want to wait to 6.1?

The single failing test is unrelated to this PR:

Resolved .nvmrc as 14
Found in cache @ /opt/hostedtoolcache/node/14.19.3/x64
/opt/hostedtoolcache/node/14.19.3/x64/bin/npm config get cache
/home/runner/.npm
Error: getCacheEntry failed: Cache service responded with 503

Would you be willing to reconsider including this patch in 6.0.1, or do you want to wait to 6.1?

As this is an enhancement and part of a refactoring, it could not go into a minor, but is a candidate for a major such as 6.1. Minors are for bug fixes and security.

@hellofromtonya does it need to be added to any list, or do we leave it here and it will get discovered when the time comes?

Great question @adamziel (and Hello 👋 ). The Trac ticket is already in the 6.1 milestone. And you've refreshed this PR and discussion. As it's in the milestone, it'll also be part of the 6.1 scrub/triage process once the release squad is formed.

Is the updated PR ready for feedback and test report(s)? If yes, some tips: (a) remove the needs-refresh keyword and add a comment in Trac that it's ready and (b) drop the PR into the core-test channel for wider testing and test report(s).

[zieladam]

I believe the related PR, ​https://github.com/WordPress/wordpress-develop/pull/1525, is now ready for re-review and, hopefully, also good to be merged during the next major release (6.1).

Thank you @costdev ! I just applied the changes you suggested

Thank you @costdev ! I just applied the changes you suggested

@adamziel Thanks! There's also the message parameters that need to be added to ​the assertions in the test. See ​my review comment on this (step 3).

All PHPUnit assertions, as well as all WordPress custom assertions, allow for a $message parameter to be passed. This message will be displayed when the assertion fails and can help immensely when debugging a test. This parameter should always be used if more than one assertion is used in a test method. ​Handbook Reference

@costdev Ah good note! I just added these messages and committed your suggestion.

[desrosj]

This one hasn't been touched in 3 months. With beta 1 in less than 24 hours, I'm apprehensive to rework how widgets read/write. I'm going to punt and let's try again in 6.2. If another committer has the time and is willing to own this one for 6.1, it can be moved back.

Also changing back to needs-dev-note, as has-dev-note should only be used once the dev note has been published on the Make blog.

[audrasjb]

I updated the @since mention directly in the pull request.
Tests are passing.

The patch would benefit from another test/review before commit :)

Thanks @adamziel, Left nit-pick feedback.

Thanks @mukeshpanchal27!

[desrosj]

With the beta 1 deadline for 6.2 fast approaching (less than an hour from now), I'm going to punt this. Since it's been a really long time since I've taken a look at this, there's not enough time for me to properly review and test this before the deadline.

If another committer feels more confident and would like to land this, please feel free to move it back and do so.

[mrinal013]

This ticket was discussed during the recent bug scrub. needs-testing-info keyword was added.