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

Jump to bottom

Remove scripts for components that were not detected in output buffer #3705

Merged
merged 11 commits into from
Nov 15, 2019
Merged

Remove scripts for components that were not detected in output buffer #3705

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cee8d58
Remove scripts for components that were not detected in output buffer
schlessera Nov 10, 2019
7d1bce5
Merge branch 'develop' of github.com:ampproject/amp-wp into 3489-remo…
westonruter Nov 13, 2019
8744c89
Eliminate extracting distict also_requires_tag_warning from spec
westonruter Nov 13, 2019
dba4ead
Normalize required_usage in extension_spec
westonruter Nov 13, 2019
2f9f8b9
Add required_usage info as script data and use to determine which scr…
westonruter Nov 13, 2019
d9469bf
Merge branch 'develop' of github.com:ampproject/amp-wp into 3489-remo…
westonruter Nov 15, 2019
879cb17
Improve handling of versions in extension_spec
westonruter Nov 15, 2019
17d03ad
Remove unused deprecated_allow_duplicates from extension_spec
westonruter Nov 15, 2019
9956c90
Implement method to get all extension_specs
westonruter Nov 15, 2019
16f35b4
Cache the collection of the extension_specs
westonruter Nov 15, 2019
617d17d
Remove todo
schlessera Nov 15, 2019
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
26 changes: 25 additions & 1 deletion includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1727,7 +1727,7 @@ public static function ensure_required_markup( DOMDocument $dom, $script_handles
$script->parentNode->removeChild( $script ); // So we can move it.
}

// Create scripts for any components discovered from output buffering.
// Create scripts for any components discovered from output buffering that are missing.
foreach ( array_diff( $script_handles, array_keys( $amp_scripts ) ) as $missing_script_handle ) {
if ( ! wp_script_is( $missing_script_handle, 'registered' ) ) {
continue;
Expand All @@ -1745,6 +1745,30 @@ public static function ensure_required_markup( DOMDocument $dom, $script_handles
$amp_scripts[ $missing_script_handle ] = AMP_DOM_Utils::create_node( $dom, 'script', $attrs );
}

// Remove scripts that had already been added but couldn't be detected from output buffering.
foreach ( array_diff( array_keys( $amp_scripts ), $script_handles ) as $superfluous_script_handle ) {
Copy link
Member

Choose a reason for hiding this comment

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

array_diff( array_keys( $amp_scripts ), $script_handles ) may need to be array_intersect()'ed with a list of components that are actually prone to cause validation errors. Scripts like amp-video and amp-form don't cause any issue when being included. Perhaps this is due to some having built-in status. We need to find out.

In that case, there would be no need for the amp-dynamic-css-classes check below either.

$src = $amp_scripts[ $superfluous_script_handle ]->getAttribute( 'src' );

// Skip scripts unrelated to AMP.
if ( ! $src || 0 !== strpos( $src, 'https://cdn.ampproject.org/' ) ) {
westonruter marked this conversation as resolved.
Show resolved Hide resolved
continue;
}

// Skip AMP runtime script.
if ( 'https://cdn.ampproject.org/v0.js' === $src ) {
continue;
}

$custom_element = $amp_scripts[ $superfluous_script_handle ]->getAttribute( 'custom-element' );

// Skip dynamic CSS classes script, it has no associated element.
if ( 'amp-dynamic-css-classes' === $custom_element ) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this single check here, and depending on what the AMP validator team comes back with regarding any list of components, perhaps [ 'amp-dynamic-css-classes' ] should be merged with $script_handles when it is passed into array_diff(). Then there would be no need for $custom_element, as the script handles are the component names.


unset( $amp_scripts[ $superfluous_script_handle ] );
}

/* phpcs:ignore Squiz.PHP.CommentedOutCode.Found
*
* "2. Next, preload the AMP runtime v0.js <script> tag with <link as=script href=https://cdn.ampproject.org/v0.js rel=preload>.
Expand Down
37 changes: 37 additions & 0 deletions tests/php/test-class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,7 @@ public function test_validate_non_amp_theme() {
<?php wp_head(); ?>
</head>
<body>
<amp-mathml layout="container" data-formula="\[x = {-b \pm \sqrt{b^2-4ac} \over 2a}.\]"></amp-mathml>
<?php wp_print_scripts( 'amp-mathml' ); ?>
</body>
</html>
Expand Down Expand Up @@ -1424,6 +1425,41 @@ public function test_scripts_get_moved_to_head() {
}
}

/**
* Test removing AMP scripts that are not needed.
*
* @covers AMP_Theme_Support::ensure_required_markup()
*/
public function test_unneeded_scripts_get_removed() {
ob_start();
?>
<html>
<head></head>
<body>
<amp-list width="auto" height="100" layout="fixed-height" src="/static/inline-examples/data/amp-list-urls.json">
<template type="amp-mustache">
<div class="url-entry">
<a href="{{url}}">{{title}}</a>
</div>
</template>
</amp-list>
<?php wp_print_scripts( [ 'amp-anim', 'amp-runtime', 'amp-ad', 'amp-mustache', 'amp-list', 'amp-bind' ] ); ?>
</body>
</html>
<?php
$html = ob_get_clean();
$html = AMP_Theme_Support::prepare_response( $html );

$dom = AMP_DOM_Utils::get_dom( $html );
$xpath = new DOMXPath( $dom );

$scripts = $xpath->query( '//script[ not( @type ) or @type = "text/javascript" ]' );
$this->assertSame( 3, $scripts->length );
$this->assertEquals( 'https://cdn.ampproject.org/v0.js', $scripts->item( 0 )->getAttribute( 'src' ) );
$this->assertEquals( 'amp-mustache', $scripts->item( 1 )->getAttribute( 'custom-template' ) );
$this->assertEquals( 'amp-list', $scripts->item( 2 )->getAttribute( 'custom-element' ) );
}

/**
* Test dequeue_customize_preview_scripts.
*
Expand Down Expand Up @@ -1999,6 +2035,7 @@ static function() {
{ "aExperiment": {} }
</script>
</amp-experiment>
<amp-list src="https://example.com/list.json?RANDOM"></amp-list>
</body>
</html>
<!--comment-after-html-->
Expand Down