WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 3 weeks ago

#53816 new enhancement

Overview: Refactor the widgets read/write logic

Reported by: zieladam Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch has-unit-tests needs-docs has-dev-note needs-testing needs-refresh
Focuses: Cc:

Description (last modified by 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

Change History (34)

#1 @zieladam
4 months ago

  • Description modified (diff)

#2 @zieladam
4 months ago

  • Description modified (diff)

#3 @zieladam
4 months ago

  • Description modified (diff)

#4 @zieladam
4 months ago

  • Description modified (diff)

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


4 months ago

  • Keywords has-patch has-unit-tests added

#7 @hellofromTonya
4 months ago

  • Component changed from General to Widgets
  • Type changed from defect (bug) to enhancement

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

#8 @prbot
4 months ago

adamziel commented on PR #1525:

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

#9 @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 ?

#10 @prbot
3 months ago

draganescu commented on PR #1525:

This looks good now IMO

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

#12 @prbot
3 months ago

draganescu commented on PR #1524:

Thanks for renaming @adamziel. LGTM now!

#13 @andraganescu
3 months ago

  • Keywords needs-dev-note needs-docs commit added
  • Milestone changed from Awaiting Review to 5.8.1

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.

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

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

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

#17 @desrosj
3 months ago

  • Keywords commit removed

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

#18 @prbot
3 months ago

adamziel commented on PR #1524:

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

#19 @zieladam
3 months ago

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.

#20 @prbot
3 months ago

TimothyBJacobs commented on PR #1524:

That looks great to me!

#21 @zieladam
3 months ago

  • Keywords has-dev-note commit added; needs-dev-note removed

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

#22 @hellofromTonya
3 months ago

  • Keywords commit removed

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.

#24 @hellofromTonya
3 months ago

  • Keywords needs-testing needs-refresh added

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.

#25 @prbot
3 months ago

adamziel commented on PR #1525:

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

#26 @zieladam
3 months ago

#53660 was marked as a duplicate.

#27 @prbot
3 months ago

hellofromtonya commented on PR #1525:

## 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

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 ✅

#28 @desrosj
3 months ago

  • Milestone changed from 5.8.1 to 5.8.2

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

#29 @prbot
3 months ago

spacedmonkey commented on PR #1525:

Related: #1578

#31 @prbot
3 months ago

adamziel commented on PR #1525:

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

#32 @desrosj
2 months ago

  • Milestone changed from 5.8.2 to 5.9

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.

#33 @zieladam
2 months ago

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.

Last edited 2 months ago by zieladam (previous) (diff)

#34 @hellofromTonya
3 weeks ago

  • Milestone changed from 5.9 to Future Release

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.

Note: See TracTickets for help on using tickets.