Add debugging query arguments

[kienstra]

Summary

Adds query arguments for debugging under the amp_flags query arg. For example: https://example.com/baz-post/?amp&amp_flags[disable_post_processing]=1

Query argument	Description
disable_post_processing	Prevents output buffering and sanitizers from running
disable_response_cache	Prevents the response from being cached
prevent_redirect	Prevents redirecting from an AMP URL to a non-AMP URL, like if there are unaccepted validation errors in Transitional mode
reject_all_errors	Automatically sets all errors to 'ack rejected'
accept_excessive_css	Accepts all excessive_css errors. Tree shaking still takes place, but no styling is removed because it's over the limit.
disable_amp	Disables AMP for the URL
disable_tree_shaking	Disables tree shaking

@todo:

• [x] Exclude sanitizers (already covered in disable_post_processing)
• [x] Add a UI or wiki page, documenting the query arguments (Update: add a UI like this)

Addresses issue #1294

Checklist

• [x] My pull request is addressing an open issue (please create one otherwise).
• [x] My code is tested and passes existing tests.
• [x] My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[westonruter]

• Add a UI or wiki page, documenting the query arguments

I think these would work well as toggles in an submenu item of the AMP admin bar menu item, if the user has the capability to use them. Clicking on a nav menu item can toggle the option on/off as it reloads the page.

[kienstra]

Hi @westonruter,

I think these would work well as toggles in an submenu item of the AMP admin bar menu item, if the user has the capability to use them. Clicking on a nav menu item can toggle the option on/off as it reloads the page.

Ah, interesting. Something like this?

[westonruter]

Yes, something like that. I realize that some of the toggles are mutually-exclusive, so that complicates it a bit.

[kienstra]

Ah, the commit above needs reworking.

[swissspidy]

Should the query vars be prefixed to not cause conflicts with other plugins?

[kienstra]

Hi @swissspidy,

Should the query vars be prefixed to not cause conflicts with other plugins?

Good question. https://github.com/ampproject/amp-wp/pull/3448/commits/19363fc23939a38173ecfe0bd4d4baad3de2583a adds an amp_flags query var, to access these query vars like:

https://loc.test/your-post/?amp&amp_flags[reject_all_errors]

...which is accessed like:

$_GET[ AMP_Theme_Support::AMP_FLAGS_QUERY_VAR ][ self::REJECT_ALL_VALIDATION_ERRORS_QUERY_VAR ] )

[kienstra]

The UI for these query vars still needs a lot of work, but here's its current state:

[westonruter]

The UI for these query vars still needs a lot of work, but here's its current state:

@kienstra I think the submenu could be made a bit simpler by removing the checkboxes. In other words, each of the nav menu items would be a link which would be preceded by some on/off indicator. Clicking the link would cause the page to reload with the query var update(s), and opening the submenu would then show the toggle indicator had been flipped.

[kienstra]

Hi @westonruter, that's a really good idea. It'll make this much simpler.

[kienstra]

Here's the PHP-rendered submenu. Each node, like 'Disable post-processing', is a link to the AMP URL with that query var.

[kienstra]

There will be a need to set the font-family explicitly to prevent the first one from being rendered as an emoji. So it can be wrapped in a span.

Also, did you have a font-family in mind? This uses some fonts from admin-bar.css, but doesn't use -apple-system, BlinkMacSystemFont.

[schlessera]

As debugging is obviously a big and important part of using the AMP plugin, I think sprinkling debugging code throughout the other pieces of logic too much is a problem in terms o technical debt.

I suggest adding a separate class that deals with this responsibility and abstracts away the details (abbreviated, untested example):

final class AMP_Debug {

	/**
	 * A top-level query var for AMP debug flags.
	 *
	 * @var string
	 */
	const QUERY_VAR = 'amp_flags';

	/**
	 * A query var to disable post processing.
	 *
	 * @var string
	 */
	const DISABLE_POST_PROCESSING = 'disable_post_processing';

	/**
	 * A query var to disable the response cache.
	 *
	 * @var string
	 */
	const DISABLE_RESPONSE_CACHE = 'disable_response_cache';

	/**
	 * A query var to prevent a redirect to a non-AMP URL.
	 *
	 * @var string
	 */
	const PREVENT_REDIRECT_TO_NON_AMP = 'prevent_redirect';

	/**
	 * Check whether a given debug flag is currently set.
	 *
	 * @param string $flag Flag to check.
	 * @return bool Whether the given debug flag is set.
	 */
	public static function has_flag( $flag ) {
		static $debug_flags = null;

		if ( null === $debug_flags ) {
			$debug_flags = filter_input_array(
				INPUT_GET,
				[
					AMP_Debug::QUERY_VAR => [
						AMP_Debug::DISABLE_POST_PROCESSING     => FILTER_VALIDATE_BOOLEAN,
						AMP_Debug::DISABLE_RESPONSE_CACHE      => FILTER_VALIDATE_BOOLEAN,
						AMP_Debug::PREVENT_REDIRECT_TO_NON_AMP => FILTER_VALIDATE_BOOLEAN,
						// [...]
					],
				]
			);
		}

		return array_key_exists( $flag, $debug_flags ) ? (bool) $debug_flags[ $flag ] : false;
	}
}

This allows us to keep everything in one place that is directly related to dealing with that (like adding the debugging menu to the admin bar), and also lets us streamline the naming (as we have the class itself as the context.

Then, within the other logic, a check turns from this:

isset( $_GET[ AMP_Theme_Support::AMP_FLAGS_QUERY_VAR ][ AMP_Theme_Support::DISABLE_POST_PROCESSING_QUERY_VAR ] )

into this:

AMP_Debug::has_flag( AMP_Debug::DISABLE_POST_PROCESSING )

Also the above now properly respects the expectations we have for booleans, like returning false if the URL variable is ?amp_flags[disable_post_processing=false].

[kienstra]

Hi @schlessera, That's a really good idea to use AMP_Debug, preventing the logic from being mixed into several classes.

[kienstra]

With https://github.com/ampproject/amp-wp/pull/3448/commits/996157a242412354099945d448d5a17b72f10b1a, the behavior of the debugging query vars is a little different.

For example, here are the return values of AMP_Debug::has_flag( 'reject_all_errors' ):

True

&amp_flags[reject_all_errors] (no value) &amp_flags[reject_all_errors]=true &amp_flags[reject_all_errors]=1

False

&amp_flags[reject_all_errors]=false &amp_flags[reject_all_errors]=0 &amp_flags[reject_all_errors]=made-up &amp_flags[reject_all_errors]=5555

[schlessera]

With 996157a, the behavior of the debugging query vars is a little different.

What about the flag without value?

&amp_flags[reject_all_errors]

[kienstra]

What about the flag without value?

Ah, good question. That'll be false.

[kienstra]

Request For Another Round Of Review

Hi @westonruter and @schlessera, Thanks for your help on this.

When you have a chance, could you please do another round of review?

The table of flags above is still up-to-date.

And here are the values that the query var can have.

[schlessera]

Ah, good question. That'll be false.

@kienstra Would it make sense to change this so that no flag value means true?

[kienstra]

@kienstra Would it make sense to change this so that no flag value means true?

Sure, that's the behavior for the amp query var.

[kienstra]

Excellent PR so far @kienstra. The only concern I have right now is that with the use of checkboxes it gives the impression that multiple flags can be assigned at once, which is not so. I would suggest using something similar to the radio input.

Hi @pierlon, Thanks a lot for reviewing this.

That's an interesting suggestion. It's possible, though not likely, that 2 debug flags would be present at once.

Still, one would have to manually add those query vars, it's not possible to get 2 debugging flags by simply clicking in the admin bar.

For example: https://example.com/?amp&amp_flags[reject_all_errors]&amp_flags[prevent_redirect]

[westonruter]

That's an interesting suggestion. It's possible, though not likely, that 2 debug flags would be present at once.

Ah, this actually is the intention here, to allow multiple debug flags to be checked at once. A tricky bit, however, is that various debug flags are mutually-exclusively (https://github.com/ampproject/amp-wp/pull/3448#issuecomment-538585575), so there would need to be some logic that removes/disables some of the options based on the state of the others.

[westonruter]

@kienstra Let's put this on pause until we can flesh out more of the acceptance criteria and determine how the different options relate to each other, and now those options apply in Standard vs Transitional modes.

[kienstra]

Sounds good, thanks

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

:white_check_mark: westonruter
:x: kienstra
_{You have signed the CLA already but the status is still pending? Let us recheck it.}