Rename `retrieve_widgets` to `sync_registered_widgets`

[zieladam]

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.

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

[hellofromTonya]

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.

[zieladam]

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.

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

Thanks for renaming @adamziel. LGTM now!

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.

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!

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.

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

That looks great to me!

[hellofromTonya]

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.

[hellofromTonya]

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.

[hellofromTonya]

Reopening for merging to the 5.8 branch.

Merged with changeset https://core.trac.wordpress.org/changeset/51705.

[peterwilsoncc]

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.

[zieladam]

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.

[desrosj]

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.

[hellofromTonya]

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?

[azaozz]

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.

[hellofromTonya]

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.

[hellofromTonya]

Marking for the docs improvement.

[hellofromTonya]

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.

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

[azaozz]

In 51842:

Update and enhance the docs for retrieve_widgets().

Props zieladam, hellofromtonya.
Fixes #53811.

[SergeyBiryukov]

In 51849:

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

Follow-up to [51842].

See #53811.

Committed via https://core.trac.wordpress.org/changeset/51842.