Make WordPress Core

Opened 9 months ago

Closed 4 weeks ago

Last modified 3 weeks ago

#64484 closed defect (bug) (fixed)

viewScriptModule is also being loaded in the block editor

Reported by: vrishabhsk Owned by: jonsurrell
Priority: normal Milestone: 7.2
Component: Editor Version: 6.9
Severity: minor Keywords: has-patch dev-feedback has-unit-tests commit
Cc: Focuses:

Description

viewScriptModule file defined in the block.json is meant to be loaded only in the frontend when the block is displayed but right now is also being loaded in the block editor.

Additional information : ​https://github.com/WordPress/gutenberg/issues/72833

Change History (26)

This ticket was mentioned in ​PR #10693 on ​WordPress/wordpress-develop by ​@vrishabhsk.


9 months ago
#1

  • Keywords has-patch added
  • Added a is_admin check before the viewScriptModule scripts are enqueued via wp_enqueue_script_module

Trac ticket: https://core.trac.wordpress.org/ticket/64484

#2 @westonruter
9 months ago

  • Keywords reporter-feedback added
  • Milestone Awaiting Review → 7.0

Is this not also an issue for ​viewScript?

This ticket was mentioned in ​Slack in #core by juanmaguitar. ​View the logs.


7 months ago

#4 @juanmaguitar
7 months ago

  • Keywords dev-feedback added; reporter-feedback removed

From today's bug scrub

As per @jonsurrell comment on #10693 this needs more exploration by a developer

This really needs some debugging and an explanation. I don't think this change should move ahead until there's an understanding of why these systems behave differently.

I'm removing the reporter-feedback keyboard and adding the dev-feedback keyword.

@jonsurrell and @westonruter, do you think we could clarify this ticket in time for WP 7.0 RC1?

#5 @westonruter
7 months ago

It's unlikely that I'm going to have time to investigate for 7.0.

#6 @juanmaguitar
7 months ago

  • Milestone 7.0 → 7.1

It's unlikely that I'm going to have time to investigate for 7.0.

Punting it then for 7.1

#7 @wildworks
3 months ago

Hi, does anyone have the bandwidth to review ​PR 10693? Otherwise, this ticket may be punted to 7.2.

This ticket was mentioned in ​Slack in #core by adrianduffell. ​View the logs.


3 months ago

#9 @adrianduffell
3 months ago

  • Milestone 7.1 → Future Release

This was discussed in today's bug scrub. With RC 1 is due in a few days time, let's punt this to a future release for more time to work through.

#10 @jonsurrell
5 weeks ago

  • Milestone Future Release → 7.2
  • Owner set to jonsurrell
  • Status new → accepted

This ticket was mentioned in ​PR #13507 on ​WordPress/wordpress-develop by ​@jonsurrell.


5 weeks ago
#11

  • Keywords has-unit-tests added

The block editor page preloads REST responses in-process with block_editor_rest_api_preload(). One of those requests is the post with context=edit. The posts controller applies the_content, which renders the post's blocks. WP_Block::render() enqueues each block's view scripts, view styles, and view script modules.

After preloading, the function restores $wp_scripts and $wp_styles to their state from before the preload (see [52733] for #55151). Classic view scripts and view styles enqueued during rendering are discarded. $wp_script_modules is not restored. A block's viewScriptModule stays enqueued and is printed in the editor page footer by the admin_print_footer_scripts hooks.

This change backs up and restores $wp_script_modules the same way.

The test registers a block with a view script, a view style, and a view script module, preloads the post route, and asserts that all three are discarded and that a script module enqueued before preloading is kept. It also asserts that the block rendered during preloading, so the negative assertions cannot pass by accident. Without the fix, only the script module assertion fails.

This replaces the approach in #10693, which added an is_admin() check to WP_Block::render(). That hides the symptom in the editor but leaves the module and script paths inconsistent and blocks script modules on admin pages that print them on purpose.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Fable 5.1, Claude Opus
Used for: diagnosis, the code change, the test, and this 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

This ticket was mentioned in ​PR #13509 on ​WordPress/wordpress-develop by ​@jonsurrell.


5 weeks ago
#12

WP_Script_Modules::add_hooks() registered every print callback as array( $this, 'method' ). wp-settings.php calls add_hooks() on after_setup_theme via array( wp_script_modules(), 'add_hooks' ). The callbacks were therefore bound to whichever instance was the global at that moment.

wp_script_modules() reads the $wp_script_modules global. [57503] made it a global so the instance can be replaced. After this change, replacing the global changed what wp_enqueue_script_module() wrote to, but not what was printed. The old instance kept printing.

Classic scripts and styles do not have this problem. Their hooks are procedural functions such as wp_print_head_scripts(), _wp_footer_scripts(), and wp_print_styles(). Those read the global when they run.

This inconsistency caused a defect in #64484. The fix in ​https://github.com/WordPress/wordpress-develop/pull/13507 swapped the global for a clone. The unit test passed. The page was unchanged.

This patch adds procedural print functions to src/wp-includes/script-modules.php. Each one is a single call to the current global instance. add_hooks() registers those functions. Every hook name, priority, and the block theme conditional are unchanged. The instance methods stay public and unchanged. wp-settings.php is unchanged.

The new test calls add_hooks() on the instance that is the global, replaces the global with a new instance, enqueues a module through wp_enqueue_script_module(), and fires wp_footer. Before this patch the output contains no module, because the bound original instance has an empty queue. After it, the module is printed.

### Backward compatibility

Plugins that remove these hooks by instance callback will no longer match. Known cases in the plugin directory:

  • AMP 2.5.5 (about 400,000 installs) removes print_import_map, print_enqueued_script_modules, and print_script_module_preloads from wp_head and wp_footer via [ wp_script_modules(), '...' ] in includes/class-amp-theme-support.php:924-926. Those remove_action() calls will stop working.
  • NextGenThemes jsDelivr CDN 1.3.6 does the same.

Plugins that add the instance callbacks are unaffected. Templately 3.7.5 re-adds them on wp_footer and keeps working.

Trac ticket: new ticket to be filed; see https://core.trac.wordpress.org/ticket/64484 for the defect this inconsistency caused.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Fable 5.1, Claude Opus
Used for: the code change, the test, and this 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

This ticket was mentioned in ​PR #13510 on ​WordPress/wordpress-develop by ​@jonsurrell.


5 weeks ago
#13

WP_Interactivity_API::add_hooks() registered its filters as array( $this, 'method' ). process_directives() added array( $this, 'print_router_markup' ) on wp_footer the same way. wp-settings.php calls add_hooks() on after_setup_theme through array( wp_interactivity(), 'add_hooks' ), so the callbacks stay bound to whichever object was the global at that moment.

wp_interactivity() reads the $wp_interactivity global, so the instance can be replaced. After a replacement, wp_interactivity_state() and wp_interactivity_config() write to the new instance, but the bound callbacks still read the old one. The data printed for @wordpress/interactivity then comes from the wrong instance. Classic scripts and styles do not have this problem, because their hooks are procedural functions that read the global when they run. The same inconsistency in WP_Script_Modules caused a defect in #64484.

This adds four procedural functions to src/wp-includes/interactivity-api/interactivity-api.php. Each is a one-line delegate to the current global instance. add_hooks() and the wp_footer registration now use those functions. Hook names and priorities are unchanged, and the instance methods stay public and unchanged.

The new test calls add_hooks() on the current global, replaces the global with a fresh instance, sets state through wp_interactivity_state(), and applies the script_module_data_@wordpress/interactivity filter. It asserts that the filtered data contains the state set on the new global. The test fails before the change and passes after it.

Tests_Interactivity_API_WpInteractivityAPI::set_up() created a WP_Interactivity_API instance and called add_hooks() on it without making it the global. With the fix, the hooks read the global, so set_up() now installs the test instance as the global and tear_down() restores the previous one.

### Compatibility

Code that looks up these callbacks by instance no longer matches. The Gutenberg plugin does this in lib/interactivity-api.php:

if ( ! has_filter( 'script_module_data_@wordpress/interactivity-router', array( wp_interactivity(), 'filter_script_module_interactivity_router_data' ) ) ) {

That check runs on after_setup_theme at priority 20, after Core's add_hooks(). Today it finds Core's callback and skips the plugin's own filter. After this change it finds nothing, so Gutenberg adds its closure and both callbacks run. Both set $data['i18n']['loading'] and $data['i18n']['loaded'] to the same strings in the default text domain, so the printed data is identical. The result is a redundant filter, not a change in output.

Gutenberg should still be updated. The check can test for wp_interactivity_router_script_module_data, or the shim can be removed, since Core has printed this data since 6.7. No plugin or theme in the WordPress.org directories removes these callbacks by instance.

Trac ticket: new ticket to be filed; see https://core.trac.wordpress.org/ticket/64484 for the defect this inconsistency caused.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Fable 5.1, Claude Opus
Used for: the code change, the test, and this 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

#14 @jonsurrell
5 weeks ago

I discovered #66100 while working on a fix for this. Fixing that is a prerequisite.

​@jonsurrell commented on ​PR #13510:


5 weeks ago
#15

Ping @darerodz for review.

​@jonsurrell commented on ​PR #13507:


5 weeks ago
#16

There's a potential follow-up to do the same with Interactivity API state, which could also be populated undesirably on preload.

​@jonsurrell commented on ​PR #10693:


5 weeks ago
#17

The discrepancy in behavior is explained by some preloading. It includes resets for script and style state, but not for modules.

We'll fix that cause instead of addressing the symptom.

Closing in favor of ​https://github.com/WordPress/wordpress-develop/pull/13507.

​@jonsurrell commented on ​PR #13509:


4 weeks ago
#19

This was motivated by ​https://github.com/WordPress/wordpress-develop/pull/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.

​@westonruter commented on ​PR #13509:


4 weeks ago
#20

The change in r57503 does not have the desired impact because if the

Because of the what?

​@jonsurrell commented on ​PR #13509:


4 weeks ago
#21

Sorry, I've corrected the updated comment. In short, the global value can be replaced but it doesn't change relevant behavior because script module hooks are registered with whatever instance was used at the after_setup_theme action that registers the hooks.

​See my updated comment for full details.

​@jonsurrell commented on ​PR #13509:


4 weeks ago
#22

There are a number of alternatives I've been considering, none of which I like. For example, the hook methods could perform the expected action if it's the global instance, or invoke the global instance otherwise. This seems like it would address back-compat _and_ the problem of not running the actions on the new global, but it also seems messy and surprising.

#23 @jonsurrell
4 weeks ago

  • Keywords commit added

#24 @jonsurrell
4 weeks ago

  • Resolution → fixed
  • Status accepted → closed

In 63654:

Editor: Discard script modules enqueued during REST API preloading.

Ensure that side effects of preloading do not impact script modules state.

Developed in: ​https://github.com/WordPress/wordpress-develop/pull/13507

Follow-up to r52733.

Props jonsurrell, westonruter, vrishabhsk, juanmaguitar, sirlouen.
Fixes #64484. See #55151.

​@westonruter commented on ​PR #13509:


4 weeks ago
#25

In short, the global value can be replaced but it doesn't change relevant behavior because script module hooks are registered with whatever instance was used at the after_setup_theme action that registers the hooks.

Is replacing global variables like this something that is a common occurrence. It happens all the time in unit tests, but in theme/plugin code I would think that such globals are almost always left alone.

Note: See TracTickets for help on using tickets.