Repository navigation
fix(admin-ui): apply page hooks to the admin_page_ fallback prefix - #52622
Conversation
When the user has no Jetpack top-level menu, WordPress registers an Admin_Menu page as admin_page_<slug>, so the core-notice hiding and the design-tokens stylesheet, both keyed on jetpack_page_<slug>, never ran. Register both prefixes.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
Mention offline mode in the Jetpack changelog entry too.
xavier-lc
left a comment
There was a problem hiding this comment.
The fix looks right to me: admin_page_ is only used when the Jetpack plugin skips its top-level menu for a non-admin, which covers both the disconnected and offline cases, so adding the changelog entry to the Jetpack plugin alone is correct. I have two small test cleanups inline and nothing blocking.
| // test_page_suffix_matches leaves a `jetpack` entry behind, which would give core's jetpack_page_ name. | ||
| global $admin_page_hooks; | ||
| unset( $admin_page_hooks['jetpack'] ); |
There was a problem hiding this comment.
This depends on which tests ran first. setUp() already resets $menu, $submenu, $_parent_pages and $_registered_pages. Adding $admin_page_hooks = array(); there would let you drop this unset and the comment that names another test.
There was a problem hiding this comment.
Good call, I moved the $admin_page_hooks reset into setUp() and dropped the unset and its comment. Once the hooks reset for every test, test_page_suffix_matches also needed the menu registered per data set, so that's in 5e489d3 too.
| wp_set_current_user( | ||
| wp_insert_user( | ||
| array( | ||
| 'user_login' => 'fallback_editor', | ||
| 'user_pass' => 'pass', | ||
| 'role' => 'editor', | ||
| ) | ||
| ) | ||
| ); | ||
|
|
There was a problem hiding this comment.
This user is never deleted. wp_set_current_user( self::$editor_user_id ); reuses the class's editor, the same way the neighbouring tests do.
There was a problem hiding this comment.
Done, it now uses self::$editor_user_id like the neighbouring tests.
xavier-lc
left a comment
There was a problem hiding this comment.
Thanks, both addressed. LGTM.
Proposed changes
Admin_Menu::add_menu()now registers each page's hooks under bothjetpack_page_<slug>andadmin_page_<slug>. WordPress uses theadmin_page_name when a user can open the page but has no Jetpack top-level menu, for example an editor on a site that is not connected yet while the Jetpack plugin is active. The core-notice hiding and the design-tokens stylesheet now load on that page too. The return value ofadd_menu()is unchanged.admin_page_hook.Bundles that change (every plugin that includes admin-ui): agents-manager, automattic-for-agencies-client, backup, beta, boost, classic-theme-helper-plugin, inspect, jetpack, mu-wpcom-plugin, paypal-payment-buttons, premium-analytics, protect, search, social, starter-plugin, stats, videopress, wpcloud-sso, wpcomsh.
Does this pull request change what data or activity we track or use?
No.
Testing instructions
wp-admin/admin.php?page=my-jetpack. The body class isadmin_page_my-jetpack. Before this change, neitherjetpack-admin-ui-design-tokens-cssnorjetpack-admin-ui-hide-core-notices-inline-cssis in the page; after it, both are.jetpack_page_my-jetpackand has both styles before and after; screenshots are byte-identical.