WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 2 months ago

#53811 closed defect (bug)

Rename `retrieve_widgets` to `sync_registered_widgets` — at Version 19

Reported by: zieladam Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch
Focuses: docs Cc:

Description (last modified by hellofromTonya)

We have a function called retrieve_widgets. The name suggests that it's a getter, yet it updates the database by assigning the orphaned widgets to an inactive sidebar.

The name is misleading and it bit us in https://github.com/WordPress/gutenberg/issues/33335#issuecomment-879903958 and #53657.

I propose we deprecate the current name and move forward with a new one which communicates the intention in a clearer way: sync_registered_widgets.

Part of the epic ticket #53816.

Change History (19)

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


4 months ago

  • Keywords has-unit-tests added

#2 @hellofromTonya
4 months ago

  • Keywords 2nd-opinion added

The retrieve_widgets function does more than retrieve hidden or lost widgets. In essence, it is rebuilding the global $sidebars_widgets by:

  • Validation and remapping widgets (in each sidebar) against the widget registry -> _wp_remove_unregistered_widgets()
  • Validation and remapping sidebars against the sidebar registry -> wp_map_sidebars_widgets()
  • Finding all hidden or lost widgets and assigning them to the inactive sidebar
  • Updating the database with the new $sidebars_widgets configuration
  • Returning $sidebars_widgets

It does retrieve all of the widgets but in that retrieval process it's doing a validation process, finding all of lost widgets, and updating state and the db.

The middle chunk of the function that finds the hidden and lost widgets could be moved to a private function named _wp_recover_lost_widgets. Then the retrieve_widgets function would be more of a task runner by invoking more specific granular tasks that are encapsulated in separate functions.

Last edited 4 months ago by hellofromTonya (previous) (diff)

#3 @zieladam
4 months ago

Then the retrieve_widgets function would be more of a task runner by invoking more specific granular tasks that are encapsulated in separate functions.

@hellofromTonya Encapsulation would surely be nice here. +1 for doing it regardless of this issue.

In this ticket I want to address the fact that the word retrieve suggest a read-only operation for something that is a read-write operation.

The retrieve_widgets function does more than retrieve hidden or lost widgets. In essence, it is rebuilding the global $sidebars_widgets by:

It is quite a complex function indeed. I don't think we can come up with a name that would accurately reflect all it does. The second best thing is making it clear that it fundamentally is a write-oriented function. remap_widgets would perhaps be an even better choice here.

As a side note, I don't fully understand why do we need to run it before reading the data instead of bundling it with writes and theme changes. Intuitively, I would expect write operations to clean up after themselves.

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

#4 @prbot
4 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 ?

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

#6 @prbot
3 months ago

draganescu commented on PR #1524:

Thanks for renaming @adamziel. LGTM now!

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

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

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

#10 @prbot
3 months ago

adamziel commented on PR #1524:

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

#11 @prbot
3 months ago

TimothyBJacobs commented on PR #1524:

That looks great to me!

#12 @desrosj
3 months ago

  • Component changed from General to Widgets

#13 @hellofromTonya
3 months ago

  • Keywords commit added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.8.1
  • Owner set to hellofromTonya
  • Status changed from new to reviewing
  • Summary changed from Rename `retrieve_widgets` to `recover_lost_widgets` to Rename `retrieve_widgets` to `sync_registered_widgets`

A new name has been selected: sync_registered_widgets. I agree this new name is more readable and understandable as to what it's purpose is. Removing the 2nd-opinion.

Moving this PR into the milestone to match its epic ticket #53816.

Timothy has already reviewed PR 1524. I'm doing a pre-commit review and then get it committed.

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


3 months ago

#15 @hellofromTonya
3 months ago

In 51705:

Widgets: Rename and soft deprecate retrieve_widgets().

The original name retrieve_widgets() was unclear as it suggested it was a getter, i.e. getting the widgets. This function does more than get: finds orphaned widgets, assigns them to the inactive sidebar, and updates the database.

The new name is sync_registered_widgets() which better represents what happens when this function is invoked.

The original retrieve_widgets() function is soft deprecated to avoid unnecessary code churn downstream for developers that support more than the latest version of WordPress.

Follow-up to [18630].

Props zieladam, timothyblynjacobs, andraganescu, hellofromTonya.
See #53811.

#16 @hellofromTonya
3 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

#17 @hellofromTonya
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging to the 5.8 branch.

#19 @hellofromTonya
3 months ago

  • Description modified (diff)
Note: See TracTickets for help on using tickets.