Make WordPress Core

Opened 3 years ago

Last modified 8 months ago

#53816 new enhancement

Overview: Refactor the widgets read/write logic

Reported by: zieladam's profile zieladam Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch has-unit-tests needs-docs needs-testing needs-dev-note needs-testing-info
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 (61)

#1 @zieladam
3 years ago

  • Description modified (diff)

#2 @zieladam
3 years ago

  • Description modified (diff)

#3 @zieladam
3 years ago

  • Description modified (diff)

#4 @zieladam
3 years ago

  • Description modified (diff)

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


3 years ago
#5

  • Keywords has-patch has-unit-tests added

#7 @hellofromTonya
3 years 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.

adamziel commented on PR #1525:


3 years ago
#8

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

draganescu commented on PR #1524:


3 years ago
#9

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 ?

draganescu commented on PR #1525:


3 years ago
#10

This looks good now IMO

adamziel commented on PR #1524:


3 years ago
#11

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

draganescu commented on PR #1524:


3 years ago
#12

Thanks for renaming @adamziel. LGTM now!

#13 @andraganescu
3 years 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.

TimothyBJacobs commented on PR #1524:


3 years ago
#14

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.

adamziel commented on PR #1524:


3 years ago
#15

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!

TimothyBJacobs commented on PR #1524:


3 years ago
#16

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 years ago

  • Keywords commit removed

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

adamziel commented on PR #1524:


3 years ago
#18

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

#19 @zieladam
3 years 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.

TimothyBJacobs commented on PR #1524:


3 years ago
#20

That looks great to me!

#21 @zieladam
3 years 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 years 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 years 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.

adamziel commented on PR #1525:


3 years ago
#25

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

#26 @zieladam
3 years ago

#53660 was marked as a duplicate.

hellofromtonya commented on PR #1525:


3 years ago
#27

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

spacedmonkey commented on PR #1525:


3 years ago
#29

Related: #1578

adamziel commented on PR #1525:


3 years ago
#31

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

#32 @desrosj
3 years 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
3 years 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 3 years ago by zieladam (previous) (diff)

#34 @hellofromTonya
3 years 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.

#35 @andraganescu
2 years ago

  • Milestone changed from Future Release to 6.0

#36 @costdev
2 years ago

  • Milestone changed from 6.0 to 6.1

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

draganescu commented on PR #1525:


2 years ago
#37

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

adamziel commented on PR #1525:


2 years ago
#38

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?

adamziel commented on PR #1525:


2 years ago
#39

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

hellofromtonya commented on PR #1525:


2 years ago
#40

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.

adamziel commented on PR #1525:


2 years ago
#41

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

hellofromtonya commented on PR #1525:


2 years ago
#42

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

#43 @zieladam
2 years ago

  • Keywords needs-refresh removed

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

adamziel commented on PR #1525:


2 years ago
#44

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

costdev commented on PR #1525:


2 years ago
#45

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

adamziel commented on PR #1525:


2 years ago
#46

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

#47 @desrosj
21 months ago

  • Keywords needs-dev-note added; has-dev-note removed
  • Milestone changed from 6.1 to 6.2

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.

#48 @audrasjb
17 months ago

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

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

@mukesh27 commented on PR #1525:


16 months ago
#49

Thanks @adamziel, Left nit-pick feedback.

@zieladam commented on PR #1525:


16 months ago
#50

Thanks @mukeshpanchal27!

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


16 months ago

#52 @desrosj
16 months ago

  • Milestone changed from 6.2 to 6.3

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.

This ticket was mentioned in Slack in #core by oglekler. View the logs.


12 months ago

#54 @mrinal013
12 months ago

  • Keywords needs-testing-info added

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

Last edited 12 months ago by mrinal013 (previous) (diff)

#55 @oglekler
11 months ago

It still needs to be tested, and hopefully @since will be updated only one more time.

#56 @audrasjb
11 months ago

  • Milestone changed from 6.3 to 6.4

Still needs testing. Moving to 6.4 as WP 6.3 beta 1 is going to be released in a couple days.

#57 @oglekler
9 months ago

Hi @zieladam, can you please provide the steps for testing the patch?

This ticket was mentioned in Slack in #core by oglekler. View the logs.


9 months ago

#59 @mukesh27
9 months ago

This ticket was discussed during the recent bug scrub, and we are still awaiting the testing steps for the patch. @zieladam, could you please provide them?

We are only a few weeks away from the beta release.

#60 @oglekler
8 months ago

  • Milestone changed from 6.4 to Future Release

Because there is no actual development in 8 months and Beta 1 is close, I am moving this ticket into Future Release. When it will be ready for review, it can be rescheduled to the next available milestone.

#61 @zieladam
8 months ago

@oglekler @mukesh27 sorry for the radio silence, I was on a sabbatical. The testing steps are listed in the Pull Request on GitHub: https://github.com/WordPress/wordpress-develop/pull/1525 I would rather not copy&paste them as there's a lengthy code snippet involved.

Note: See TracTickets for help on using tickets.