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

Interactivity API: Move Store's data encoding to the echo call #51974

Merged
merged 3 commits into from
Jul 24, 2023
Merged

Interactivity API: Move Store's data encoding to the echo call #51974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6df0ef1
Move `json_enconde` to the `echo` call inside `render`
DAreRodz Jun 27, 2023
c24f7ee
Escape tags and ampersands in WP_Interactivity_Store output
DAreRodz Jul 21, 2023
9176b2f
Fix expected and add missing commas
DAreRodz Jul 21, 2023
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
16 changes: 4 additions & 12 deletions lib/experimental/interactivity-api/class-wp-interactivity-store.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,6 @@ static function merge_data( $data ) {
self::$store = array_replace_recursive( self::$store, $data );
}

/**
* Serialize store data to JSON.
*
* @return string|false Serialized JSON data.
*/
static function serialize() {
// TODO: Escape?
return wp_json_encode( self::$store );
}

/**
* Reset the store data.
*/
Expand All @@ -71,7 +61,9 @@ static function render() {
if ( empty( self::$store ) ) {
return;
}
$store = self::serialize();
echo "<script id=\"wp-interactivity-store-data\" type=\"application/json\">$store</script>";
echo sprintf(
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered using native WordPress functions like wp_register_script that support inline scripts? They give better control over where the code is going rendered in the document, and plugin authors can use filtering if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

This one?

Copy link
Member

@gziolo gziolo Aug 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that would work, you can connect it with the existing interactivity script handle and put it before or after depending on the actual use case.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect, thanks. Added to the roadmap 🙂👍️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gziolo, do you know if wp_add_inline_script works with application/json scripts? 👀

Copy link
Member

Choose a reason for hiding this comment

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

@DAreRodz, most likely, you would need to use WP hooks to inject that custom type.

@adamsilverstein or @westonruter, what would you advise to handle in this special case? Is it something that we should bake into wp_add_inline_script in WP core?

Copy link
Member

Choose a reason for hiding this comment

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

As of WordPress 6.3, such inline scripts get printed via \WP_Scripts::get_inline_script_tag() which finally makes use of wp_get_inline_script_tag(). This function fortunately includes a wp_inline_script_attributes filter for manipulating the inline script's attributes, such as adding a nonce or even change the type to application/json. Nevertheless, since this is a low-level function it isn't aware of the script handle specifically, although there is an id attribute that is passed. So if the script handle was interactivity and the script was a before inline script, this should work:

add_filter(
    'wp_inline_script_attributes',
    static function ( $attributes ) {
        if ( isset( $attributes['id'] ) && 'interactivity-js-before' === $attributes['id'] ) {
            $attributes['type'] = 'application/json';
        }
        return $attributes;
    }
);

I'm not sure whether it makes sense to add a more direct API to change the type of such inline scripts without seeing whether there are more such use cases.

Copy link
Member

Choose a reason for hiding this comment

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

@westonruter, thank you so much for the detailed explanation. The id should be enough in this case as it is standardized based on the script handle and always included for a few major WP releases.

I'm not sure whether it makes sense to add a more direct API to change the type of such inline scripts without seeing whether there are more such use cases.

Let's see if wp_add_inline_script needs to be extended. If it's one of case, then the filter is more than enough.

'<script id="wp-interactivity-store-data" type="application/json">%s</script>',
wp_json_encode( self::$store, JSON_HEX_TAG | JSON_HEX_AMP )
);
}
}
18 changes: 18 additions & 0 deletions phpunit/experimental/interactivity-api/class-wp-interactivity-store-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,22 @@ public function test_store_should_be_correctly_rendered() {
$rendered
);
}

public function test_store_should_also_escape_tags_and_amps() {
WP_Interactivity_Store::merge_data(
array(
'state' => array(
'amps' => 'http://site.test/?foo=1&baz=2&bar=3',
'tags' => 'Do not do this: <!-- <script>',
),
)
);
ob_start();
WP_Interactivity_Store::render();
$rendered = ob_get_clean();
$this->assertSame(
'<script id="wp-interactivity-store-data" type="application/json">{"state":{"amps":"http:\/\/site.test\/?foo=1\u0026baz=2\u0026bar=3","tags":"Do not do this: \u003C!-- \u003Cscript\u003E"}}</script>',
$rendered
);
}
}