Add ability to suppress plugins from AMP responses by westonruter · Pull Request #4657 · ampproject/amp-wp

westonruter · 2020-05-05T16:10:17Z

Summary

Fixes #4477.
Fixes #3474.

This PR introduces a new Suppressed Plugins (Plugin Suppression) section in the AMP settings screen which is shown when:

There are plugins which have been detected to cause validation errors (in non-stale validated URLs).
Such plugins have been suppressed.

Old screenshot

The Suppressed Plugins section is omitted entirely if no validation errors are being caused by plugins.

When a plugin is causing validation errors or it has been suppressed, it will appear in the list of initially-visible plugins with select dropdowns. The other plugins are hidden, and the entire section will appear only if there are any plugins that have been suppressed or if there are other plugins which are causing validation errors. When there are validation errors associated with a given plugin, the errors will be listed with each plugin to provide you with a quick way to evaluate whether the errors are serious enough to warrant turning off the plugin on AMP responses. Validation errors which are bolded are those which are unreviewed. Validation errors with markup removed have a green bullet, and validation errors with red markup have a red bullet.

Old screenshot

The validation errors are only accounted for when the recent validated URLs are not stale. In other words, when a plugin has been updated and the validated URLs were last checked with an older version of the plugin, then the plugins won't be listed among those which are candidates for suppression. Similarly, once a plugin has been suppressed and then the user updates the plugin, a message will be added next to the plugin to indicate that a new version has been installed and that it should perhaps be re-checked for validity since the issue may not be present anymore:

Old screenshot

Plugins are suppressed by:

Unhooking filters and actions whose callbacks are defined in the plugin.
Overriding shortcodes with plugin-defined callbacks to just __return_empty_string.
Overriding widgets with plugin-defined callbacks to just __return_null. In addition to this, the display is also blocked with the widget_display_callback filter to ensure widgets rendered with the_widget() are accounted for.
Overriding dynamic blocks with plugin-defined callbacks to just __return_empty_string and to also nullify the frontend style and script.

Some plugins to help with testing (which have also been adapted to be included as part of the tests):

When all of those plugins are active and a post is validated which has the shortcode, widget, and block all present on its template, the validation errors will show as:

When all of these are suppressed, no validation errors should be raised by any of them:

Checklist

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

westonruter · 2020-05-08T17:15:13Z

This isn't ready for review yet.

src/PluginSuppression.php

includes/options/class-amp-options-menu.php

westonruter · 2020-05-13T06:11:22Z

I still have to add tests, but this is ready for review otherwise.

includes/options/class-amp-options-manager.php

includes/options/class-amp-options-menu.php

pierlon · 2020-05-15T05:44:54Z

includes/validation/class-amp-validated-url-post-type.php

 	 *     @type string $theme   The theme that was active but is no longer. Absent if theme is the same.
-	 *     @type array  $plugins Plugins that used to be active but are no longer, or which are active now but weren't. Absent if the plugins were the same.
+	 *     @type array  $plugins Plugins that used to be active but are no longer, or which are active now but weren't. Also includes plugins that have version updates. Absent if the plugins were the same.
 	 *     @type array  $options Options that are now different. Absent if the options were the same.


Is an array of stale options still supposed to be returned here?

Actually, you identified some code that needed to be resurrected to address #3474. Resurrection implemented in 4e1c478.

includes/validation/class-amp-validated-url-post-type.php

pierlon · 2020-05-15T06:39:40Z

includes/options/class-amp-options-menu.php

+			<ul>
+				<?php foreach ( $plugins as $plugin_file => $plugin ) : ?>
+					<?php
+					$plugin_slug = strtok( $plugin_file, '/' );


What about in (rare) cases where multiple plugins can be found in the same directory?

Yeah, it's a good point. The reality is that AMP_Validation_Manager::get_source() is not able to detect such nested plugins anyway. The source slug extracted from a plugin will always be just the directory name if the plugin is not a single file:

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

Lines 1636 to 1639 in 9d662d0

if ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WP_PLUGIN_DIR ) ), ':' ) . $slug_pattern . '(/(?P<file>.*$))?:s', $file, $matches ) ) {

$source['type'] = 'plugin';

$source['name'] = $matches['slug'];

$source['file'] = isset( $matches['file'] ) ? $matches['file'] : $matches['slug'];

So that is why the the directory alone is being considered (or else just the file name if the plugin is not in a directory).

pierlon · 2020-05-15T06:43:34Z

includes/options/class-amp-options-menu.php

+						</label>
+						<?php if ( $is_suppressed && version_compare( $suppressed_plugins[ $plugin_slug ][ Option::SUPPRESSED_PLUGINS_LAST_VERSION ], $plugins[ $plugin_file ]['Version'], '!=' ) ) : ?>
+							<small>
+								<?php if ( $suppressed_plugins[ $plugin_slug ][ Option::SUPPRESSED_PLUGINS_LAST_VERSION ] && $plugins[ $plugin_file ]['Version'] ) : ?>


Should this condition be checked before comparing versions above?

This is here in the rare situation where a plugin was updated to add a version which previously lacked it, or it was updated to remove the version where it previously had one.

pierlon · 2020-05-15T17:46:40Z

This should also address #3474.

westonruter · 2020-05-27T17:25:18Z

Thoughts on the use of strike-through styling when a plugin is checked? I realize that may not be so helpful.

amedina · 2020-05-27T20:22:58Z

I personally don't like the strike through approach much. IIUC the goal is to make sure users understand clearly that the given plugin has been disabled on AMP. I would like to see that clearly conveyed. Perhaps a label next to the plugin name?

westonruter · 2020-05-27T21:10:27Z

I personally don't like the strike through approach much. IIUC the goal is to make sure users understand clearly that the given plugin has been disabled on AMP. I would like to see that clearly conveyed. Perhaps a label next to the plugin name?

I think perhaps the strike-through is entirely redundant. The checked state indicates what is suppressed. I'll just remove the styling.

pierlon · 2020-05-28T09:15:22Z

I think perhaps the strike-through is entirely redundant. The checked state indicates what is suppressed.

Agreed. I also think the updated suppressed plugins could be more indicative. Maybe having them highlighted, or even put into a separate group, for example.

schlessera · 2020-05-28T12:22:16Z

I think the checkboxes might visually indicate the opposite of what they should. Normally, a checked checkbox indicates "enabled", "on", "active", ...

How about using similar comboboxes than with the validation error lists here, to keep the same language? Instead of Removed/Kept, they could say Suppressed/Active or so.

westonruter · 2020-05-31T06:43:16Z

@schlessera @pierlon What do you think of this:

westonruter · 2020-05-31T18:04:41Z

I'm not entirely sure of the title of the “Details” column. I added a datetime at which the plugin was suppressed in order to avoid the cell from being blank. I've also added the plugin URL, description and author so that the user can get more context for the plugin to decide whether it is safe to suppress:

Aside: Someone should really reach out to that Weston Ruter guy to educate him on how to write plugins so they don't cause validation errors!

amedina · 2020-06-01T16:41:07Z

I am not too keen on putting the details of the offending plugin functionality on this screen. The amount of real estate is very limited and the details may not convey the right information.

amedina · 2020-06-01T16:46:44Z

The description of the Suppressed plugins section is too verbose. We should simplify it and link to associated documentation if needed.

amedina · 2020-06-01T16:47:41Z

Consider using Blocked plugins in lieu of Suppressed plugins

westonruter · 2020-06-01T21:17:11Z

Consider using Blocked plugins in lieu of Suppressed plugins

I still prefer “suppressed” in favor of “blocked”. The term “block” is getting overly used in WordPress now, and I think “suppress” precisely depicts what is going on: “prevent the development, action, or expression of (a feeling, impulse, idea, etc.); restrain.”

I am not too keen on putting the details of the offending plugin functionality on this screen. The amount of real estate is very limited and the details may not convey the right information.

I'm not sure. Where else would the information be displayed? The details elements are collapsed by default. How would someone discover understand what validation errors are being caused by the plugin, or to what extent a plugin is causing validation errors?

It's important for there to be as much information here as possible to help the user make a judgement as to whether the plugin can indeed be suppressed. This is why I added links off to the plugin homepage, the author name, and the plugin description.

In general, it could feel somewhat familiar to the plugins list table on the Plugins screen. That's kinda what I was aiming for.

The description of the Suppressed plugins section is too verbose. We should simplify it and link to associated documentation if needed.

Agreed. It should be made more concise. But a benefit of it being in the plugin is that it will get translated. If we link off to docs on amp-wp.org then it won't get the benefit of translations (at least via translate.wordpress.org).

Nevertheless, it's important to note that this section only appears when there are plugins that are causing validation errors. Otherwise, the section is not shown at all. So in an ideal world, the appearance of this section should an be “exception”.

westonruter · 2020-06-01T21:42:18Z

In general, it could feel somewhat familiar to the plugins list table on the Plugins screen. That's kinda what I was aiming for.

In general, a table is able to present more more information in a structured way. My initial design of displaying the information in a linear list was inadequate for that purpose.

amedina · 2020-06-01T21:46:08Z

I still prefer “suppressed” in favor of “blocked”. The term “block” is getting overly used in WordPress now, and I think “suppress” precisely depicts what is going on: “prevent the development, action, or expression of (a feeling, impulse, idea, etc.); restrain.”

Makes sense.

I'm not sure. Where else would the information be displayed? The details elements are collapsed by default. How would someone discover understand what validation errors are being caused by the plugin, or to what extent a plugin is causing validation errors?

Is the goal to provide information to the user about the functionality of the plugin? And then the assumption is that users will use that information to get a better understanding of the potential issues being reported?

If that is the goal, my concern is that the functionality of a plugin is hard to capture in one sentence or so. For example, how to define the functionality of Jetpack? And having either a truncated description may not serve the purpose we want, and if we have a full description the UI can be cluttered.

It's important for there to be as much information here as possible to help the user make a judgement as to whether the plugin can indeed be suppressed. This is why I added links off to the plugin homepage, the author name, and the plugin description.

Agree. We should strike the right balance. See my concern above.

Nevertheless, it's important to note that this section only appears when there are plugins that are causing validation errors. Otherwise, the section is not shown at all. So in an ideal world, the appearance of this section should an be “exception”.

I agree with the goal. We may convey the same information more concisely.

…n-suppression * 'develop' of github.com:ampproject/amp-wp: Fix PHP 5.6 back-compat code when ReflectionParameter has no class Fix Generic.Formatting.MultipleStatementAlignment Fix compat with PHP 5.6 where uniform variable syntax is not available Fix version command in lib/optimizer Use latest commit of PHPStan for now in lib/optimizer Use latest commit of PHPStan for now in lib/common Fix remaining PHPStan issues in main plugin Update Composer lock file Use latest commit of PHPStan for now Add PHPStan version output for CI debugging Remove exception for bug that was fixed upstream Remove autoload_files option

westonruter · 2020-06-18T06:49:53Z

I'm working on adding tests for the new functionality.

…n-suppression * 'develop' of github.com:ampproject/amp-wp: Update dependency moment to v2.27.0

…prior to WP 5.3

westonruter · 2020-06-19T07:04:44Z

includes/options/class-amp-options-manager.php

+		 * @param array $options     Existing options with already-sanitized values for updating.
+		 * @param array $new_options Unsanitized options being submitted for updating.
+		 */
+		$options = apply_filters( 'amp_options_updating', $options, $new_options );


@schlessera I've added this to allow the option updating logic to be removed from the AMP_Options_Manager and over to the respective services.

I thought about turning the individual options into objects to inject as dependencies already. But the filter will do the trick for now.

BTW, this is what I referred to when I meant that we might need to add hooks so we can reverse the flow of the logic. So we seem to be on the same page wrt this.

schlessera

Looks good, only 2 nitpicks!

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

src/PluginRegistry.php

Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>

googlebot added the cla: yes Signed the Google CLA label May 5, 2020

westonruter force-pushed the add/plugin-suppression branch 4 times, most recently from 3452b99 to b3c5a23 Compare May 8, 2020 06:55

amedina requested review from amedina, pierlon and schlessera May 8, 2020 15:24

westonruter force-pushed the add/plugin-suppression branch from b3c5a23 to e884822 Compare May 9, 2020 21:00

westonruter commented May 11, 2020

View reviewed changes

src/PluginSuppression.php Outdated Show resolved Hide resolved

amedina reviewed May 12, 2020

View reviewed changes

includes/options/class-amp-options-menu.php Outdated Show resolved Hide resolved

westonruter mentioned this pull request May 12, 2020

Highlight themes and plugins that are AMP-compatible inside WordPress #2313

Closed

westonruter marked this pull request as ready for review May 13, 2020 06:11

pierlon suggested changes May 15, 2020

View reviewed changes

Base automatically changed from add/4876-dependency-injector to develop June 17, 2020 19:38

westonruter added 2 commits June 17, 2020 14:07

Fix phpcs issues

c6f08c1

westonruter added this to the v1.6 milestone Jun 17, 2020

westonruter added 6 commits June 17, 2020 14:33

Finish refactoring factoring into service, fixing PHPStan analysis

879db79

Use bold for unreviewed validation errors, improve CSS formatting

b0044dc

Improve sort order of validation errors

8edb2a3

Add tests for validating suppressed plugins

4aac2f7

Remove redundant plugins cache

d46f657

Add PluginRegistryTest and improve get_plugin_from_slug

de5db1a

westonruter and others added 4 commits June 18, 2020 00:42

Obtain plugin dir from plugin_registry rather than WP_PLUGIN_DIR

0ae2820

Configure ServiceBasedPlugin for plugin use instead of theme use

c8a0ef0

Add majority of tests for PluginSuppression; account for the_widget()

837f4c9

Account for block editor not being present

90419cf

westonruter force-pushed the add/plugin-suppression branch from 33ad429 to 90419cf Compare June 19, 2020 05:17

westonruter added 5 commits June 18, 2020 22:23

Merge branch 'develop' of github.com:ampproject/amp-wp into add/plugi…

6d646c0

…n-suppression * 'develop' of github.com:ampproject/amp-wp: Update dependency moment to v2.27.0

Account for widget_display_callback filter not being in the_widget() …

851bb9a

…prior to WP 5.3

Add remaining tests for PluginSuppression

e743508

Update Suppressed Plugins description

2824b3b

Centralize option sanitization logic into Service

2a42b2f

westonruter commented Jun 19, 2020

View reviewed changes

schlessera requested changes Jun 19, 2020

View reviewed changes

includes/validation/class-amp-validation-manager.php Outdated Show resolved Hide resolved

src/PluginRegistry.php Outdated Show resolved Hide resolved

Improve variable name and comment

40913f2

Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>

schlessera approved these changes Jun 19, 2020

View reviewed changes

westonruter merged commit 0212bb2 into develop Jun 19, 2020

westonruter deleted the add/plugin-suppression branch June 19, 2020 17:58

westonruter mentioned this pull request Jul 1, 2020

Make the settings screen more visually appealing and reuse React components #4704

Closed

westonruter mentioned this pull request Jul 9, 2020

Suppress plugins early when making a Reader theme request #5009

Merged

3 tasks

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 17, 2020

	if ( preg_match( ':' . preg_quote( trailingslashit( wp_normalize_path( WP_PLUGIN_DIR ) ), ':' ) . $slug_pattern . '(/(?P<file>.*$))?:s', $file, $matches ) ) {
	$source['type'] = 'plugin';
	$source['name'] = $matches['slug'];
	$source['file'] = isset( $matches['file'] ) ? $matches['file'] : $matches['slug'];

Conversation

westonruter commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

westonruter commented May 8, 2020

Uh oh!

Uh oh!

Uh oh!

westonruter commented May 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierlon commented May 15, 2020

Uh oh!

westonruter commented May 27, 2020

Uh oh!

amedina commented May 27, 2020

Uh oh!

westonruter commented May 27, 2020

Uh oh!

pierlon commented May 28, 2020

Uh oh!

schlessera commented May 28, 2020

Uh oh!

westonruter commented May 31, 2020

Uh oh!

westonruter commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amedina commented Jun 1, 2020

Uh oh!

amedina commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amedina commented Jun 1, 2020

Uh oh!

westonruter commented Jun 1, 2020

Uh oh!

westonruter commented Jun 1, 2020

Uh oh!

amedina commented Jun 1, 2020

Uh oh!

westonruter commented Jun 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schlessera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

westonruter commented May 5, 2020 •

edited

Loading

westonruter commented May 31, 2020 •

edited

Loading

amedina commented Jun 1, 2020 •

edited

Loading