Remove duplicate and unused AMP component scripts from the response

[westonruter]

Feature description

If you include more than one copy of an AMP component script on a page, e.g. amp-form, then the validator gives a warning:

In the same way, if there are scripts on the page which are not used, a validation error is also raised:

These issues come up often in the Reader mode templates which don't send the entire templates through the post-processor (which will be fixed in #2202), but it can also happen in Standard/Transitional mode when duplicate scripts are manually output in wp_head or wp_footer actions (which normally should not be done since the plugin adds them already).

Cf. #3253 (comment)

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

The AMP_Theme_Support::ensure_required_markup() method is passed a list of $script_handles for the AMP scripts that are required to be in the page. If those scripts are not already present in the DOM, then they are added:

amp-wp/includes/class-amp-theme-support.php

Lines 1730 to 1746 in 394b202

	// Create scripts for any components discovered from output buffering.
	foreach ( array_diff( $script_handles, array_keys( $amp_scripts ) ) as $missing_script_handle ) {
	if ( ! wp_script_is( $missing_script_handle, 'registered' ) ) {
	continue;
	}
	$attrs = [
	'src' => wp_scripts()->registered[ $missing_script_handle ]->src,
	'async' => '',
	];
	if ( 'amp-mustache' === $missing_script_handle ) {
	$attrs['custom-template'] = $missing_script_handle;
	} else {
	$attrs['custom-element'] = $missing_script_handle;
	}
	$amp_scripts[ $missing_script_handle ] = AMP_DOM_Utils::create_node( $dom, 'script', $attrs );
	}

There are two things missing here:

1. If there is more than one copy of a given script on the page, the duplicates need to be removed.
2. If there are AMP scripts present on the page which are not present in $script_handles then they should be removed, as otherwise there can be an AMP validation error (as they are unused).

QA testing instructions

• Add a "Custom HTML" block to a post.
• Paste the following component into the block:

   <script async custom-element="amp-kaltura-player" src="https://cdn.ampproject.org/v0/amp-kaltura-player-0.1.js"></script>

• Go to the non-AMP version of the page.
• View the source of the page.
• Verify: The "amp-kaltura-player" script can be found in the source of the page.
• Go to the AMP version of the page.
• View the source of the page.
• Verify: The "amp-kaltura-player" script cannot be found in the source of the page.

Demo

Changelog entry

[kienstra]

Hi @westonruter,
Hope you had a great day.

Would the duplicate component scripts, like in the screenshot above, be output with an amp_post_template_head hook, like this?

add_action( 
        'amp_post_template_head', 
        function() {
                // Echo component scripts here.
        }
);

It looks like the recommended way of adding component scripts won't normally have duplicates.

Thanks, Weston!

[westonruter]

@kienstra Sorry for the delay. They could be added in that way, yes, but this won't be the concern in this issue at the moment. Handling scripts added in Reader mode would be fixed as part of #2202, as the entire document would be then sent through the post-processor.

So for this issue, focus on scripts that are added in Standard/Transitional mode. That means scripts that are added at the wp_head or wp_footer actions. I've updated the summary of the issue and provided an implementation brief. The scope is also expanded to include not just duplicate script but also unused scripts.

[kienstra]

Thanks, @westonruter! No problem, I didn't expect or need a quick response, with the stable release and WCUS this past week.

[schlessera]

I'm currently working on this and I'll do two separate PRs, as both the implementation as well as the ramifications are different for the two cases enumerated above.

[westonruter]

I'm currently working on this and I'll do two separate PRs, as both the implementation as well as the ramifications are different for the two cases enumerated above.

The other PR would be about removing the duplicates? It seems to me that the existing PR could include the de-duplication as well.

[schlessera]

The other PR would be about removing the duplicates? It seems to me that the existing PR could include the de-duplication as well.

The thinking was that removing duplicates is a no-brainer, always fixing and never breaking. However, removing unused AMP scripts is more complicated and could potentially lead to us automatically breaking an otherwise working page, because we're missing something.

Therefore, splitting this into two PRs would mean we'll be able to merge one either way, while the other might need to be put on hold.

[csossi]

Verified in QA