Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
8bc861d to
6a90a3c
Compare
…lbacks. `WP_Script_Modules::add_hooks()` registered every print callback as `array( $this, 'method' )`, binding the callbacks to the instance that was the global when `wp-settings.php` ran `add_hooks()` on `after_setup_theme`. Replacing the `$wp_script_modules` global after that point changed where `wp_enqueue_script_module()` wrote, but not what was printed. Classic scripts and styles do not have this problem because their hooks are procedural functions that read the global when they run. Add procedural print functions that delegate to `wp_script_modules()` and register those instead. See #66100, #64484.
6a90a3c to
c42ebcd
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
…unctions. Five of the seven new procedural print functions declared no return type while `wp_print_script_module_translations()` and `wp_print_script_module_data()` declared `: void`. Declare `: void` on the other five so the set is consistent. None of them returns a value. See #66100.
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
I have concerns about this:
Compatibility:
remove_action()with the instance callback no longer matches. AMP does this inincludes/class-amp-theme-support.php.
In addition to the AMP plugin removing these, this is also done by 3 other plugins, with atum-stock-manager-for-woocommerce (10K+ active installs) seeming being the most concerning: https://veloria.dev/search/5536fd24-0f42-484b-8b24-813e03b952be
In searching GitHub, I also see this would seem to break some Jetpack code: https://github.com/Automattic/jetpack/blob/fc6b865fe09ea7b407925c7e66757f7dd7c4dcf7/projects/packages/jetpack-mu-wpcom/src/features/wpcom-blocks/code/class-code-block.php#L587-L634
This also would break some old Gutenberg compat code which is no longer shipping: https://github.com/WordPress/gutenberg/blob/497e846fe0f157daf97912d95c860558de495574/lib/compat/wordpress-6.7/script-modules.php#L92-L104
It would also break this plugin from PEW Research: https://github.com/pewresearch/prc-print-engine/blob/8f927a2af9b991987c5990ee5cabaaa1062e3dac/includes/class-print-engine.php#L589-L612
I fear the back-compat breakage could be too great.
While not related to the compat question, these lines of code in Gutenberg would eventually need to be updated as well:
// Print import map first so it's available for inline scripts
wp_script_modules()->print_import_map();
print_footer_scripts();
wp_script_modules()->print_enqueued_script_modules();
wp_script_modules()->print_script_module_preloads();
wp_script_modules()->print_script_module_data();These will continue to work as-is in the mean time.
|
This was motivated by #13507 originally. That work no longer depends on this change so I don't have a pressing need to land this. However, this is a correctness issue. The change in r57503 does not have the desired impact because if the I do share the back-compat concerns, but I'm also worried that this is a bug where the global can be modified but that doesn't actually do anything. |
2 participants
WP_Script_Modules::add_hooks()registersarray( $this, 'method' ), so the print callbacks are bound to the instance that was the global onafter_setup_theme. Replacing$wp_script_moduleslater changes what enqueues but not what prints. Scripts and styles use procedural callbacks that read the global at run time.This adds procedural print functions to
script-modules.phpand registers those instead. Hook names, priorities, and the instance methods are unchanged.Test: register the hooks, replace the global, enqueue a module, fire
admin_print_footer_scripts. Fails on trunk with empty output.Compatibility:
remove_action()with the instance callback no longer matches. AMP does this inincludes/class-amp-theme-support.php.Trac ticket: https://core.trac.wordpress.org/ticket/66100
Follow-up to r57503.
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Fable 5.1, Claude Opus
Used for: code, test, description.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.
🤖 Generated with Claude Code