Plugin suppression can fail to work during validate requests

[westonruter]

Bug Description

In a support topic I discovered that even though I suppressed Autoptimize, validation errors for it were still showing up when validating the page. However, they would not appear in the admin bar.

I found a way to replicate the issue with the following plugin:

<?php
/**
 * Plugin Name: Auto Add Script to Footer
 */

namespace Auto_Add_Script_To_Footer;

function print_script() {
	echo '<script>document.write("Automatically added to footer!");</script>';
}

function add_wp_footer_hook() {
	add_action( 'wp_footer', __NAMESPACE__ . '\print_script' );
}

function add_wp_hook() {
	add_action( 'wp', __NAMESPACE__ . '\add_wp_footer_hook' );
}

add_action( 'plugins_loaded', __NAMESPACE__ . '\add_wp_hook' );

With that plugin active and suppressed:

Going to an AMP page on the frontend shows no validation errors being detected:

However, if I go to the Validated URL screen then I see an error detected:

I believe it has to do with this logic in AMP_Validation_Manager::wrap_hook_callbacks():

amp-wp/includes/validation/class-amp-validation-manager.php

Lines 1463 to 1481 in c2b9b38

	$original_function = $callback['function'];
	$wrapped_callback = self::wrapped_callback(
	array_merge(
	$callback,
	compact( 'priority', 'source', 'indirect_sources' )
	)
	);
	if ( 1 === $passed_by_ref ) {
	$callback['function'] = static function( &$first, ...$other_args ) use ( &$callback, $wrapped_callback, $original_function ) {
	$callback['function'] = $original_function; // Restore original.
	return $wrapped_callback->invoke_with_first_ref_arg( $first, ...$other_args );
	};
	} else {
	$callback['function'] = static function( ...$args ) use ( &$callback, $wrapped_callback, $original_function ) {
	$callback['function'] = $original_function; // Restore original.
	return $wrapped_callback( ...$args );
	};
	}

The replacement of the original callback with the wrapped callback closure I believe is interfering with PluginSuppression::suppress_hooks() specifically when it calls $this->is_callback_plugin_suppressed( $callback['function'], $suppressed_plugins ):

amp-wp/src/PluginSuppression.php

Lines 368 to 379 in c2b9b38

	private function suppress_hooks( $suppressed_plugins ) {
	global $wp_filter;
	foreach ( $wp_filter as $tag => $filter ) {
	foreach ( $filter->callbacks as $priority => $prioritized_callbacks ) {
	foreach ( $prioritized_callbacks as $callback ) {
	if ( $this->is_callback_plugin_suppressed( $callback['function'], $suppressed_plugins ) ) {
	$filter->remove_filter( $tag, $callback['function'], $priority );
	}
	}
	}
	}
	}

Since at this point $callback['function'] may be the closure and not the original callable from the other plugin, the check fails because the AMP plugin is detected and not the Auto Add Script to Footer function.

What's strange is that this issue specifically happens when the wp action is involved. If I change my test plugin as follows:

function print_script() {
	echo '<script>document.write("Automatically added to footer!");</script>';
}

function add_wp_footer_hook() {
	add_action( 'wp_footer', __NAMESPACE__ . '\print_script' );
}

add_action( 'plugins_loaded', __NAMESPACE__ . '\add_wp_footer_hook' );

Then the suppression works successfully.

Expected Behaviour

The plugin should be suppressed on the frontend and during a validate request.

Screenshots

No response

PHP Version

No response

Plugin Version

2.2

AMP plugin template mode

Transitional

WordPress Version

No response

Site Health

No response

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

[westonruter]

The issue is that the maybe_suppress_plugins method runs at the wp action (unless a Reader theme is being served):

amp-wp/src/PluginSuppression.php

Lines 107 to 123 in c2b9b38

	// When a Reader theme is selected and an AMP request is being made, start suppressing as early as possible.
	// This can be done because we know it is an AMP page due to the query parameter, but it also _has_ to be done
	// specifically for the case of accessing the AMP Customizer (in which customize.php is requested with the query
	// parameter) in order to prevent the registration of Customizer controls from suppressed plugins. Suppression
	// could be done early for Transitional mode as well since a query parameter is also used for frontend requests
	// but there is no similar need to suppress the registration of Customizer controls in Transitional mode since
	// there is no separate Customizer for AMP in Transitional mode (or legacy Reader mode).
	// @todo This check could be replaced with ( ! amp_is_canonical() && $this->paired_routing->has_endpoint() ).
	if ( $this->is_reader_theme_request() ) {
	$this->suppress_plugins();
	} else {
	$min_priority = defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : ~PHP_INT_MAX; // phpcs:ignore PHPCompatibility.Constants.NewConstants.php_int_minFound
	// In Standard mode we _have_ to wait for the wp action because with the absence of a query parameter
	// we have to rely on amp_is_request() and the WP_Query to determine whether a plugin should be suppressed.
	add_action( 'wp', [ $this, 'maybe_suppress_plugins' ], $min_priority );
	}

Since it is running at wp and AMP_Validation_Manager::wrap_hook_callbacks() has already wrapped all the hooks in closures, maybe_suppress_plugins is not able to see the original callback when it runs during that action.

[westonruter]

I think is_callback_plugin_suppressed needs to do something similar to what was done in #6579 to get the original underlying callback:

amp-wp/includes/validation/class-amp-validation-manager.php

Lines 1297 to 1302 in c2b9b38

	$render_callback = $block_type->render_callback;
	// Access the underlying callback which was wrapped by ::wrap_block_callbacks() below.
	while ( $render_callback instanceof AMP_Validation_Callback_Wrapper ) {
	$render_callback = $render_callback->get_callback_function();
	}

[westonruter]

(QA still necessary on 2.2 branch.)

[delawski]

QA Passed

For QA I've used the sample plugin shared in the PR description and AMP Version 2.2.1-alpha-20220127T170248Z-78e27d9.

Before suppressing the plugin, an AMP validation issue was displayed in the WP admin bar, on the AMP Validated URL screen and in the Site Scan panel.

After suppressing the plugin, there was no AMP validation issue in neither of the mentioned locations:

WP admin bar	AMP Validated URL	Site Scan panel