WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#53811 closed defect (bug) (fixed)

Rename `retrieve_widgets` to `sync_registered_widgets`

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 (33)

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


3 months ago

  • Keywords has-unit-tests added

#2 @hellofromTonya
3 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:

  • Remapping widgets (in each sidebar) against the widget registry -> _wp_remove_unregistered_widgets()
  • 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.

Version 0, edited 3 months ago by hellofromTonya (next)

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

#4 @prbot
2 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
2 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
8 weeks ago

draganescu commented on PR #1524:

Thanks for renaming @adamziel. LGTM now!

#7 @prbot
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks ago

adamziel commented on PR #1524:

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

#11 @prbot
7 weeks ago

TimothyBJacobs commented on PR #1524:

That looks great to me!

#12 @desrosj
7 weeks ago

  • Component changed from General to Widgets

#13 @hellofromTonya
6 weeks 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.


6 weeks ago

#15 @hellofromTonya
6 weeks 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
6 weeks ago

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

#17 @hellofromTonya
6 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging to the 5.8 branch.

#19 @hellofromTonya
6 weeks ago

  • Description modified (diff)

#20 @peterwilsoncc
6 weeks ago

Sorry for coming to this late but I think the docblock for the deprecated function will need to be updated.

Typically WP keeps the description of what the function does, at least in summary form. Developer.wordpress.org then picks up the warning from the docblock, in this case:

@deprecated 5.8.1 Use sync_registered_widgets()

My inclination is to hold of deprecating this until a major release (either hard or soft deprecation) as the function has been around since WP 2.8 so there isn't any urgency for limiting the use of the function in the wild.

#21 @zieladam
6 weeks ago

My inclination is to hold of deprecating this until a major release (either hard or soft deprecation) as the function has been around since WP 2.8 so there isn't any urgency for limiting the use of the function in the wild.

@peterwilsoncc pardon my ignorance but just to make sure we are on the same page – is the argument here basically keeping the retrieve_widgets docs on developer.wordpress.org for a little bit longer? I'm asking because the intention is very much to keep the function usable. nothing changes there.

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


6 weeks ago

#23 @desrosj
6 weeks ago

  • Milestone changed from 5.8.1 to 5.8.2

Some thoughts on [51705]:

  • Deprecated functions are supposed to be moved to deprecated.php. However, I'd like to confirm whether "soft" deprecations have also been moved here in the past, or if the precedent is to leave them in their original place.
  • While this does seem to make things more clear, I agree with @TimothyBlynJacobs that it's borderline refactoring for the sake of refactoring.

This isn't actually fixing anything as it pertains to the user. It's changing a function name in order to make it more clear to developers. So I'm inclined to agree it should wait till 5.9, or at least have more discussion before being reconsidered for 5.8.2.

#24 @desrosj
6 weeks ago

  • Keywords fixed-major added

#25 @hellofromTonya
6 weeks ago

For more context, here is the link to discussion thread in Make WordPress slack #core channel.

Decisions needed:

  • Soft vs hard deprecation?
    • For core: a hard deprecation helps to further resolve the function purpose confusion (which is the purpose of this ticket)
    • For extenders who do WP cross-version automated testing: a soft deprecation helps them avoid code churn; however, each hard deprecation introduces this same churn.
  • Include in 5.8.x or wait for 5.9?
    • Including in point release helps to avoid continued code churn and delays with ongoing development of and issue fixing with the block widgets. As noted in the description, the original name caused these problems
  • If soft deprecation, should the original function stay in its file or be moved to deprecated file?

#26 @azaozz
6 weeks ago

  • Milestone changed from 5.8.2 to 5.9

As far as I know only (major) bug fixes and regression fixes belong in dot releases. There are two reasons:

  1. Dot releases are usually not tested as thoroughly as major releases. The risks of introducing a bug and deploying it across millions of installs should be minimized as much as possible.
  2. The autoupdate failures rate slightly increases when the update is larger/has more files. That increase is very small as a percentage but given the size of WP it easily translates into thousands of installs.

In that terms moving to 5.9 for further consideration.

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

Perhaps a better way to fix this would be to extend and enhance the inline docs. Would be great to add some inline comments too. True, the new name is better than the old, but at the same time that function does a lot of things and the name will never be able to describe it particularly well. Also renaming and deprecating a function has some downsides like filling the PHP error logs with deprecation warnings on thousands of servers.

Last edited 6 weeks ago by azaozz (previous) (diff)

#27 @hellofromTonya
5 weeks ago

  • Keywords has-patch has-unit-tests commit removed

After discussing with @azaozz, @zieladam, and @andraganescu, decided to revert [51705]. Why?

This function is 12 years old. While the new name is much better, it doesn't fully tell us what is happens when the function is invoked. Why? The function is doing too much. And naming is hard.

This function is part of a bigger problem of globals and having to fix data when accessed/touched.

Instead of renaming this function, a suggestion could be to explore improving the widgets read/write/in-memory state API. This type of enhancement is beyond the scope of this ticket.

In the interim, @azaozz suggestion of improving the docs is an incremental quality improvement.

#28 @hellofromTonya
5 weeks ago

  • Focuses docs added
  • Keywords needs-patch added

Marking for the docs improvement.

#29 @hellofromTonya
5 weeks ago

In 51791:

Widgets: Revert [51705].

While the new name is much better, it doesn't fully tell what will happen when invoked nor does it fully solve the root problems.

Why? The function is doing too much. And naming is hard.

Props azaozz, desrosj, andraganescu, zieladam, hellofromTonya.
See #53811.

#30 @SergeyBiryukov
4 weeks ago

  • Keywords fixed-major removed

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


3 weeks ago

  • Keywords has-patch added; needs-patch removed

This PR improves the documentation of retrieve_widgets function as discussed in https://core.trac.wordpress.org/ticket/53811

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

#32 @azaozz
3 weeks ago

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

In 51842:

Update and enhance the docs for retrieve_widgets().

Props zieladam, hellofromtonya.
Fixes #53811.

#33 @SergeyBiryukov
3 weeks ago

In 51849:

Docs: Update description for retrieve_widgets() per the documentation standards.

Follow-up to [51842].

See #53811.

Note: See TracTickets for help on using tickets.