Repository navigation
Premium Analytics: carry declarative widget actions and icons through the registry - #51925
Conversation
a local build/ redeclared the stub on first use
ports the Gutenberg v23.8.0 sanitizers; widget-local hrefs never resolve
drops the icon reference the story cannot resolve, as useWidgetTypes does
a name that sanitizes to nothing becomes true; only false navigates
|
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! |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 2 files.
|
tools/replace-next-version-tag.sh only rewrites the version token in a single-line call, so the build failed on the token
kangzj
left a comment
There was a problem hiding this comment.
Tests well for me 👍 The port matches the upstream copy pretty much line by line (minus the widget-local file branch, which makes sense since widgets/ doesn't ship), and the whole pipeline works end to end on my docker site, so approving. Left a couple of small non-blocking notes inline.
One more while you are there, not caused by this PR: Widget_Modules_Test still calls get_widget_modules_response() without the manifest path filter, so on its own with a local build/ around it fatals with Cannot redeclare jpa_get_registered_widget_modules() (composer phpunit -- --filter Widget_Modules_Test), and the full suite only passes because Widget_Metadata_Test sorts first and flips the static $ready. The same filter you added to the REST test would fix it, maybe as a shared helper.
Tested on my dev docker site (connected, through the JT tunnel) with jetpack build plugins/premium-analytics --deps on the branch:
| What | What happened | Status |
|---|---|---|
jp test php packages/premium-analytics |
green, Widget_Metadata_Test 16 tests / 51 assertions |
✅ |
phpcs on the changed PHP, eslint + tsgo --noEmit on the package |
clean | ✅ |
| Build on the branch | manifest emits 'icon' => null / 'actions' => null for all 81 entries |
✅ |
| Premium Analytics dashboard on the PR build | renders as on trunk, 11 tiles, no per-tile "More" menu anywhere | ✅ |
GET /wp-json/wpcom/v2/widget-modules while logged in |
200, 49 records, every one has icon: null and actions: null |
✅ |
Demo: icon + 5 actions in top-posts/widget.json, rebuild |
record carries the 3 that should survive; download: "My Report.csv" comes out as My-Report.csv, openInNewTab: 1 as true, a bad icon / relevance: urgent drop the key but keep the action |
✅ |
Unsafe hrefs (javascript:alert(1), relative report.csv) |
both dropped, two _doing_it_wrong() notices from sanitize_widget_actions (captured on doing_it_wrong_run, also show up in the QM panel) |
✅ |
| Top pages tile with the demo actions | "More" menu shows View all / Export CSV ↗ / Bad icon, and "View all" opens the Posts & Pages report as a full page load | ✅ |
Widget-level icon: core/chart-bar |
reaches the record, the tile keeps its module icon since no resolver is registered | ✅ |
createStoryWidgetType() with actions |
maps id / label / href / relevance / download / openInNewTab, drops icon, and no actions key when nothing is declared |
✅ |
| Revert widget.json, rebuild | records back to all-null | ✅ |
Dashboard on the PR build (no visual diff vs trunk by design), then the demo menu and where "View all" lands:
Thanks Damián!
| if ( str_contains( $href, '..' ) ) { | ||
| return ''; | ||
| } | ||
|
|
||
| $path_only = preg_split( '/[?#]/', $href, 2 )[0]; | ||
| if ( str_ends_with( strtolower( $path_only ), '.php' ) ) { | ||
| // Single-segment admin entry points stay as-is. Deeper relative | ||
| // paths would come out of `esc_url_raw()` as `http://` URLs. | ||
| return str_contains( $path_only, '/' ) ? '' : $href; |
There was a problem hiding this comment.
Couple of things I hit poking at this on my site, both non-blocking since nothing declares actions yet: export_report.php?type=csv comes back as http://export_report.php?type=csv (esc_url_raw() only exempts [a-z0-9-]+.php, this check lets anything .php through), and any : in the query of a relative admin href gets read as a scheme by kses and the action is dropped with the notice - e.g. admin.php?page=jetpack-premium-analytics-wp-admin&p=/reports/posts?from=2026-08-05T00:00:00.000+00:00, while the %3A encoded form survives. Same story for .. in the query (range=2024-01-01..2024-01-31) tripping the traversal check, since it runs on $href rather than $path_only.
I think anchoring the single-segment entry points through admin_url() and moving the .. check onto $path_only would cover all three, at the cost of the record carrying absolute admin URLs (and the tests wanting the absolute form). Up to you whether it's worth doing here or in the follow-up that adds the first real action.
There was a problem hiding this comment.
Thanks, all three reproduce as described. Nothing declares an action yet and the fix changes the href shape the links capability will match, so I filed it as a follow-up with your admin_url() + $path_only proposal, plus the upstream report since the resolver is identical there. The Widget_Modules_Test fatal and the WidgetActionRecord reuse are in the two latest commits.
| * registered icon name the story cannot resolve and `relevance` widened to | ||
| * `string` by the JSON import. | ||
| */ | ||
| interface StoryWidgetManifestAction { |
There was a problem hiding this comment.
I feel like WidgetActionRecord from widget-primitives already is this shape (minus relevance being widened by the JSON import), so sth like Omit< WidgetActionRecord, 'relevance' > & { relevance?: string } plus a { ...action, icon: undefined, relevance: ... } spread might save re-typing it here, and a key upstream adds later wouldn't silently go missing in stories. Not a big deal tho.
Widget_Modules_Test alone required a local build/ and fataled on the redeclared stub
spread the manifest action instead of re-typing its wire shape



Proposed changes
The package keeps its own copy of Gutenberg's experimental dashboard-widgets backend under
src/(registry, sanitization, translation, the/widget-modulesrecord).This syncs that copy with the widget contract the bundled
@wordpress/widget-dashboardand@wordpress/widget-primitives0.6.0 (#51701) read from the record, so widgets can start declaring actions and icon references inwidget.json.Widget_Typegainsiconandactions, andregister_widget_types()copies both from the wp-build manifest, which already emits them.sanitize_widget_actions()keepsid,labelandhref(throughesc_url_raw()) plus the optionaldownload,openInNewTab,iconandrelevance(high,mediumorlow) keys. Incomplete or unsafe entries are dropped, unsafe hrefs with a_doing_it_wrong()notice. Adownloadname that sanitizes to nothing keeps the download under the original name; onlyfalsenavigates.resolve_widget_action_href()admits absolute, scheme-relative, root-relative and single-segment admin.phphrefs. Unlike upstream, widget-local files never resolve:widgets/does not ship with the package.sanitize_widget_icon()constrains an icon reference tocollection/icon-name.Action labels are translated server-side under the
widget action labelcontext (widget-i18n.json).The REST record carries
iconandactions. Both arenullwhen the widget declares none, which is every widget today, so the dashboard renders exactly as before.createStoryWidgetType()mapsactionsfromwidget.json, so a story can exercise a widget that declares them.Out of scope
The client already knows how to promote
highandmediumactions into a footer strip and how to turn an action into a router link through the hostlinkscapability.Neither is adopted here: no widget declares an action yet, and how the footer scales across the dashboard's tile sizes is still an open question. Declarative icon references also stay inert on the client until the package registers an icon resolver. Each of those is a separate follow-up.
Related product discussion/links
hrefsanitization WordPress/gutenberg#80510iconthrough the widget pipeline WordPress/gutenberg#80969Does this pull request change what data or activity we track or use?
No.
Testing instructions
Run the package tests:
jp test php packages/premium-analytics.Widget_Metadata_Testcovers hydration, action and icon sanitization, translation, and the REST record.Build the branch (
jetpack build --deps packages/premium-analytics) and open Premium Analytics on a connected site: the dashboard renders as on trunk, since no widget declares actions or an icon.While logged in, request
/wp-json/wpcom/v2/widget-modules: every record carriesicon: nullandactions: null.widgets/top-posts/widget.json, rebuild, and reload. The record carries the action, and the Top pages tile shows a "More" menu whose "View all" item opens the Posts report (a full page load, since this PR does not provide the hostlinkscapability). Revert before merging.