Opened 22 months ago
Last modified 35 hours ago
#53816 new enhancement
Overview: Refactor the widgets read/write logic
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 6.3 | 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 )
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 calledretrieve
, 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 (54)
This ticket was mentioned in PR #1525 on WordPress/wordpress-develop by adamziel.
22 months ago
#5
- Keywords has-patch has-unit-tests added
This ticket was mentioned in PR #1524 on WordPress/wordpress-develop by adamziel.
22 months ago
#6
Trac ticket: https://core.trac.wordpress.org/ticket/53816
Trac ticket: https://core.trac.wordpress.org/ticket/53811
#7
@
22 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.
draganescu commented on PR #1524:
22 months 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:
22 months ago
#10
This looks good now IMO
22 months 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:
22 months ago
#12
Thanks for renaming @adamziel. LGTM now!
#13
@
22 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.
TimothyBJacobs commented on PR #1524:
22 months 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.
22 months 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:
22 months 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
@
22 months ago
- Keywords commit removed
Removing the commit
keyword as there is still some ongoing discussion on the PR.
#19
@
21 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.
TimothyBJacobs commented on PR #1524:
21 months ago
#20
That looks great to me!
#21
@
21 months ago
- Keywords has-dev-note commit added; needs-dev-note removed
Restoring the commit
keyword (for both patches attached to this ticket)
hellofromtonya commented on PR #1524:
21 months ago
#23
Merged with changeset https://core.trac.wordpress.org/changeset/51705.
#24
@
21 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.
21 months ago
#25
@hellofromtonya I committed your suggestions – thank you! There were also some conflicts that I resolved. This should be good to go!
hellofromtonya commented on PR #1525:
21 months 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
- 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 ✅
#28
@
21 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.
spacedmonkey commented on PR #1525:
21 months ago
#29
Related: #1578
#32
@
21 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
@
20 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.
#34
@
19 months 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.
#36
@
14 months 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:
12 months ago
#37
Are we still onboard with this? @desrosj #53811 landed in the mean time so this can move forward?
12 months 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?
12 months 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:
12 months 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.
12 months 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:
12 months 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
@
12 months 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).
12 months 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
#47
@
9 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
@
5 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:
4 months ago
#49
Thanks @adamziel, Left nit-pick feedback.
@zieladam commented on PR #1525:
4 months ago
#50
Thanks @mukeshpanchal27!
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
4 months ago
#52
@
4 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.
Trac ticket: https://core.trac.wordpress.org/ticket/53816