Skip to content

Forms: Fix import map ordering for wp-build boot script#47824

Closed
enejb wants to merge 3 commits intotrunkfrom
fix/event-calendar-cfm
Closed

Forms: Fix import map ordering for wp-build boot script#47824
enejb wants to merge 3 commits intotrunkfrom
fix/event-calendar-cfm

Conversation

@enejb
Copy link
Copy Markdown
Member

@enejb enejb commented Mar 27, 2026

Fixes FORMS-667

See also potential upstream fix:

Proposed changes

  • Fix TypeError: Failed to resolve module specifier '@wordpress/boot' on the Jetpack Forms wp-build admin page.
  • In wp-admin, _wp_footer_scripts (classic scripts) and print_import_map (script modules) both hook into admin_print_footer_scripts at priority 10, but _wp_footer_scripts is registered first. The inline import("@wordpress/boot") call — attached to a classic script via wp_add_inline_script — executes before the import map is printed, so the browser cannot resolve the bare specifier.
  • The fix intercepts the inline script at admin_enqueue_scripts time, removes it from the classic script handle, and re-emits it as a <script type="module"> at admin_print_footer_scripts priority 11 (after the import map).

Other information

  • Generate changelog entries for this PR (using AI).

Related product discussion/links

  • This issue was surfaced when The Events Calendar plugin was active, but the root cause is a WordPress core hook ordering issue that affects all sites using wp-build pages in wp-admin.
    See p1774633136941859-slack-C086RGTJT1D

Does this pull request change what data or activity we track or use?

No.

Testing instructions

  1. Install and activate The Events Calendar plugin (or any plugin that enqueues scripts globally in wp-admin).
  2. Enable the wp-build forms dashboard (jetpack_forms_alpha filter must return true).
  3. Go to Jetpack → Forms in wp-admin.
  4. Before this fix: The page shows a blank content area with TypeError: Failed to resolve module specifier '@wordpress/boot' in the browser console.
  5. After this fix: The Forms page renders correctly with the full table of forms, tabs, search, and pagination.

In wp-admin, _wp_footer_scripts and print_import_map both hook into
admin_print_footer_scripts at priority 10, but _wp_footer_scripts is
registered first. This causes the inline import("@wordpress/boot") call
(attached via wp_add_inline_script to a classic script) to execute before
the import map is available, resulting in "Failed to resolve module
specifier '@wordpress/boot'".

The fix intercepts the inline script at admin_enqueue_scripts time,
removes it from the classic script handle, and re-emits it as a
<script type="module"> at admin_print_footer_scripts priority 11
(after the import map).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@enejb enejb added the [Status] Needs Review This PR is ready for review. label Mar 27, 2026
@enejb enejb self-assigned this Mar 27, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 27, 2026

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WoA dev site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin (Jetpack), and enable the fix/event-calendar-cfm branch.
  • To test on Simple, run the following command on your sandbox:
bin/jetpack-downloader test jetpack fix/event-calendar-cfm

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@github-actions
Copy link
Copy Markdown
Contributor

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ Add a "[Status]" label (In Progress, Needs Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Make sure to test your changes on all platforms that it applies to. You're responsible for the quality of the code you ship.
  3. You can use GitHub's Reviewers functionality to request a review.
  4. When it's reviewed and merged, you will be pinged in Slack to deploy the changes to WordPress.com simple once the build is done.

If you have questions about anything, reach out in #jetpack-developers for guidance!

… improve tests

- Add page guard so the fix only runs on the Forms admin page
- Use wp_print_inline_script_tag instead of raw echo for proper escaping
- Add setUp to reset wp_scripts before each test
- Add test verifying the fix does not run on other admin pages

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@enejb enejb requested review from a team and Copilot March 27, 2026 19:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a wp-admin hook ordering issue that can cause wp-build pages (Jetpack Forms dashboard) to execute an inline import("@wordpress/boot") before WordPress prints the import map, leading to “Failed to resolve module specifier” errors.

Changes:

  • Add a dashboard-level hook to move the inline @wordpress/boot import from a classic script’s inline after block into a module script printed after the import map.
  • Add PHP unit tests covering the boot import-map ordering behavior and no-op scenarios.
  • Add a changelog fragment describing the fix.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
projects/packages/forms/src/dashboard/class-dashboard.php Implements the runtime fix by extracting the inline boot import and re-printing it as a module script later in the footer.
projects/packages/forms/tests/php/dashboard/Dashboard_Boot_Import_Map_Test.php Adds tests validating the inline-script extraction/re-emission behavior and guard conditions.
projects/packages/forms/changelog/fix-event-calendar-cfm Adds a changelog entry for the wp-admin import map ordering fix.

Comment on lines +82 to +85
$current_screen = get_current_screen();
$is_forms_page = (
( isset( $_GET['page'] ) && self::FORMS_WPBUILD_ADMIN_SLUG === $_GET['page'] ) || // phpcs:ignore WordPress.Security.NonceVerification.Recommended
( $current_screen && 'jetpack-forms-responses' === $current_screen->id )
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix_boot_import_map_ordering() calls get_current_screen() unconditionally inside the admin_enqueue_scripts callback. In this repo’s WorDBless-based unit tests (see Dashboard::is_jetpack_forms_admin_page() which guards get_current_screen()), get_current_screen() can be undefined, which will cause a fatal when the hook fires. Guard with function_exists( 'get_current_screen' ) (or avoid calling it entirely and rely on self::get_admin_query_page()), and if you keep the screen check, use the correct screen id (jetpack_page_ . self::FORMS_WPBUILD_ADMIN_SLUG) rather than 'jetpack-forms-responses' so the condition works in wp-admin.

Suggested change
$current_screen = get_current_screen();
$is_forms_page = (
( isset( $_GET['page'] ) && self::FORMS_WPBUILD_ADMIN_SLUG === $_GET['page'] ) || // phpcs:ignore WordPress.Security.NonceVerification.Recommended
( $current_screen && 'jetpack-forms-responses' === $current_screen->id )
$current_screen = function_exists( 'get_current_screen' ) ? get_current_screen() : null;
$is_forms_page = (
( isset( $_GET['page'] ) && self::FORMS_WPBUILD_ADMIN_SLUG === $_GET['page'] ) || // phpcs:ignore WordPress.Security.NonceVerification.Recommended
( $current_screen && 'jetpack_page_' . self::FORMS_WPBUILD_ADMIN_SLUG === $current_screen->id )

Copilot uses AI. Check for mistakes.
@enejb
Copy link
Copy Markdown
Member Author

enejb commented Mar 27, 2026

@claude can you review this PR as well?

@claude
Copy link
Copy Markdown

claude bot commented Mar 27, 2026

Claude finished @enejb's task in 2m 7s —— View job


PR Review

  • Gather context and understand the request
  • Read changed files
  • Analyze implementation logic
  • Review tests
  • Post review feedback

The approach is sound and directly addresses a real WordPress core hook ordering limitation. Here are my findings:

Bug: Wrong screen ID (dead code)

class-dashboard.php:85

( $current_screen && 'jetpack-forms-responses' === $current_screen->id )

This condition can never be true. For the FORMS_WPBUILD_ADMIN_SLUG ('jetpack-forms-responses-wp-admin'), WordPress generates a screen ID of 'jetpack_page_jetpack-forms-responses-wp-admin' — not 'jetpack-forms-responses'. The correct pattern is already used in the existing is_jetpack_forms_admin_page() at line 651:

'jetpack_page_' . self::FORMS_WPBUILD_ADMIN_SLUG,

The $_GET['page'] check on line 84 does work correctly, so this is a silent dead-code bug rather than a functional regression. But if the screen ID branch was intentional (e.g. for callers that don't set $_GET['page']), it needs to be fixed.

Fix this →

Bug: Missing function_exists('get_current_screen') guard

class-dashboard.php:82

$current_screen = get_current_screen();

The existing is_jetpack_forms_admin_page() at line 639 guards this identically:

if ( ! function_exists( 'get_current_screen' ) ) {
    return false;
}

The new code is inconsistent with this established pattern. While WorDBless likely stubs the function (tests pass), the guard is defensive best practice throughout the codebase. The simplest fix here would be to just reuse the existing helper directly:

if ( ! self::is_jetpack_forms_admin_page() ) {
    return;
}

This is equivalent, already correct, avoids duplication, and handles both the $_GET['page'] and screen ID checks.

Fix this →

Minor: Missing test case for mixed inline scripts

The test suite covers the happy path, non-Forms pages, and the no-op scenario — but doesn't cover the case where the -prerequisites handle has other inline scripts alongside the boot import. This is important to verify that those other scripts are preserved on the classic handle and not accidentally dropped.

public function test_fix_preserves_other_inline_scripts() {
    $_GET['page'] = Dashboard::FORMS_WPBUILD_ADMIN_SLUG;
    $handle = Dashboard::FORMS_WPBUILD_ADMIN_SLUG . '-prerequisites';

    wp_register_script( $handle, '', array(), '1.0', true );
    wp_add_inline_script( $handle, 'console.log("other script");' );
    wp_add_inline_script(
        $handle,
        'import("@wordpress/boot").then(mod => mod.initSinglePage({mountId: "app", routes: []}));'
    );

    Dashboard::fix_boot_import_map_ordering();
    do_action( 'admin_enqueue_scripts', '' );

    $after = wp_scripts()->get_data( $handle, 'after' );
    $this->assertTrue(
        $this->array_contains_string( is_array( $after ) ? $after : array(), 'other script' ),
        'Non-boot inline scripts should be preserved on the classic handle.'
    );
}

Nit: PHP_INT_MAX priority

The comment explains the intent ("Run after the page's own enqueue hook"), but it's worth noting that PHP_INT_MAX is effectively a magic number here. A large but explicit constant like 99999 would be more conventional and equally effective unless there's a specific reason to guarantee running after absolutely every possible callback.


Summary

The core fix is correct and well-targeted. The two functional issues above (wrong screen ID, missing function_exists guard) are relatively low-risk since the $_GET['page'] path handles the real-world case — but they should be corrected for correctness and consistency with the rest of the codebase. The Copilot review also flagged these same issues. The changelog entry is appropriate.

$current_screen = get_current_screen();
$is_forms_page = (
( isset( $_GET['page'] ) && self::FORMS_WPBUILD_ADMIN_SLUG === $_GET['page'] ) || // phpcs:ignore WordPress.Security.NonceVerification.Recommended
( $current_screen && 'jetpack-forms-responses' === $current_screen->id )
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be checking this part at all since it doesn't apply for this case.

- Replace inline get_current_screen() + $_GET check with the existing
  is_jetpack_forms_admin_page() helper which already handles the
  function_exists guard and correct screen ID format
- Add test for preserving other inline scripts on the same handle
- Use set_current_screen() in tests instead of $_GET['page']

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jp-launch-control
Copy link
Copy Markdown

Code Coverage Summary

Coverage changed in 1 file.

File Coverage Δ% Δ Uncovered
projects/packages/forms/src/dashboard/class-dashboard.php 116/266 (43.61%) 7.74% -2 💚

Full summary · PHP report · JS report

@enejb
Copy link
Copy Markdown
Member Author

enejb commented Mar 30, 2026

Closing in favour of #47823

@enejb enejb closed this Mar 30, 2026
@github-actions github-actions bot removed the [Status] Needs Review This PR is ready for review. label Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants