Add AMP compatibility

[westonruter]

This PR is revisiting one I previously closed (#332) which prevented Query Monitor from doing anything on AMP pages. This is no longer necessary now that AMP Dev Mode is a thing: Query Monitor's full functionality can exist on AMP pages. This follows up on a discussion that we had with @schlessera around a table during the WordFest Party at WCUS 2019.

---

When the official AMP plugin is active on a site, the functionality provided by Query Monitor is broken on AMP pages. For example, when looking at an AMP page and AMP Developer Tools is enabled, the Admin Bar shows AMP validation errors being reported and that the AMP CSS limit is close to being breached:

When clicking the Query Monitor admin bar menu item, nothing happens:

This is expected because the invalid AMP markup is being removed by the plugin, which is indicated by the raising of validation errors. Clicking to validate the URL to show the validation results shows 5 script elements that are getting removed due to being invalid AMP (which doesn't permit custom scripts in the traditional way):

The last two inline scripts lack attribution detection from the AMP plugin, but the penultimate inline script is coming from QM_Dispatcher_Html::before_output() which prints var qm = {...}, and the last inline script is the load event handler on the window that is also printed in the same method.

The validation errors for these scripts can be avoided by designating them as being part of AMP Dev Mode. This allows for AMP-invalid scripts (and other elements) from being sanitized when a page is in AMP Dev Mode which happens by default when the user is logged-in and the Admin Bar is showing. Designating elements to be exempted from sanitization is done by adding the data-ampdevmode attribute. This can either be done during template rendering, such as via the script_loader_tag filter, or afterward during post-processing via the amp_dev_mode_element_xpaths filter.

When those are added, no more validation errors are reported:

In addition to designating invalid scripts as being part of AMP Dev Mode, this should also be done for stylesheets that are related to those scripts which only appear in Dev Mode. This will prevent validation errors due to going over the CSS limit but it will also prevent required CSS rules from being removed during tree shaking. So this PR also designates query-monitor and dashicons as also being part of AMP Dev Mode so that they are omitted from the AMP plugin's CSS processing. Notice how the CSS usage is decreased:

Before	After

Another key difference to point out here is that before Query Monitor is showing up as the source for various stylesheets that it is definitely not responsible for registering: wp-block-library-theme, wp-block-library, and amp-default. This is prevented in this PR with the addition of this bit of code in query-monitor.php:

if ( isset( $_GET['amp_validate'] ) ) {
	# Prevent loading QM during AMP validation requests since it conflicts with the AMP plugin's own reflection routines.
	return;
}

I couldn't determine a better place for this logic. The condition isn't particularly robust either, but I was unsure of how else to validate it being a valid AMP validation request so early. (One idea could be to check if hash_equals( $_GET['amp_validate'], wp_hash( self::VALIDATE_QUERY_VAR . wp_nonce_tick(), 'nonce' ) ) but this would require loading pluggable.php too early.)

When clicking “Validate URL” in the AMP plugin, a loopback request is initiated in which the amp_validate query param is added to indicate that an AMP validation request is being made. See logic in AMP_Validation_Manager::validate_url(). As part of a validation request, the AMP plugin adds similar logic that Query Monitor adds to determine which theme/plugin is responsible for a given asset being added to the page. Apparently Query Monitor's source attribution logic is confusing the AMP plugin's source attribute logic, since they probably do similar things. Since it doesn't make sense for Query Monitor to be running during the loopback request for AMP validation, to me it seems the logical course of action is to just prevent Query Monitor from running during such requests.

So, after the changes in this PR, Query Monitor's functionality works properly on the frontend on AMP pages:

WIth bf5f446, Query Monitor is also successfully integrated with the legacy AMP post templates:

[johnbillion]

I'll be honest, this is a lot of AMP-specific implementation detail that I'm not particularly interested in maintaining. I assume that other plugins which need to do the same thing will also be mostly repeating this logic.

I'd like to recommend that the AMP plugin provides a declarative integration layer to simplify this, like the existing amp_dev_mode_element_xpaths filter. For example in Query Monitor I'd be happy to declare via a filter those assets in QM which should be treated as development-only:

add_filter( 'amp_dev_mode_scripts', function( $scripts ) {
	$scripts[] = 'query-monitor';
	return $scripts;
} );

add_filter( 'amp_dev_mode_styles', function( $styles ) {
	$styles[] = 'query-monitor';
	return $styles;
} );

Then the AMP-specific implementation details (adding the data-ampdevmode attribute to those assets and their dependencies) can be handled internally by the AMP plugin and I don't need to worry about them, and I don't need to worry about maintaining the accuracy of the AMP request detection.

For inline scripts the existing amp_dev_mode_element_xpaths is fine, although the XPaths can be simplified here by adding an id attribute to each of the inline script elements.

Regarding the AMP validation loopback request, this is another piece of AMP implementation detail that will likely need maintaining, but I can't see an alternative. If the AMP plugin provided a wrapper function for this (eg. amp_is_validation_request()) then QM couldn't rely on it being available because QM forces itself to the front of the plugin load order. We might need to live with this.

[westonruter]

I'll be honest, this is a lot of AMP-specific implementation detail that I'm not particularly interested in maintaining. I assume that other plugins which need to do the same thing will also be mostly repeating this logic.

You're right. We actually have an issue pending for this (ampproject/amp-wp#4598) which will greatly reduce the amount code needed to identify scripts/styles for AMP Dev Mode.

We'll make progress on that issue and then update this PR to make use of the new API.

For inline scripts the existing amp_dev_mode_element_xpaths is fine, although the XPaths can be simplified here by adding an id attribute to each of the inline script elements.

This would mean adding id attributes to the scripts that QM is outputting:

query-monitor/dispatchers/Html.php

Lines 325 to 328 in fbde50d

	echo '<!-- Begin Query Monitor output -->' . "\n\n";
	echo '<script type="text/javascript">' . "\n\n";
	echo 'var qm = ' . json_encode( $json ) . ';' . "\n\n";
	echo '</script>' . "\n\n";

query-monitor/dispatchers/Html.php

Lines 584 to 586 in fbde50d

	echo '<script type="text/javascript">' . "\n\n";
	?>
	window.addEventListener('load', function() {

So add the id attribute for these scripts and then use them in the XPath expressions? How about query-monitor-data and query-monitor-load, respectively?

[johnbillion]

So add the id attribute for these scripts and then use them in the XPath expressions? How about query-monitor-data and query-monitor-load, respectively?

Yep, sounds good to me

[westonruter]

Note: This expression will only work in WP≥5.5: WordPress/wordpress-develop@d241ab3

I suppose we could do a version check and fall back to using '//script[ contains( text(), "var qm_number_format =" ) ]' if on WP<5.5.

[archon810]

@westonruter Fancy seeing you here. Thanks for raising this, we just ran into this ourselves.

[rickalee]

We have an AMP-first site I would love to see this on.

[westonruter]

@rickalee Try this compatibility plugin in the meantime: https://gist.github.com/westonruter/621137b5a5ae1caaaee48c63f61ce7b7

[rickalee]

I would note that AMP validator flags td scope="row" within Query Monitor tables as invalid as well.

https://rickal.ee/6quxQG2q

[westonruter]

I would note that AMP validator flags td scope="row" within Query Monitor tables as invalid as well.

Ah, well that appears to be a bug in Query Monitor, as the use of the scope attribute on the td element is deprecated in HTML (as it is supposed to be on th instead):

Nevertheless, I'm not seeing Query Monitor emit any such markup:

[johnbillion]

There were a couple of instances of td scope="row" in QM, which I removed in d8e9a0a.

[johnbillion]

Thanks for the work on this but I'm going to close it off. It's a fair amount of implementation detail that I'm not interested in maintaining, there's never been any other requests to support the AMP plugin, and there is a compatibility plugin available from Weston for those who need it.

Cheers!