Create background events for URL validation#5515
Conversation
|
|
|
Plugin builds for f64e4ab are ready 🛎️!
|
…ation-cron * origin/develop: Run composer update Ensure alphabetical order for patches in composer.json Reorder composer.json fields Remove obsolete amp_is_canonical() examples Add more markdown formatting and restore linebreak Wrap HTML tag in backticks Add variable to docs for `amp_dev_mode_enabled` filter Fix parsing of docblocks that contain code snippets
westonruter
left a comment
There was a problem hiding this comment.
@johnwatkins0 I tried turning off DevTools to see the SingleScheduledBackgroundTask in action. However, I found that SavePostValidationEvent::should_schedule_event() would never get called. The reason appears to be the logic in is_needed:
public static function is_needed() {
return is_admin() || wp_doing_cron();
}When making an edit to a post in the block editor, neither is_admin() nor wp_doing_cron() are true for the REST API request. This means the save_post action is never added.
Should the CronBasedBackgroundTask abstract class not be made Conditional?
That's the simple solution. Updated in a53afa5 |
| /** | ||
| * Callback for the cron action. | ||
| * | ||
| * @param mixed[] ...$args Unused callback arguments. | ||
| */ | ||
| public function process( ...$args ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable | ||
| $urls = $this->scannable_url_provider->get_urls(); | ||
| $sleep_time = $this->get_sleep_time(); | ||
|
|
||
| foreach ( $urls as $url ) { | ||
| $this->url_validation_provider->get_url_validation( $url['url'], $url['type'], true ); | ||
| if ( $sleep_time ) { | ||
| sleep( $sleep_time ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We can address this in a subsequent PR, but I have an idea for how this could be modified to not make use of sleep(). Instead of there being one single event that scans all of the URLs, we can instead register a separate event for each for each URL, and offset the time() for each event by 10 minute intervals (for example). In this way only one URL would be validated during any given cron request, and there would be no need for sleep().
|
I was testing After deactivating, they remained. I found there were a few problems:
I've addressed these points in f424735. Now when deactivating the plugin, no AMP hooks remain: |
westonruter
left a comment
There was a problem hiding this comment.
Thanks for the mammoth effort here. This is going to go a long way to help site owners monitor their sites for AMP validation issues.
Summary
Fixes #1756. Some related discussion also took place in #5228
Main changes
This PR does the following:
hourlydaily cron task that validates a set of URLs. The URLs are the default set returned from theScannableURLProviderclass. This set will always include most recent posts for all AMP-enabled post types plus a standard set of others such as the homepage and a search result.save_postto validate the saved post's URL when the user has dev tools off. When the user has dev tools on, the existing behavior (URL is validated synchronously on save) is unchanged.Additional changes
BackgroundTaskDeactivator
I also took the suggestion from this to-do comment in the
CronBasedBackgroundTaskclass:I created the new class,
BackgroundTaskDeactivatorwhich is injected into the background task classes and used to queue up registered background tasks to be cleared when the plugin is deactivated.CronBasedBackgroundTask refactor
CronBasedBackgroundTaskwas previously being extended for a couple of different recurring cron tasks. I have createdRecurringBackgroundTaskalong withSingleScheduledBackgroundTaskto account for the two different scheduling approaches we need for background. These new classes are both abstract, inheriting fromCronBasedBackgroundTask, andCronBasedBackroundTaskis no longer directly extended by any non-abstract class.Remove locking
I removed the locking functions from
URLValidationProvider. I introduced these in another recent PR, but we subsequently determined they're not needed for now at least.Checklist