Scripts: Use HTML API to build SCRIPT tags #10639
Conversation
…html-api-for-script-tags
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. |
Co-authored-by: Weston Ruter <westonruter@gmail.com>
…html-api-for-script-tags
| * Other types of script tags cannot be escaped safely because there is | ||
| * no general escaping mechanism for arbitrary types of content. | ||
| */ | ||
| return false; |
There was a problem hiding this comment.
if we think about changing the design of the is_javascript_script_tag() methods, we could expose something like a MIME type and allow custom filtering of escapes to allow plugins to provide their own escaping routines rather than reject the updates.
for now I see no reason to block progress for this, especially since we already rejected these contents before, but I think we can imagine a situation where someone can replace their SCRIPT contents and we can perform the <script and </script checks after the filter, and only then reject if nothing was able to escape
Avoid naming unescaped plaintext variable as "escaped."
…html-api-for-script-tags
This aligns with the way r61418 produces style tags
| $expected = str_replace( "'", '"', "<script src='/main-script-a4.js' id='main-script-a4-js' {$strategy} data-wp-strategy='{$strategy}'></script>" ); | ||
| $this->assertStringContainsString( $expected, $output, 'Only enqueued dependents should affect the eligible strategy.' ); | ||
| $expected = "<script src='/main-script-a4.js' id='main-script-a4-js' {$strategy} data-wp-strategy='{$strategy}'></script>"; | ||
| $this->assertEqualHTMLScriptTagById( $expected, $output, 'Only enqueued dependents should affect the eligible strategy.' ); |
There was a problem hiding this comment.
These test changes should be extracted to their own PR.
There was a problem hiding this comment.
I'm curious why you chose DOT as opposed to Mermaid? I know that GitHub supports Mermaid.
In any case, I'm very impressed that you went to the effort of making a diagram to document this.
| $processor->next_tag(); | ||
| foreach ( $attributes as $name => $value ) { | ||
| if ( is_string( $value ) || true === $value ) { | ||
| $processor->set_attribute( $name, $value ); |
There was a problem hiding this comment.
I appreciate this type guarding, but it feels a bit redundant. are we attempting to type-check the filter results or are we trying to guard against sending invalid types to set_attribute()?
it doesn’t currently type-check against a string, but we could add _doing_it_wrong() if we want to incur the runtime cost of type-checking there, which would seem okay.
I just want to question the intent here and the fact that this hides type errors and mistakes in the calling code by skipping over attributes which presumably were supposed to be set.
| } | ||
| } | ||
| $processor->set_modifiable_text( $data ); | ||
| return "{$processor->get_updated_html()}\n"; |
| * | ||
| * While it may seem tempting to replace the `<` character instead, but doing so | ||
| * would break JavaScript syntax. The `<` character is used in comparison operators | ||
| * and other JavaScript syntax, so replacing it would break valid JavaScript.j |
There was a problem hiding this comment.
| * and other JavaScript syntax, so replacing it would break valid JavaScript.j | |
| * and other JavaScript syntax, so replacing it would break valid JavaScript. |
There was a problem hiding this comment.
@westonruter these are handled in #10635 — might want to avoid scrutinizing the changes too closely that are in that other branch, or scrutinize them there.
| * @param string $sourcecode Raw contents intended to be serialized into an HTML SCRIPT element. | ||
| * @return string Escaped form of input contents which will not lead to premature closing of the containing SCRIPT element. | ||
| */ | ||
| public static function escape_javascript_script_contents( string $sourcecode ): string { |
There was a problem hiding this comment.
This is intended to be public, yes?
| * escape (`\u0073`) back into its original plaintext value, but only after having | ||
| * been safely extracted from the HTML. | ||
| * | ||
| * While it may seem tempting to replace the `<` character instead, but doing so |
There was a problem hiding this comment.
| * While it may seem tempting to replace the `<` character instead, but doing so | |
| * While it may seem tempting to replace the `<` character instead, doing so |
Gemini suggests the "but" is redundant. I agree.
3 participants
Includes #10635
Trac ticket:
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.