Discontinue doing synchronous validation requests upon activating plugins

[westonruter]

Feature description

Depends on #4719.

Relates to #2069.

In #5100 no longer is a synchronous validation request performed when switching template modes. However, what remains is the synchronous validation request when activating plugins.

The validation errors are obtained in AMP_Validation_Manager::validate_after_plugin_activation() and then displayed via AMP_Validation_Manager::print_plugin_notice().

Instead, upon activating a plugin when the “Plugin activated” notice is displayed:

There should at that point also be an admin notice from the AMP plugin which is accompanied by an enqueued script which does an asynchronous request to check the site for validation issues. A spinner should be shown along with a message saying “checking for AMP compatibility issues”. Then once the results are back, a message can be shown saying there is no validation errors or that there are errors present with those activated plugins, and a link to see what they are.

As it is currently, this admin notice should probably be suppressed if the user doesn't have developer tools enabled.

There could also be a good tie-in here with Plugin Suppression to allow a user to easily deactivate the plugins on AMP responses.

Relates to wizard compatibility scanning in #4795 and #4719.

---

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

Acceptance criteria

Implementation brief

1. All existing server-side synchronous logic is removed.
2. Upon selecting to activate a plugin, the user is immediately taken to the resulting page with the normal "Plugin activated" notice shown.
3. On the resulting page, another "Plugin scanning" notice is displayed with a loading spinner.
4. REST API requests are initiated to do a site scan (Wizard Step: Plugin compatibility scanning #4719).
5. The plugin sources for unreviewed invalid markup are then aggregated from the REST API responses.
6. If there are plugin sources for unreviewed invalid markup:
   1. Give the notice a warning class.
   2. List out the plugins which are causing unreviewed validation errors.
   3. List out the validated URLs on which the errors occur. (The validated URLs on which a plugin's errors occur should probably be listed under the plugin itself.)
   4. Link to Plugin Suppression screen so user has ability to prevent plugins from running on AMP pages. (It's likely overkill to provide a way to suppress plugins directly from this notice.)
7. If there are no unreviewed validation errors, then the notice should be given a success class.

QA testing instructions

Demo

Changelog entry

[jwold]

Is an initial idea with some concepts. What am I missing?

[johnwatkins0]

This looks right to me.

Side note: This will depend on the PR #5306 that I opened a few weeks ago because I think it will use the ValidationURLProvider created in that PR.

[westonruter]

@jwold Yes, that's right. We may want to provide information about suppressing that plugin from AMP pages as well.

I was able to reproduce the existing warning we have:

Here's the current code that is responsible for that:

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

Lines 211 to 220 in 5f42cde

	// Add actions for checking theme support is present to determine plugin compatibility and show validation links in the admin bar.
	// Actions and filters involved in validation.
	add_action(
	'activate_plugin',
	static function() {
	if ( ! has_action( 'shutdown', [ __CLASS__, 'validate_after_plugin_activation' ] ) && self::get_dev_tools_user_access()->is_user_enabled() ) {
	add_action( 'shutdown', [ __CLASS__, 'validate_after_plugin_activation' ] ); // Shutdown so all plugins will have been activated.
	}
	}
	);

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

Lines 1972 to 1993 in 5f42cde

	/**
	* Validates the latest published post.
	*
	* @return array|WP_Error The validation errors, or WP_Error.
	*/
	public static function validate_after_plugin_activation() {
	$url = amp_admin_get_preview_permalink();
	if ( ! $url ) {
	return new WP_Error( 'no_published_post_url_available' );
	}
	$validity = self::validate_url_and_store( $url );
	if ( is_wp_error( $validity ) ) {
	return $validity;
	}
	$validation_errors = wp_list_pluck( $validity['results'], 'error' );
	if ( is_array( $validity ) && count( $validation_errors ) > 0 ) { // @todo This should only warn when there are unaccepted validation errors.
	set_transient( self::PLUGIN_ACTIVATION_VALIDATION_ERRORS_TRANSIENT_KEY, $validation_errors, 60 );
	} else {
	delete_transient( self::PLUGIN_ACTIVATION_VALIDATION_ERRORS_TRANSIENT_KEY );
	}
	return $validation_errors;
	}

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

Lines 2319 to 2374 in 5f42cde

	/**
	* On activating a plugin, display a notice if a plugin causes an AMP validation error.
	*
	* @return void
	*/
	public static function print_plugin_notice() {
	global $pagenow;
	if ( ( 'plugins.php' === $pagenow ) && ( ! empty( $_GET['activate'] ) || ! empty( $_GET['activate-multi'] ) ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
	$validation_errors = get_transient( self::PLUGIN_ACTIVATION_VALIDATION_ERRORS_TRANSIENT_KEY );
	if ( empty( $validation_errors ) || ! is_array( $validation_errors ) ) {
	return;
	}
	delete_transient( self::PLUGIN_ACTIVATION_VALIDATION_ERRORS_TRANSIENT_KEY );
	$errors = AMP_Validation_Error_Taxonomy::summarize_validation_errors( $validation_errors );
	$invalid_plugins = isset( $errors[ AMP_Validation_Error_Taxonomy::SOURCES_INVALID_OUTPUT ]['plugin'] ) ? array_unique( $errors[ AMP_Validation_Error_Taxonomy::SOURCES_INVALID_OUTPUT ]['plugin'] ) : null;
	if ( isset( $invalid_plugins ) ) {
	$reported_plugins = [];
	$plugin_registry = Services::get( 'plugin_registry' );
	foreach ( $invalid_plugins as $plugin_slug ) {
	$plugin_data = $plugin_registry->get_plugin_from_slug( $plugin_slug );
	$plugin_name = is_array( $plugin_data ) ? $plugin_data['data']['Name'] : $plugin_slug;
	$reported_plugins[] = $plugin_name;
	}
	$more_details_link = sprintf(
	'<a href="%s">%s</a>',
	esc_url(
	add_query_arg(
	'post_type',
	AMP_Validated_URL_Post_Type::POST_TYPE_SLUG,
	admin_url( 'edit.php' )
	)
	),
	__( 'More details', 'amp' )
	);
	printf(
	'<div class="notice notice-warning is-dismissible"><p>%s %s</p><button type="button" class="notice-dismiss"><span class="screen-reader-text">%s</span></button></div>',
	esc_html(
	sprintf(
	/* translators: %s is comma-separated list of one or more plugins */
	_n(
	'Warning: The following plugin may be incompatible with AMP: %s.',
	'Warning: The following plugins may be incompatible with AMP: %s.',
	count( $invalid_plugins ),
	'amp'
	),
	implode( ', ', $reported_plugins ) // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
	)
	),
	$more_details_link, // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
	esc_html__( 'Dismiss this notice.', 'amp' )
	);
	}
	}
	}

[jwold]

@westonruter are you suggesting that we allow users to disable the offending plugin directly from the notice? That could work when a single plugin is throwing errors, but might get messy if multiple plugins are mentioned in the same notice.

[westonruter]

That's true. So we should probably just add a link to go to the Plugin Suppression section so the user can decide which plugins to suppress from there.

[jwold]

From our meeting today:

This is blocked by plugin compatibility scanning. When we have the endpoint for scanning, we can delete the server side PHP code. Right now when a user activates a plugin there's a loopback request server side, this slows activation fo the plugin. Can take 15-20 seconds to activate. This has been fixed in the post editor by making validation async.

We can do the same here, upon activating a plugin we show an admin notice that's initially PHP rendered, "checking site for compatibility," then use JS to fetch the results and put them with an admin notice loading spinner and show the results.

[kenrodriguesdesign]

Hey @westonruter,
The UX UI team discussed an alternate flow to this issue and we concluded that this flow would be better from a UX perspective.

1. Select the “Activate” action.
2. We can intercept the activation of the plugin by using this https://developer.wordpress.org/reference/functions/register_activation_hook/
3. Once intercepted, the AMP Plugin can check for compatibility issues.
4. Based on the results:
   1. If there are no errors then the activation of the plugin continues.
   2. If there are errors the (warning) notice will include CTA's to continue the activation or not.
   

If the user selects to Activate the plugin, then:

1. For technical users we can redirect them to the errors page.

2. For non technical users, we could recommend to suppress the plugin.

I would love to know your thoughts on this, so do let me know.

[westonruter]

2. We can intercept the activation of the plugin by using this https://developer.wordpress.org/reference/functions/register_activation_hook/
3. Once intercepted, the AMP Plugin can check for compatibility issues.

I don't think this will be feasible because the activation hook happens when activation happens and it doesn't provide an API to intercept activation.

If you try to output something in the activation hook, WordPress core outputs this notice:

If you try to die in the activation hook, then a different notice is presented:

In this case the plugin is not activated at all.

In order to actually evaluate whether the plugin is causing any AMP/PX issues, the plugin has to be fully active so that we can do requests on the site to see its output.

So instead of there being a notice to continue activation after errors are encountered, it needs to instead show a notice with a "Checking for plugin compatibility…" message which is then populated asynchronously after the site scan results are obtained. The info notice would change to a success status if no errors for the activated plugins are found, or else the status would change to a warning if there were issues encountered.

In the case of encountered errors, the user could then have the option to suppress the plugin from running on AMP pages as you've depicted, but as we discussed earlier today we want to move away from suppression as being the first choice, especially when Standard mode is selected (which we want to default to in the future). Plugin suppression is really only a viable option when in a paired AMP scheme (Transitional mode or Reader mode). If we keep the plugin suppression CTA, it may make sense just as a link to the Plugin Suppression table on the settings screen.

For technical users, the "view errors" link should probably be replaced with one or more links to Validated URL screens. If we only validate one URL after activating a plugin, then there could be just one link, but I'm thinking it will probably be best to do a full site scan (i.e. checking ~10 URLs) and then to debug the errors the user should be presented with links to each of the validated URL screens on which the activated plugin(s)' error(s) are present.

[kenrodriguesdesign]

Hey @westonruter

Okay, I understood the flow that you've stated above and the errors that come along with it. So in this case we'll revert to the flow mentioned in the Implementation brief.

For the remaining comments:

So instead of there being a notice to continue activation after errors are encountered, it needs to instead show a notice with a "Checking for plugin compatibility…" message which is then populated asynchronously after the site scan results are obtained.

The info notice would change to a success status if no errors for the activated plugins are found, or else the status would change to a warning if there were issues encountered.

If we keep the plugin suppression CTA, it may make sense just as a link to the Plugin Suppression table on the settings screen.

Yes, at present, for non-technical users it'll be a link to the plugin suppression section in the AMP settings screen.

For this comment:

If we only validate one URL after activating a plugin, then there could be just one link, but I'm thinking it will probably be best to do a full site scan (i.e. checking ~10 URLs) and then to debug the errors the user should be presented with links to each of the validated URL screens on which the activated plugin(s)' error(s) are present.

I do agree with the first statement, that we could show one link for the Validated URL.

But for Multiple URLs i.e. site scan, showing each and every URL will not be feasible and could cause clutter.

I do have an idea for this though.
What if we show the list of URLs affected by the recently scanned plugin i.e. filter Validated URL according to the plugin.

Do let me know if that's possible.

If not, then would you recommend the link just directs to the list of Validated URLs?

[westonruter]

What if we show the list of URLs affected by the recently scanned plugin i.e. filter Validated URL according to the plugin.

In the case of there being validation errors detected for the newly activated plugin(s), I think there should be (1) a link to the Plugin Suppression section, and (2) a list of links to Validated URLs on which the plugins' have errors occurring.

The list of validated URLs could appear collapsed in a details element with the summary being Found issues on X URL(s), and then expanding that summary could then show the individual links to the validated URLs.

Then after that there can be the CTA to review the plugin suppression settings.

Lastly, this would be another opportunity to mention the collection of AMP-compatible plugins. Once we have the ecosystem data integrated into the plugin directory (#4668), there could be a third CTA to "Consider alternative AMP-compatible plugins" that links to the tab in the Add Plugins screen that lists out AMP-compatible plugins.

[kenrodriguesdesign]

In the case of there being validation errors detected for the newly activated plugin(s), I think there should be (1) a link to the Plugin Suppression section, and (2) a list of links to Validated URLs on which the plugins' have errors occurring.

Okay, understood.
But I think we should place the CTA for Plugin Suppression settings above the List of validated URLs rather than after the list.

Sending an Updated UI design for this.
For now, I have taken 3 URLs as an example, do let me know the number of URLs we decide to scan.

Collapsed view:

Expanded view:

Once the ecosystem data is integrated into the plugin directory, then we could have this version of the design.

Collapsed View:

Expanded View:

Let me know your thoughts.

[westonruter]

Yeah, I think that's good.

For now, I have taken 3 URLs as an example, do let me know the number of URLs we decide to scan.

The number will be variable, but figure on maybe up to 10. In any case, the list is collapsed by default so I don't think that's a problem.

[kenrodriguesdesign]

Adding a Figma link for reference and copy as well.

https://www.figma.com/file/SfMlDvHc5KHxJmAZN12PIM/AMP-Design?node-id=2903%3A22353

[fellyph]

Hi @westonruter,

I have tested this item the validation work as expected if I activate a valid and invalid plugin:

Valid Scenario:

Invalid scenario:

Some edge cases:

1. If I activated multiple plugins, the check doesn't mention which one was checked
2. If I install, for example, elementor and go to review suppression, and go back to plugins list and activate a compatible plugin, I will get one confirmation and the verification from elementor appearing again, is this the behavior expected?

3. When I click on "Review plugin suppression" the next page just scroll halfway to the sections:
   

[westonruter]

• If I activated multiple plugins, the check doesn't mention which one was checked

@fellyph That's correct. It will return results for all plugins, not just the one(s) activated. This was something that @delawski noted as well, but there's not a clean way to determine which plugin(s) have just been activated. And it's not entirely needed anyway.

2. If I install, for example, elementor and go to review suppression, and go back to plugins list and activate a compatible plugin, I will get one confirmation and the verification from elementor appearing again, is this the behavior expected?

@fellyph Are you saying that you have suppressed Elementor, after activating another plugin you still see validation errors coming from Elementor? I'm not seeing that. After I select “Suppressed” for Elementor in Plugin Suppression and Save:

I can activate another plugin and I don't see Elementor listed anymore:

3. When I click on "Review plugin suppression" the next page just scroll halfway to the sections:

I think this has to do with certain sections growing in height after the initial scrolling has been done. I suggest opening a low-priority issue regarding that.

[fellyph]

QA Passed

I will open, low priority issues for the small items