Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Script Modules: Add data server->client data passing #6433

Closed
wants to merge 10 commits into from
Closed

Script Modules: Add data server->client data passing #6433

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
83218e1
Add Script Module hooks registration on admin_footer
sirreal Apr 26, 2024
c0d71ac
Remove empty line in PHPDoc
sirreal Apr 22, 2024
70780de
add data passing for script modules
sirreal Apr 23, 2024
2c052a1
Fix filters and ID
sirreal Apr 23, 2024
3dc7e5f
Replace array_merge with assignment
sirreal May 3, 2024
4fa47e3
Merge branch 'fix/script-modules-admin-hooks' into add/script-module-…
sirreal May 3, 2024
e7c4afc
Print script data in admin_footer
sirreal May 3, 2024
21d006d
DROPME: Enqueue api-fetch module dependent
sirreal May 3, 2024
d36e0c2
Merge fix/script-modules-admin-hooks
sirreal May 8, 2024
d185ac6
Improve data encoding
sirreal May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/wp-includes/block-editor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,13 @@ function block_editor_rest_api_preload( array $preload_paths, $block_editor_cont
),
'after'
);
add_filter(
'scriptmoduledata_@wordpress/api-fetch',
function ( $data ) use ( $preload_data ) {
$data['preloadData'] = $preload_data;
return $data;
}
);
}

/**
Expand Down
30 changes: 30 additions & 0 deletions src/wp-includes/class-wp-script-modules.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,13 @@ public function add_hooks() {
add_action( $position, array( $this, 'print_import_map' ) );
add_action( $position, array( $this, 'print_enqueued_script_modules' ) );
add_action( $position, array( $this, 'print_script_module_preloads' ) );

add_action( 'admin_footer', array( $this, 'print_import_map' ) );
add_action( 'admin_footer', array( $this, 'print_enqueued_script_modules' ) );
add_action( 'admin_footer', array( $this, 'print_script_module_preloads' ) );

add_action( 'wp_footer', array( $this, 'print_script_data' ) );
add_action( 'admin_footer', array( $this, 'print_script_data' ) );
}

/**
Expand Down Expand Up @@ -359,4 +366,27 @@ private function get_src( string $id ): string {

return $src;
}

public function print_script_data(): void {
$modules = array();
foreach ( array_keys( $this->get_marked_for_enqueue() ) as $id ) {
$modules[ $id ] = true;
}
foreach ( array_keys( $this->get_import_map()['imports'] ) as $id ) {
$modules[ $id ] = true;
}

foreach ( array_keys( $modules ) as $module_id ) {
$config = apply_filters( 'scriptmoduledata_' . $module_id, array() );
if ( ! empty( $config ) ) {
wp_print_inline_script_tag(
wp_json_encode( $config, JSON_HEX_TAG | JSON_HEX_AMP ),
Copy link

Choose a reason for hiding this comment

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

Any reason not to include JSON_UNESCAPED_SLASHES here too? I suspect URLs of endpoints will be relatively common data, and avoiding the leaning toothpick syndrome would be nice. Having JSON_HEX_TAG should be sufficient to avoid problems with </script> that the default escaping of slashes was added to avoid.

JSON_UNESCAPED_UNICODE is another one to consider, but that would need testing for whether it breaks if pages are served with an encoding other than UTF-8.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't consider this at all, it's copied from how the Interactivity API implements the same thing:

wordpress-develop/src/wp-includes/interactivity-api/class-wp-interactivity-api.php

Lines 170 to 179 in 473e255

wp_print_inline_script_tag(
wp_json_encode(
$interactivity_data,
JSON_HEX_TAG | JSON_HEX_AMP
),
array(
'type' => 'application/json',
'id' => 'wp-interactivity-data',
)
);

I did some testing and it seems like we should be able to add JSON_UNESCAPED_SLASHES in both of these places, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

JSON_UNESCAPED_UNICODE might also be nice to add, but as you mention it might need to be behind a UTF-8 check:

$opts = JSON_HEX_TAG | JSON_HEX_AMP | JSON_UNESCAPED_SLASHES;
if ( get_bloginfo( 'blog_charset' ) === 'UTF-8' ) {
  $opts |= JSON_UNESCAPED_UNICODE;
}

It looks like it would certainly improve readability and potentially size:

<?php
$opts = 0;
$t = '🏴󠁧󠁢󠁥󠁮󠁧󠁿';
var_dump(
    $t,
    strlen($t),
    json_encode($t, $opts),
    strlen(json_encode($t, $opts)),
    json_encode($t, $opts | JSON_UNESCAPED_UNICODE),
    strlen(json_encode($t, $opts | JSON_UNESCAPED_UNICODE)),
);
string(28) "🏴󠁧󠁢󠁥󠁮󠁧󠁿"
int(28)
string(86) ""\ud83c\udff4\udb40\udc67\udb40\udc62\udb40\udc65\udb40\udc6e\udb40\udc67\udb40\udc7f""
int(86)
string(30) ""🏴󠁧󠁢󠁥󠁮󠁧󠁿""
int(30)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll follow up for Interactivity API here: #6520

Copy link

Choose a reason for hiding this comment

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

In some quick experimenting, I find Firefox 125 and Chromium 124 both do need the document to be utf-8 to interpret unescaped Unicode properly.

Also they seem to use the actual Content-Type HTTP header over <meta> tags if both specify a different charset, but they'll use <meta> tags if the HTTP header doesn't specify a charset. The deprecated charset attribute on a <script> tag doesn't seem to be used by either.

Copy link
Member

Choose a reason for hiding this comment

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

It is vital that JSON_UNESCAPED_SLASHES not be added, as it avoids a security vulnerability when encoding a string that includes </script>. When slashes are escaped, this becomes <\/script> and the browser won't prematurely close the script tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for flagging that! I'd like to continue discussing on #6520 (comment), where I've replied. That PR is likely to land sooner, and whatever implementation is used there should also be used here. (In fact, I'd expect Interactivity API to use this proposal to print its data.)

Copy link

Choose a reason for hiding this comment

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

@westonruter I already addressed that concern

Having JSON_HEX_TAG should be sufficient to avoid problems with </script> that the default escaping of slashes was added to avoid.

Is there any reason to think that's not accurate?

Copy link
Member

@westonruter westonruter May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yes. As long as JSON_HEX_TAG is present, then it seems safe to include JSON_UNESCAPED_SLASHES. Sorry I missed that. Maybe add a comment explaining why the particular set of flags are used, noting the security implications.

array(
'type' => 'application/json',
'id' => 'wp-scriptmodule-data_' . $module_id,
)
);
}
}
}
}
16 changes: 16 additions & 0 deletions src/wp-includes/script-modules.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,3 +123,19 @@ function wp_dequeue_script_module( string $id ) {
function wp_deregister_script_module( string $id ) {
wp_script_modules()->deregister( $id );
}


function wp_register_default_script_modules(): void {
add_filter(
'scriptmoduledata_@wordpress/api-fetch',
function ( $data ) {
$data['rootURL'] = sanitize_url( get_rest_url() );
$data['nonce'] = wp_installing() ? '' : wp_create_nonce( 'wp_rest' );
$data['shouldRegisterMediaUploadMiddleware'] = true;
$data['nonceEndpoint'] = admin_url( 'admin-ajax.php?action=rest-nonce' );
return $data;
}
);
wp_enqueue_script_module( '__DEV__/noop', includes_url( '/noop.js' ), array( '@wordpress/api-fetch' ) );
}
add_action( 'init', 'wp_register_default_script_modules', 0 );
8 changes: 8 additions & 0 deletions src/wp-includes/theme-previews.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ function wp_attach_theme_preview_middleware() {
),
'after'
);

add_filter(
'scriptmoduledata_@wordpress/api-fetch',
function ( $data ) {
$data['themePreviewPath'] = sanitize_text_field( wp_unslash( $_GET['wp_theme_preview'] ) );
return $data;
}
);
}

/**
Expand Down
Loading