Repository navigation
PA - VideoPress: carry former names on widget types - #52874
Conversation
the default-layout policy asks the widget registry once it can answer
phan cannot see past the finally
…ity-videopress-layout-validation # Conflicts: # projects/packages/premium-analytics/docs/dashboard-widgets.md
a renamed type keeps the layouts saved under its old name; contract 1.1.0
retrofox
added
Enhancement
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Package] Premium Analytics
[Tests] Includes Tests
Docs
labels
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 6 files. Only the first 5 are listed here.
1 file is newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
a Widget_Type object as type dropped the whole sections route on PHP 8
…ity-videopress-former-names # Conflicts: # projects/packages/premium-analytics/docs/dashboard-sections.md # projects/packages/premium-analytics/src/dashboard-layout.php # projects/packages/premium-analytics/tests/php/Dashboard_Layout_Test.php
…ity-videopress-former-names
build the broken instances by hand for phan; cover the non-instance branch and the sanitizer
retrofox
added
[Status] Needs Review
Nikschavan
left a comment
Nikschavan
left a comment
There was a problem hiding this comment.
Thank you for the work here. I left two comments inline. The first one is a crash of the whole dashboard when former_names is not a list, so I think it needs a look before this merges.
What I checked: I registered demo/the-new-clicks with former_names set to array( 'demo/clicks' ) and stored a Traffic layout with a demo/clicks instance. The dashboard rendered that instance as "Demo the new clicks", and the stored preference kept the old name.
| } | ||
|
|
||
| $former_names = $widget_type ? $widget_type->former_names : ( $args['former_names'] ?? null ); | ||
| if ( null !== $former_names && ! $this->former_names_are_free( $name, $former_names ) ) { |
There was a problem hiding this comment.
Only the manifest path runs former_names through sanitize_widget_former_names(). A direct register_widget_type() call with array_unique( array( 'demo/clicks', 'demo/clicks', 'demo/older-clicks' ) ) passes former_names_are_free(), because it checks values only. widget-modules.php#L84 then serializes the names as {"0":"demo/clicks","2":"demo/older-clicks"}, and the for…of in buildWidgetTypeRenames() throws "object is not iterable" and takes down the whole dashboard. Normalizing the list in register() would cover both routes. Should the registry own that normalization instead of the manifest helper?
There was a problem hiding this comment.
Yes, the registry owns it now (7dd1ca8): register() normalizes the declared list to distinct values with list keys before validating and stores that list on the type, so array_unique() leftovers and the direct register_widget_type() path both serialize as ["demo/clicks", "demo/older-clicks"].
sanitize_widget_former_names() is gone from the manifest helper, so both routes are strict; a non-string former name is refused with a _doing_it_wrong(), where the old check let it through silently (a second bug behind yours).
The client also stops trusting the shape: buildWidgetTypeRenames() skips a record whose former_names is not an array, so a bad payload renames nothing instead of taking the dashboard down.
| } | ||
| return resetCount ? [ ...sectionDefault ] : sectionDefault; | ||
| }, [ sectionLayouts, activeSectionId, sectionDefault, resetCount ] ); | ||
| const fallback = resolveLayoutTypes( sectionDefault, renames ); |
There was a problem hiding this comment.
The section default reaches the client already renamed by resolve_former_widget_types_in_default_layout(). When the registry cannot answer there, the widget-modules records are empty too, so this rename has nothing to map. With const fallback = sectionDefault; the dashboard suite still passes. Is there a path where the default arrives with an old name, or could this rename apply to the stored layout only?
There was a problem hiding this comment.
Right 👍
There is no such path: boot_routes() loads widget-modules.php, and with it widget-types.php, for the sections route too, so whenever the registry can answer the default reaches the client already renamed at priority 99, and when it cannot, the records are empty as well.
Same commit: the hook renames the stored layout only, and the default passes through as the record served it.
a keyed list reached the client as an object; the client renames stored layouts only
…ity-videopress-former-names
…ity-videopress-former-names
Nikschavan
left a comment
Nikschavan
left a comment
There was a problem hiding this comment.
Thank you, the changes look good.
github-actions
Bot
added
[Status] UI Changes
2 participants
Fixes WOOA7S-2200
Proposed changes
Step 2 of the VideoPress consumer stack (WOOA7S-2221), after #52866.
A layout item references its widget type by name, and the dashboard looks the type up by exact name. Renaming a type turns every persisted instance into a "Widget is no longer available" tile, and every move of a widget to its owner package is a rename (
jpa/xtoowner/x).The Ads move got away with it because Ads sits outside the customer preview. Top videos sits in Traffic, so the rename needs a bridge first.
The type declares its former names
Widget_Typegainsformer_names. A type declares them inwidget.json, in the$args['former_names']map ofregister_widget_types_from_manifest()(current name to former names), or in the arguments ofregister_widget_type().The registry keeps the former-to-current map.
get_registered()answers for both names,resolve_name()gives the current one, andregister()refuses a former name that a registered type or another type's former name already holds, and a name that is someone's former name. Unregistering releases them.WIDGET_API_VERSIONmoves to 1.1.0: a consumer can rely on something new, and nothing built against 1.0 breaks.The record and the client
GET /wpcom/v2/widget-modulespublishesformer_nameson each record.The dashboard stage builds the rename map from the records and hands it to
useDashboardSectionLayout(), which renames a stored layout's items on the way out. There is no write-back: the stored preference keeps the old name until the section's next commit, which persists the current one. A layout with nothing to rename keeps its identity, so staged edits are untouched.The default layouts
resolve_former_widget_types_in_default_layout()runs onjetpack_premium_analytics_dashboard_default_layoutat priority 99, ahead of the unregistered-type check of #52866, so a plugin that still adds an instance under the old name keeps it under the current one.Both callbacks read the registry through
get_answering_widget_type_registry(), the guard #52866 introduced. They also treat a non-stringtypeas an unknown type and drop it, instead of lettingisset()throw aTypeErrorthat takes the whole sections route down on PHP 8. Raised by @louwie17 on #52866:register_widget_type()returns aWidget_Type, so passing that object as the type of a default instance reads naturally.Not here
A rename that also changes attributes needs a migration, the
deprecatedequivalent of blocks; nothing offers it yet.@wordpress/widget-primitivesknows neither former names nor deprecations, which is the upstream ask.The Ads package can declare
jpa/wordads-*as former names in a follow-up.Related product discussion/links
Linear: WOOA7S-2221 for the stack, WOOA7S-2200 for this step. Umbrella #52867.
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Set up
The build matters: the PHP side applies without one, but the rename of a stored layout happens in the dashboard's JS.
Use a connected site as an administrator, with a mu-plugin you can edit between steps.
1. Save an instance of a widget type
Register a widget type,
demo/clicks, reusing the bundled Clicks modules:Add the Demo clicks widget to a section and save. The instance renders.
2. Break it
Comment the registration out and hard refresh. The instance is now "Widget is no longer available": the stored layout names a type nobody registers.
3. Register the type under a new name
Register
demo/the-new-clickswith the same modules and no former names yet:The instance stays broken, and the new type shows up in the inserter. Same as trunk so far.
4. Declare the former name
Add
'former_names' => array( 'demo/clicks' ),to that registration and hard refresh.The instance saved as
demo/clicksrenders again as "Demo the new clicks".The stored preference still says
demo/clicks; the stage renamed it on the way out.Move any widget and save: from then on, the stored
typeisdemo/the-new-clicks.Screen.Recording.2026-09-28.at.6.34.52.PM.mov
5. The record
GET /wp-json/wpcom/v2/widget-moduleslistsdemo/the-new-clickswith"former_names": ["demo/clicks"]and nodemo/clicksrecord.6. A default layout under the old name
Add
demo/clicksas a Traffic default instance throughjetpack_premium_analytics_dashboard_default_layoutand reset the section. The default carriesdemo/the-new-clicks.