-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
Eliminate manual construction of script tags in WP_Scripts and pass other scripts through wp_print_inline_script_tag() #4773
Changes from all commits
0d3ae2d
86d74f0
61c877c
9ad9e6b
b52c335
8355a8c
b9ebf8b
d05efbe
3ac9a6e
adfef39
6ba21ef
c19f75d
50aafac
0e27a38
3b9b278
96b1e7a
96788c1
def2778
8bcd7a2
1194afe
205b8de
fd9028d
2db59e5
3ba5135
ecc29a9
832c315
3840c54
97ad256
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -464,6 +464,8 @@ protected function wp_die( $ajax_message, $message = null ) { | |||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||
'error' => $ajax_message, | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
$message .= ob_get_clean(); | ||||||||||||||||||||||||||||
ob_start(); | ||||||||||||||||||||||||||||
?> | ||||||||||||||||||||||||||||
<script> | ||||||||||||||||||||||||||||
( function( api, settings ) { | ||||||||||||||||||||||||||||
|
@@ -472,7 +474,7 @@ protected function wp_die( $ajax_message, $message = null ) { | |||||||||||||||||||||||||||
} )( wp.customize, <?php echo wp_json_encode( $settings ); ?> ); | ||||||||||||||||||||||||||||
</script> | ||||||||||||||||||||||||||||
<?php | ||||||||||||||||||||||||||||
$message .= ob_get_clean(); | ||||||||||||||||||||||||||||
$message .= wp_get_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) ); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
wp_die( $message ); | ||||||||||||||||||||||||||||
|
@@ -2083,6 +2085,7 @@ public function remove_frameless_preview_messenger_channel() { | |||||||||||||||||||||||||||
if ( ! $this->messenger_channel ) { | ||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
ob_start(); | ||||||||||||||||||||||||||||
?> | ||||||||||||||||||||||||||||
<script> | ||||||||||||||||||||||||||||
( function() { | ||||||||||||||||||||||||||||
|
@@ -2106,6 +2109,7 @@ public function remove_frameless_preview_messenger_channel() { | |||||||||||||||||||||||||||
} )(); | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is out of scope for this PR, but looks like we could prevent some other corruption here by using url = new URL( location.href );
queryParams = url.searchParams;
if ( queryParams.has( 'customize_messenger_channel' ) ) {
queryParams.delete( 'customize_messenger_channel' );
url.search = queryParams;
location.replace( url );
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, this code dates back to a time before we could rely on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed this in Core-59480 |
||||||||||||||||||||||||||||
</script> | ||||||||||||||||||||||||||||
<?php | ||||||||||||||||||||||||||||
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) ); | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops, we don't want to arbitrarily replace these tags. we know they exist at the front and back of the string, so by manually removing them we can avoid unintentional HTML syntax poisoning. $script_html = ob_get_clean();
$script_html = substr( $script_html, strlen( '<script>' ), -strlen( '</script>' ) ); the this change is both safer, deterministic, and more efficient, particularly in the worse-case, though given that we expect very short args here I would be surprised if it's a measurable impact. still, doing less is faster than doing more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great catch @dmsnell! @westonruter IMO this is worth a quick follow up commit applying this throughout. If we feel strongly about introducing a helper at this point, I wouldn't be opposed to that either. Though I think that should remain an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Private helper function added in #5301 |
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||
|
@@ -2201,8 +2205,9 @@ public function customize_preview_settings() { | |||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
ob_start(); | ||||||||||||||||||||||||||||
?> | ||||||||||||||||||||||||||||
<script type="text/javascript"> | ||||||||||||||||||||||||||||
<script> | ||||||||||||||||||||||||||||
var _wpCustomizeSettings = <?php echo wp_json_encode( $settings ); ?>; | ||||||||||||||||||||||||||||
_wpCustomizeSettings.values = {}; | ||||||||||||||||||||||||||||
(function( v ) { | ||||||||||||||||||||||||||||
|
@@ -2225,6 +2230,7 @@ public function customize_preview_settings() { | |||||||||||||||||||||||||||
})( _wpCustomizeSettings.values ); | ||||||||||||||||||||||||||||
</script> | ||||||||||||||||||||||||||||
<?php | ||||||||||||||||||||||||||||
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) ); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||
|
@@ -4976,8 +4982,9 @@ public function customize_pane_settings() { | |||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
ob_start(); | ||||||||||||||||||||||||||||
?> | ||||||||||||||||||||||||||||
<script type="text/javascript"> | ||||||||||||||||||||||||||||
<script> | ||||||||||||||||||||||||||||
var _wpCustomizeSettings = <?php echo wp_json_encode( $settings ); ?>; | ||||||||||||||||||||||||||||
_wpCustomizeSettings.initialClientTimestamp = _.now(); | ||||||||||||||||||||||||||||
_wpCustomizeSettings.controls = {}; | ||||||||||||||||||||||||||||
|
@@ -5012,6 +5019,7 @@ public function customize_pane_settings() { | |||||||||||||||||||||||||||
?> | ||||||||||||||||||||||||||||
</script> | ||||||||||||||||||||||||||||
<?php | ||||||||||||||||||||||||||||
wp_print_inline_script_tag( str_replace( array( '<script>', '</script>' ), '', ob_get_clean() ) ); | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it help to create a helper function for this specific task? /**
* Removes leading and trailing _empty_ script tags.
*
* This is a helper meant to be used for literal script tag construction
* within wp_print_inline_script_tag(). It removes the literal values of
* "<script>" and "</script>" from around an inline script.
*
* @since 6.4.0
*
* @param string $contents Script body with manually created SCRIPT tag literals.
* @return string Script body without surrounding script tag literals, or
* original contents if both exact literals aren't present.
*/
function wp_remove_surrounding_empty_script_tags( $contents ) {
$opener = '<script>';
$closer = '</script>';
$has_both_empty_tags = (
strlen( $opener ) + strlen( $closer ) > strlen( $contents ) ||
substr( $contents, 0, strlen( $opener ) ) !== $opener ) ||
substr( $contents, -strlen( $closer ) ) !== $closer
);
return $has_both_empty_tags
? substr( $contents, strlen( $opener ), -strlen( $closer ) )
: $contents;
} then these repetitive calls could be marginally nicer. even with the checks for the leading and trailing SCRIPT tags, this should still be faster than $script_body = wp_remove_surrounding_empty_script_tags( ob_get_clean() );
wp_print_inline_script_tag( $script_body ); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a syntax error in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure; also just a note - all my examples are almost always wrong and are only meant to convey ideas. if you ever get the impulse, please never copy code of mine from a comment and paste it into a project. don't trust me 😄 I don't test these examples or vet them for WPCS' prefences. /**
* Removes leading and trailing _empty_ script tags.
*
* This is a helper meant to be used for literal script tag construction
* within wp_print_inline_script_tag(). It removes the literal values of
* "<script>" and "</script>" from around an inline script.
*
* @since 6.4.0
*
* @param string $contents Script body with manually created SCRIPT tag literals.
* @return string Script body without surrounding script tag literals, or
* original contents if both exact literals aren't present.
*/
function wp_remove_surrounding_empty_script_tags( $contents ) {
$opener = '<script>';
$closer = '</script>';
$has_both_empty_tags = (
strlen( $opener ) + strlen( $closer ) > strlen( $contents ) ||
substr( $contents, 0, strlen( $opener ) ) !== $opener ||
substr( $contents, -strlen( $closer ) ) !== $closer
);
/*
* What should happen if the given contents are not surrounded by
* the exact literal script tags? This question opens up a can of
* worms with no obvious or clear answer. Removing one of the tags
* could lead to just as much trouble as leaving one in place.
* This code leaves the string untouched if it can't find both the
* exact tags. Maybe someone added an attribute to the opening tag
* or maybe someone added a space; it's impossible to know from
* here.
*
* It would be possible to return `null` or `false` here to indicate
* the failure, but people probably wouldn't be checking the result
* and that could introduce corruption. An empty string could be
* another viable return and that would quickly signal that something
* is wrong and needs fixing.
*/
return $has_both_empty_tags
? substr( $contents, strlen( $opener ), -strlen( $closer ) )
: $contents;
} Maybe it's best to return an empty string here if the syntax isn't a perfect match. Is this what you were asking for in "add comments explaining the conditions"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! I meant more the conditions for $has_both_empty_tags = (
strlen( $contents ) > strlen( $opener ) + strlen( $closer ) &&
strtolower( substr( $contents, 0, strlen( $opener ) ) ) === $opener &&
strtolower( substr( $contents, -strlen( $closer ) ) ) === $closer
); It has both empty tags if:
When those conditions are satisfied, then In testing, I see that the first line of the function should also be: $contents = trim( $contents ); In regards to the comment comment before: return $has_both_empty_tags
? substr( $contents, strlen( $opener ), -strlen( $closer ) )
: $contents; What do you think about actually just doing:
I'll note that this is actually the reverse of what wordpress-develop/src/wp-includes/functions.wp-scripts.php Lines 133 to 145 in 6fa2ce4
Note also how it does the string replacement, so somewhat the naive approach which I originally committed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've implemented this in #5301 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Seems great. There's never a case to call this without the SCRIPT tags, so that makes sense to me. Make it visible as early as possible.
I have no strong opinion on this. Seems fine. The one thing is that |
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1310,12 +1310,9 @@ public function export_preview_data() { | |
foreach ( $settings['registeredWidgets'] as &$registered_widget ) { | ||
unset( $registered_widget['callback'] ); // May not be JSON-serializeable. | ||
} | ||
|
||
?> | ||
<script type="text/javascript"> | ||
var _wpWidgetCustomizerPreviewSettings = <?php echo wp_json_encode( $settings ); ?>; | ||
</script> | ||
<?php | ||
wp_print_inline_script_tag( | ||
sprintf( 'var _wpWidgetCustomizerPreviewSettings = %s;', wp_json_encode( $settings ) ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. presumably because of the |
||
); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -2787,7 +2787,11 @@ function wp_sanitize_script_attributes( $attributes ) { | |||||||
*/ | ||||||||
function wp_get_script_tag( $attributes ) { | ||||||||
if ( ! isset( $attributes['type'] ) && ! is_admin() && ! current_theme_supports( 'html5', 'script' ) ) { | ||||||||
$attributes['type'] = 'text/javascript'; | ||||||||
// Keep the type attribute as the first for legacy reasons (it has always been this way in core). | ||||||||
$attributes = array_merge( | ||||||||
array( 'type' => 'text/javascript' ), | ||||||||
$attributes | ||||||||
); | ||||||||
Comment on lines
-2790
to
+2794
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||||||||
} | ||||||||
/** | ||||||||
* Filters attributes to be added to a script tag. | ||||||||
|
@@ -2830,9 +2834,23 @@ function wp_print_script_tag( $attributes ) { | |||||||
* @return string String containing inline JavaScript code wrapped around `<script>` tag. | ||||||||
*/ | ||||||||
function wp_get_inline_script_tag( $javascript, $attributes = array() ) { | ||||||||
if ( ! isset( $attributes['type'] ) && ! is_admin() && ! current_theme_supports( 'html5', 'script' ) ) { | ||||||||
$attributes['type'] = 'text/javascript'; | ||||||||
$is_html5 = current_theme_supports( 'html5', 'script' ) || is_admin(); | ||||||||
if ( ! isset( $attributes['type'] ) && ! $is_html5 ) { | ||||||||
// Keep the type attribute as the first for legacy reasons (it has always been this way in core). | ||||||||
$attributes = array_merge( | ||||||||
array( 'type' => 'text/javascript' ), | ||||||||
$attributes | ||||||||
); | ||||||||
} | ||||||||
|
||||||||
// Ensure markup is XHTML compatible if not HTML5. | ||||||||
if ( ! $is_html5 ) { | ||||||||
$javascript = str_replace( ']]>', ']]]]><![CDATA[>', $javascript ); // Escape any existing CDATA section. | ||||||||
$javascript = sprintf( "/* <![CDATA[ */\n%s\n/* ]]> */", $javascript ); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To ensure XML-compatibility, the
Suggested change
Nevertheless, it is likely exceedingly rare that a WordPress site is actually being served with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any concern from adding this now in case the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, good call. I've done this in ecc29a9. This shows that it is indeed necessary to do the escaping. Without the escaping, doing the following: wp_print_inline_script_tag( "/* <![CDATA[ */ console.log( 'Hello World!' ); /* ]]> */" ); Results in the following output: <script type="text/javascript">
/* <![CDATA[ */
/* <![CDATA[ */ console.log( 'Hello World!' ); /* ]]> */
/* ]]> */
</script> Pasting this into the W3C Validator as a fragment under the XHTML Strict doctype: Results in an XML parse error: However, when the escaping is present, the PHP outputs: <script type="text/javascript">
/* <![CDATA[ */
/* <![CDATA[ */ console.log( 'Hello World!' ); /* ]]]]><![CDATA[> */
/* ]]> */
</script> And this is valid: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. something as bizarre as this could definitely use a link to an XML spec or the source of the reason why these have to be escaped. someone is going to look at that and "fix" it and "improve quality" by removing the escaping 🙃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even though there is a comment about the escaping? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah because it mentions that it should be compatible but not how or why. when I saw it I immediately wondered what rule necessitates this or what breaks without it. I could imagine something like this, to see if I'm properly understanding the code. /*
* XHTML extracts the contents of the SCRIPT element and then the XML parser
* decodes character references and other syntax elements. This can lead to
* misinterpretation of the script contents or invalid XHTML documents.
*
* Wrapping the contents in a CDATA section instructs the XML parser not to
* transform the contents of the SCRIPT element before passing them to the
* JavaScript engine.
*
* Example
*
* <script>console.log('…');</script>
*
* In an HTML document this would print "…" to the console,
* but in an XHTML document it would print "…" to the console.
*
*
* <script>console.log('An image is <img> in HTML');</script>
*
* In an HTML document this would print "An image is <img> in HTML",
* but it's an invalid XHTML document because it interprets the `<img>`
* as an empty tag missing its closing `/`.
*
* @see https://www.w3.org/TR/xhtml1/#h-4.8
*/
if ( ! $is_html5 ) {
/*
* If the string `]]>` exists within the JavaScript it would break
* out of any wrapping CDATA section added here, so to start, it's
* necessary to escape that sequence which requires splitting the
* content into two CDATA sections wherever it's found.
*
* Note: it's only necessary to escape the closing `]]>` because
* an additional `<![CDATA[` leaves the contents unchanged.
*/
$javascript = str_replace( ']]>', ']]]]><![CDATA[>', $javascript );
// Wrap the entire escaped script inside a CDATA section.
$javascript = sprintf( "/* <![CDATA[ */\n%s\n/* ]]> */", $javascript );
} yeah I know it's wordy and lengthy, but it was really confusing to me, and this code is now going to be responsible for general code generation, and may get a lot of eyes on it. by the way it seems like this will not trigger if the HTTP Finally, and thankfully, it doesn't appear to be a security issue because Safari, Firefox, and Chrome all prevent escaping from the script using tricks like Including |
||||||||
} | ||||||||
|
||||||||
$javascript = "\n" . trim( $javascript, "\n\r " ) . "\n"; | ||||||||
|
||||||||
/** | ||||||||
* Filters attributes to be added to a script tag. | ||||||||
* | ||||||||
|
@@ -2845,8 +2863,6 @@ function wp_get_inline_script_tag( $javascript, $attributes = array() ) { | |||||||
*/ | ||||||||
$attributes = apply_filters( 'wp_inline_script_attributes', $attributes, $javascript ); | ||||||||
|
||||||||
$javascript = "\n" . trim( $javascript, "\n\r " ) . "\n"; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: This needs to move up above the applying of the |
||||||||
|
||||||||
return sprintf( "<script%s>%s</script>\n", wp_sanitize_script_attributes( $attributes ), $javascript ); | ||||||||
} | ||||||||
|
||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have put this in
require
since the extension is required when running tests (and not just the tests being introduced in this PR). But since it is not required to actually run WordPress, I went withsuggest
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PhpStorm, at least, rightly identifies this as having been missing:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason to probably not use this approach and stick to the simple
str_replace
that doesn't rely on new dependencies :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it was already using
DOMDocument
. It turns out thatassertEquals
in PHPUnit supports comparing twoDOMElement
instances, and this then accounts for differences in attribute order or whether single vs double quotes are used automatically. So I'm just extending that a bit further inassertEqualMarkup
to normalize a bit more for HTML5 vs XHTML.And
DOMDocument
is being used in other tests as well, so it's not a new dependency. It's just a dependency that wasn't declared before.