Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create background events for URL validation #5515

Merged
merged 99 commits into from
Dec 30, 2020
Merged
Show file tree
Hide file tree
Changes from 70 commits
Commits
Show all changes
99 commits
Select commit Hold shift + click to select a range
a1b61ff
Create single scheduled background task with save_post single event
johnwatkins0 Oct 14, 2020
345b324
Additional work on SavePostValidationEvent and its parent class
johnwatkins0 Oct 16, 2020
bc4e116
Include cli utils in autoload-dev
johnwatkins0 Oct 19, 2020
4aa2a3d
Merge branch 'feature/refactor-url-validation' into feature/1756-vali…
johnwatkins0 Oct 21, 2020
ef5534e
Remove cli utils from autoload-dev
johnwatkins0 Oct 21, 2020
4f9b4b9
Increase unit test coverage for new classes
johnwatkins0 Oct 21, 2020
2c2dce5
Undo changes to bootstrap file
johnwatkins0 Oct 22, 2020
348b641
Add newline to end of file
johnwatkins0 Oct 22, 2020
f0aa4b8
Fix tests for MonitorCssTransientCaching
johnwatkins0 Oct 22, 2020
05a13f1
Fix tests for ValidatedUrlStylesheetDataGarbageCollection
johnwatkins0 Oct 22, 2020
57fe9fd
Clean up SingleScheduledBackgroundTask
johnwatkins0 Oct 22, 2020
133197a
SavePostValidationEvent cleanup
johnwatkins0 Oct 22, 2020
3097d88
Clean up and simplify tests
johnwatkins0 Oct 22, 2020
c53b089
Remove unused class property
johnwatkins0 Oct 22, 2020
54e21ff
PHPCS and other cleanup
johnwatkins0 Oct 22, 2020
8efbe1c
Increase test coverage
johnwatkins0 Oct 22, 2020
317589e
Merge branch 'feature/refactor-url-validation' into feature/1756-vali…
johnwatkins0 Nov 11, 2020
8084ca7
PHPStan fixes
johnwatkins0 Nov 11, 2020
241789f
Pass force validation flags in cron callbacks
johnwatkins0 Nov 11, 2020
8ef28ce
Fix a since version
johnwatkins0 Nov 11, 2020
1f2ed34
Merge remote-tracking branch 'origin/develop' into feature/1756-valid…
johnwatkins0 Nov 23, 2020
cda3ee2
Remove use of obsolete class constant
johnwatkins0 Nov 23, 2020
33a680c
Integrate updated ValidationRequestMocking (converted to trait)
johnwatkins0 Nov 23, 2020
cd4dd77
Comment updates and minor refactors
johnwatkins0 Nov 23, 2020
2d2a0a6
Remove URL validation locking
johnwatkins0 Nov 25, 2020
3f2d79f
Simplify plugin basename retrieval
johnwatkins0 Nov 25, 2020
7f6f775
Remove unused variable
johnwatkins0 Nov 25, 2020
9345651
Merge branch 'feature/1756-validation-cron' of https://github.com/amp…
johnwatkins0 Nov 25, 2020
28581d7
Add namespace to ValidatedUrlStylesheetDataGarbageCollectionTest.php
johnwatkins0 Nov 25, 2020
0a655f6
Typo fix
johnwatkins0 Nov 25, 2020
1562990
Add leading slash to coversDefaultClass comment
johnwatkins0 Nov 25, 2020
3d07f78
Add namespacing to MonitorCssTransientCachingTest.php
johnwatkins0 Nov 25, 2020
a9e5eac
Merge branch 'feature/1756-validation-cron' of https://github.com/amp…
johnwatkins0 Nov 25, 2020
b2bc91e
Increase test coverage
johnwatkins0 Nov 25, 2020
25ce26c
Harden post_id check
johnwatkins0 Dec 10, 2020
b08f164
Add comment to potentially misleading number arg in get_sites
johnwatkins0 Dec 10, 2020
5334bdf
Move background task deactivate add_event call to register methods
johnwatkins0 Dec 10, 2020
8fefced
Add additional should_schedule_event checks
johnwatkins0 Dec 10, 2020
16e3ee5
Use dependency injector to create instances
johnwatkins0 Dec 10, 2020
265abd6
Add missing doc comment for parameter
johnwatkins0 Dec 10, 2020
6e10b5b
Fix failing get_validated_urls test
johnwatkins0 Dec 10, 2020
e2fe6cd
Add injector delegation
johnwatkins0 Dec 10, 2020
69eddd7
Improve number validation
johnwatkins0 Dec 10, 2020
be7ca6e
PHPStan param fix
johnwatkins0 Dec 10, 2020
418fe96
Merge remote-tracking branch 'origin/develop' into feature/1756-valid…
johnwatkins0 Dec 10, 2020
a42e721
Make SavePostValidationEvent inherit from CronBasedBackgroundTask
johnwatkins0 Dec 10, 2020
94c9959
Create RecurringBackgroundTask
johnwatkins0 Dec 10, 2020
a2f95e1
Use get_timestamp instead of get_interval in SingleScheduledBackgroun…
johnwatkins0 Dec 10, 2020
43e4ffb
Add tests for schedule_event
johnwatkins0 Dec 10, 2020
38b05d6
Increase coverage for single scheduled event
johnwatkins0 Dec 10, 2020
2c7e338
Fix failing schedule_event test
johnwatkins0 Dec 10, 2020
0817cbe
Re-enable tests
johnwatkins0 Dec 10, 2020
313a13f
Update test to use a different filter; other was since 5.2
johnwatkins0 Dec 10, 2020
c31474c
Make anonymous functions static
johnwatkins0 Dec 10, 2020
1e14a97
Add more SavePostValidationEvent tests
johnwatkins0 Dec 11, 2020
5c01177
Increase test coverage
johnwatkins0 Dec 11, 2020
6d8a5c0
Fix mistaken assertTrue instead of assertFalse
johnwatkins0 Dec 11, 2020
6558fe6
Add additional tests for get_url_validation_number_per_type
johnwatkins0 Dec 11, 2020
f4754e1
Merge remote-tracking branch 'origin/develop' into feature/1756-valid…
johnwatkins0 Dec 18, 2020
e2588ec
Merge remote-tracking branch 'origin/develop' into feature/1756-valid…
johnwatkins0 Dec 22, 2020
f38297f
Move BackgroundTaskDeactivator to shared instances
johnwatkins0 Dec 22, 2020
6747c04
Add URLScanningContext class
johnwatkins0 Dec 22, 2020
7cdde96
Fix failing URLValidationCronTest tests
johnwatkins0 Dec 22, 2020
53f5c58
Fix typo in type comment
johnwatkins0 Dec 23, 2020
1419ef4
Pass args to wp_schedule_event
johnwatkins0 Dec 23, 2020
a598fce
Merge branch 'feature/1756-validation-cron' of https://github.com/amp…
johnwatkins0 Dec 23, 2020
809e0cc
Minor updates to comments and unnecessary use statements
johnwatkins0 Dec 23, 2020
1a40431
Rename MonitorCssTransientCachingTest
johnwatkins0 Dec 23, 2020
b200e53
Merge branch 'feature/1756-validation-cron' of https://github.com/amp…
johnwatkins0 Dec 23, 2020
9a0c634
Merge remote-tracking branch 'origin/develop' into feature/1756-valid…
johnwatkins0 Dec 23, 2020
599796f
Remove unneeded use statement
johnwatkins0 Dec 29, 2020
eeb2df5
Improve how potentially empty variables are set
johnwatkins0 Dec 29, 2020
daf1c17
Schedule event if user is logged in
johnwatkins0 Dec 29, 2020
7fc9711
Remove unneeded manage_options check
johnwatkins0 Dec 29, 2020
f362d40
Fix reversed logic of dev_tools_user_access check
johnwatkins0 Dec 29, 2020
c6eb855
Remove blank line between assignments
johnwatkins0 Dec 29, 2020
4f316a1
Mark class as internal
johnwatkins0 Dec 29, 2020
641a7a4
Fix type comments
johnwatkins0 Dec 29, 2020
0195b17
Make amp_url_validation_sleep_time have a minimum of 0
johnwatkins0 Dec 29, 2020
54276a1
Merge remote-tracking branch 'origin/develop' into feature/1756-valid…
johnwatkins0 Dec 29, 2020
0528d1a
Fix failing PHPUnit
johnwatkins0 Dec 29, 2020
b7c8c79
Reorder patches in composer.json
johnwatkins0 Dec 29, 2020
4982692
Regenerate composer.lock
johnwatkins0 Dec 29, 2020
fa5bba9
Reorder composer.json fields
johnwatkins0 Dec 29, 2020
895451e
Update param type for phpstan
johnwatkins0 Dec 29, 2020
07cb9f7
Use PHPStan ignore for 'cannot cast array to int'
johnwatkins0 Dec 29, 2020
7f5f4fe
Merge remote-tracking branch 'origin/develop' into feature/1756-valid…
westonruter Dec 29, 2020
8422d62
Add missing $args to wp_next_scheduled() call after 1419ef4
westonruter Dec 29, 2020
50a6bd9
Update service name for SavePostValidationEvent and add to PHPSTORM_META
westonruter Dec 29, 2020
a53afa5
Make CronBasedBackgroundTask non-Conditional
johnwatkins0 Dec 29, 2020
64f88ad
Merge branch 'feature/1756-validation-cron' of https://github.com/amp…
johnwatkins0 Dec 29, 2020
37d5041
Remove obsolete test assertion
johnwatkins0 Dec 29, 2020
6b4a537
Reduce URLValidationCron frequency to daily
westonruter Dec 30, 2020
f424735
Fix ability for BackgroundTaskDeactivator to unschedule hooks
westonruter Dec 30, 2020
2b49211
Make RecurringBackgroundTask::schedule_event() final
westonruter Dec 30, 2020
ef173e2
Guard against empty post ID causing global post to be returned by get…
westonruter Dec 30, 2020
ab4e02c
Mark URLValidationProvider and ScannableURLProvider as internal
westonruter Dec 30, 2020
035e99c
Ensure amp_url_validation_limit_per_type filter returns int
westonruter Dec 30, 2020
f64e4ab
Simplify logic to ensure -1 <= limit_per_type <= PHP_INT_MAX
westonruter Dec 30, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 8 additions & 19 deletions includes/cli/class-amp-cli-validation-command.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use AmpProject\AmpWP\Option;
use AmpProject\AmpWP\Validation\URLValidationProvider;
use AmpProject\AmpWP\Validation\ScannableURLProvider;
use AmpProject\AmpWP\Validation\URLScanningContext;
use WP_CLI\Utils;

/**
Expand Down Expand Up @@ -141,16 +142,7 @@ public function run( $args, $assoc_args ) {
$number_urls_to_crawl
);

$result = $url_validation_provider->with_lock(
function () use ( $urls ) {
$this->validate_urls( $urls );
}
);

if ( is_wp_error( $result ) ) {
WP_CLI::error( 'The site cannot be crawled at this time because validation is running in another process.' );
return;
}
$this->validate_urls( $urls );

$this->wp_cli_progress->finish();

Expand Down Expand Up @@ -248,9 +240,11 @@ private function get_validation_url_provider() {
}

$this->scannable_url_provider = new ScannableURLProvider(
$limit_type_validate_count,
$include_conditionals,
$force_crawl_urls
new URLScanningContext(
Comment on lines 242 to +243
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be addressed later, but shouldn't both ScannableURLProvider and URLScanningContext be instantiated via the injector?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have refrained so far from moving this class into the DI architecture, but it could be done easily.

$limit_type_validate_count,
$include_conditionals,
$force_crawl_urls
)
);

return $this->scannable_url_provider;
Expand Down Expand Up @@ -284,12 +278,7 @@ private function validate_urls( $urls = null ) {
$urls = $scannable_url_provider->get_urls();
}

foreach ( $urls as $index => $url ) {
// Reset lock between every five URLs.
if ( 0 === $index % 5 ) {
$this->url_validation_provider->reset_lock();
}

foreach ( $urls as $url ) {
$validity = $url_validation_provider->get_url_validation( $url['url'], $url['type'], true );

if ( $this->wp_cli_progress ) {
Expand Down
6 changes: 6 additions & 0 deletions src/AmpWpPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@

use AmpProject\AmpWP\Admin;
use AmpProject\AmpWP\BackgroundTask;
use AmpProject\AmpWP\Infrastructure\Injector;
johnwatkins0 marked this conversation as resolved.
Show resolved Hide resolved
use AmpProject\AmpWP\Infrastructure\ServiceBasedPlugin;
use AmpProject\AmpWP\Instrumentation;
use AmpProject\AmpWP\Validation\SavePostValidationEvent;
use AmpProject\AmpWP\Validation\URLValidationCron;

use function is_user_logged_in;

Expand Down Expand Up @@ -80,6 +83,8 @@ final class AmpWpPlugin extends ServiceBasedPlugin {
'server_timing' => Instrumentation\ServerTiming::class,
'site_health_integration' => Admin\SiteHealth::class,
'validated_url_stylesheet_gc' => BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class,
'url_validation_cron' => URLValidationCron::class,
'save_post_validation_single_event' => SavePostValidationEvent::class,
];

/**
Expand Down Expand Up @@ -161,6 +166,7 @@ protected function get_shared_instances() {
DevTools\CallbackReflection::class,
DevTools\FileReflection::class,
ReaderThemeLoader::class,
BackgroundTask\BackgroundTaskDeactivator::class,
];
}

Expand Down
171 changes: 171 additions & 0 deletions src/BackgroundTask/BackgroundTaskDeactivator.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
<?php
/**
* BackgroundTaskDeactivator service for clearing background tasks on plugin deactivation.
*
* @package AmpProject\AmpWP
*/

namespace AmpProject\AmpWP\BackgroundTask;

use AmpProject\AmpWP\Icon;
use AmpProject\AmpWP\Infrastructure\Conditional;
use AmpProject\AmpWP\Infrastructure\Deactivateable;
use AmpProject\AmpWP\Infrastructure\Registerable;
use AmpProject\AmpWP\Infrastructure\Service;

/**
* Clears background tasks when the plugin is deactivated.
*
* @package AmpProject\AmpWP
* @since 2.1
* @internal
*/
final class BackgroundTaskDeactivator implements Service, Conditional, Registerable, Deactivateable {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the BackgroundTaskDeactivator can be added to the list of shared services, as we'd only need a shared single instance across all cron tasks for it to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are AmpWpPlugin::SERVICES not shared instances? I thought they were, but it's possible I have been misunderstanding that. I think there are probably other Service classes I've created on other PRs that only need to be singletons.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they are not shared by default. See comment above.

Copy link
Collaborator

@schlessera schlessera Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, wait, I'm referring to regular dependencies, not services. Let me check the injector again before I continue talking nonsense...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it seems correct, services are not shared by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f38297f


/**
* List of event names to deactivate.
*
* @var string[]
*/
private $events_to_deactivate = [];

/**
* Name of the plugin as WordPress is expecting it.
*
* This should usually have the form "amp/amp.php".
*
* @var string
*/
private $plugin_file;

/**
* Check whether the conditional object is currently needed.
*
* @return bool Whether the conditional object is needed.
*/
public static function is_needed() {
return is_admin() || wp_doing_cron();
}

/**
* Class constructor.
*/
public function __construct() {
$this->plugin_file = plugin_basename( AMP__FILE__ );
}

/**
* Register the service with the system.
*
* @return void
*/
public function register() {
add_action( "network_admin_plugin_action_links_{$this->plugin_file}", [ $this, 'add_warning_sign_to_network_deactivate_action' ], 10, 1 );
add_action( 'plugin_row_meta', [ $this, 'add_warning_to_plugin_meta' ], 10, 2 );
}

/**
* Get warning icon markup.
*
* @return string Warning icon markup.
*/
private function get_warning_icon() {
return sprintf( '<span style="vertical-align: middle">%s</span>', Icon::warning()->to_html() );
}

/**
* Adds an event to the deactivate queue.
*
* @param string $event_name The event name.
*/
public function add_event( $event_name ) {
if ( ! in_array( $event_name, $this->events_to_deactivate, true ) ) {
$this->events_to_deactivate[] = $event_name;
}
}

/**
* Run deactivation logic.
*
* This should be hooked up to the WordPress deactivation hook.
*
* @param bool $network_wide Whether the deactivation was done network-wide.
* @return void
*/
public function deactivate( $network_wide ) {
if ( $network_wide && is_multisite() && ! wp_is_large_network( 'sites' ) ) {
foreach ( get_sites(
[
'fields' => 'ids',
'number' => 0, // Disables pagination to retrieve all sites.
]
) as $blog_id ) {
switch_to_blog( $blog_id );

foreach ( $this->events_to_deactivate as $event_name ) {
wp_clear_scheduled_hook( $event_name );
}

restore_current_blog();
}
} else {
foreach ( $this->events_to_deactivate as $event_name ) {
wp_clear_scheduled_hook( $event_name );
}
}
}

/**
* Add a warning sign to the network deactivate action on the network plugins screen.
*
* @param string[] $actions An array of plugin action links. By default this can include 'activate',
* 'deactivate', and 'delete'.
* @return string[]
*/
public function add_warning_sign_to_network_deactivate_action( $actions ) {
if ( ! wp_is_large_network() ) {
return $actions;
}

if ( ! array_key_exists( 'deactivate', $actions ) ) {
return $actions;
}

wp_enqueue_style( 'amp-icons' );

$warning_icon = $this->get_warning_icon();
if ( false === strpos( $actions['deactivate'], $warning_icon ) ) {
$actions['deactivate'] = preg_replace( '#(?=</a>)#i', ' ' . $warning_icon, $actions['deactivate'] );
}
Comment on lines +124 to +129
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter I noticed you were the original author of this snippet of code.

How about instead of only appending the warning icon to the anchor text, we use a confirmation dialog to verify they want to deactivate? For example:

Suggested change
wp_enqueue_style( 'amp-icons' );
$warning_icon = $this->get_warning_icon();
if ( false === strpos( $actions['deactivate'], $warning_icon ) ) {
$actions['deactivate'] = preg_replace( '#(?=</a>)#i', ' ' . $warning_icon, $actions['deactivate'] );
}
if ( preg_match( '/<a\s+.*?id\s*=\s*"([^"]+)"/', $actions['deactivate'], $matches ) ) {
$anchor_id = $matches[1];
$confirm_text = __( 'Deactivation will leave orphaned scheduled events behind. Do you want to continue?', 'amp' );
$script = "
document.getElementById( '{$anchor_id}' ).addEventListener( 'click', function ( event ) {
if ( ! confirm( '{$confirm_text}' ) ) {
event.preventDefault();
}
} )
";
wp_add_inline_script( 'wp-dom-ready', $script );
}

And here's it in action:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the warning was originally added by @schlessera in eaf8259, although it was further iterated on.

Adding a confirm() would work. The only downside would be if you select multiple plugins and do a bulk deactivation: you'd easily miss that the plugin has special considerations for deactivation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say I'm pretty agnostic on this. We could do both (the icon and the confirm), make no change, or make a new issue for further discussion. Which way are you leaning @westonruter @pierlon ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only downside would be if you select multiple plugins and do a bulk deactivation: you'd easily miss that the plugin has special considerations for deactivation.

@westonruter The confirm() could also be applied to that also, no?

I agree with @johnwatkins0 with regards to moving this discussion to its own issue.


return $actions;
}

/**
* Add a warning to the plugin meta row on the network plugins screen.
*
* @param string[] $plugin_meta An array of the plugin's metadata, including the version, author, author URI, and
* plugin URI.
* @param string $plugin_file Path to the plugin file relative to the plugins directory.
* @return string[]
*/
public function add_warning_to_plugin_meta( $plugin_meta, $plugin_file ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Same suggestion as above. If you do plan to do something similar to above, this method may not be needed. Instead, you could probably hook into plugin_action_links_{$this->plugin_file}.

if ( ! is_multisite() || ! wp_is_large_network() ) {
return $plugin_meta;
}

if ( $plugin_file !== $this->plugin_file ) {
return $plugin_meta;
}

wp_enqueue_style( 'amp-icons' );

$warning = $this->get_warning_icon() . ' ' . esc_html__( 'Large site detected. Deactivation will leave orphaned scheduled events behind.', 'amp' ) . ' ' . $this->get_warning_icon();

if ( ! in_array( $warning, $plugin_meta, true ) ) {
$plugin_meta[] = $warning;
}

return $plugin_meta;
}
}
Loading