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

Refactor cron event to validate individual URLs #6520

Merged
merged 49 commits into from
Oct 21, 2021
Merged
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
6feb425
Update URL validation cron jobs
dhaval-parekh Aug 9, 2021
2ce519a
Add unit test cases
dhaval-parekh Aug 9, 2021
0e8fa73
Validate single URL on URL validation cron, Update test cases
dhaval-parekh Oct 1, 2021
a7aeb61
Hard core the site option key that need to delete
dhaval-parekh Oct 7, 2021
d7f1659
Merge branch 'develop' into dev-tool/5750-refactor-url-validation-cron
dhaval-parekh Oct 7, 2021
9dcb139
Update comments
dhaval-parekh Oct 7, 2021
dc72b8a
Remove unused references
dhaval-parekh Oct 7, 2021
a67cadd
Merge branch 'develop' of github.com:ampproject/amp-wp into dev-tool/…
westonruter Oct 12, 2021
4f1630e
Remove return of empty array
westonruter Oct 12, 2021
c085388
Replace extraneous URLValidationProvider with direct AMP_Validation_M…
westonruter Oct 12, 2021
2a05e8f
Update tests to use DependencyInjectedTestCase
westonruter Oct 12, 2021
f41681f
Simplify URL validation scheduling by putting the queue in URLValidat…
westonruter Oct 12, 2021
422d0f2
Re-schedule with new recurrence when schedule changes
westonruter Oct 13, 2021
bf25b0c
Unschedule event with wrong recurrence instead of rescheduling it
westonruter Oct 13, 2021
e7f51ea
Fix unit test case
dhaval-parekh Oct 13, 2021
5f56cc1
Revert "Fix unit test case"
dhaval-parekh Oct 13, 2021
3f803b0
Ensure that AMP-enabled posts are scanned
westonruter Oct 14, 2021
fc2c727
Remove obsolete force argument for amp validation CLI command
westonruter Oct 14, 2021
72ffb2d
Provide wp_get_scheduled_event() implementation for WP<5.1 and add tests
westonruter Oct 14, 2021
b5afb54
Try not using past time for scheduling event
westonruter Oct 14, 2021
7371fd4
Use daily schedule to test since available in older WP versions
westonruter Oct 14, 2021
dab728c
Merge branch 'develop' of github.com:ampproject/amp-wp into dev-tool/…
westonruter Oct 14, 2021
afcbcce
Fix covers phpdoc tags
westonruter Oct 14, 2021
ca7b70d
Use assertLessThanOrEqual rather than assertGreaterThanOrEqual
westonruter Oct 14, 2021
d1ccdaf
Fix covers phpdoc tag for wrap_block_callbacks
westonruter Oct 14, 2021
2e2f363
Account for wp_schedule_event() returning bool as of WP 5.1
westonruter Oct 14, 2021
4144830
Remove obsolete arg from URLScanningContext constructor
westonruter Oct 14, 2021
79d678f
Remove unused amp_url_validation_limit_per_type filter
westonruter Oct 14, 2021
da77a0d
Allow options to be overridden when calling get_supportable_templates
westonruter Oct 15, 2021
5bbadea
Refactor ScannableURLProvider and URLValidationProvider into services
westonruter Oct 15, 2021
ab2bff1
Include more AMP settings in validated environment
westonruter Oct 15, 2021
f0c7b3b
Fix tests after 5bbadea
westonruter Oct 15, 2021
55c0ea7
Clear out cron validation queue whenever validated environment changes
westonruter Oct 15, 2021
042a74c
Remove obsolete test for amp_should_use_new_onboarding
westonruter Oct 15, 2021
4e0dda5
Fix test assertions by using polyfilled TestCase
westonruter Oct 15, 2021
42a2c35
Remove unused DEFAULT_INTERVAL_WEEKLY and fix test_process_and_dequeu…
westonruter Oct 15, 2021
acf93dd
Use get_year_link() to obtain the year archive link and ensure it has…
westonruter Oct 15, 2021
2e94ba4
Ensure author links are not 404
westonruter Oct 15, 2021
6d25e3c
Improve test coverage
westonruter Oct 15, 2021
1b1b7b1
Fix test for get_author_page_urls
westonruter Oct 15, 2021
27b18de
Fix get_date_page test in WP<=5.1
westonruter Oct 15, 2021
5820ce8
Merge branch 'develop' of github.com:ampproject/amp-wp into dev-tool/…
westonruter Oct 18, 2021
5f5fe35
Merge branch 'develop' of github.com:ampproject/amp-wp into dev-tool/…
westonruter Oct 20, 2021
fce116d
Create a post before going to the onboarding wizard so there is somet…
westonruter Oct 20, 2021
8f2985a
Create test post and page using REST API
delawski Oct 21, 2021
43899d3
Do not show site preview if there are no preview URLs
delawski Oct 21, 2021
331955f
Clean up site preview E2E test
delawski Oct 21, 2021
8376583
Fix AMP Settings Review panel E2E test
delawski Oct 21, 2021
284e489
Harden check for post being set
westonruter Oct 21, 2021
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
2 changes: 2 additions & 0 deletions .phpstorm.meta.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@
'url_validation_cron' => \AmpProject\AmpWP\Validation\URLValidationCron::class,
'url_validation_rest_controller' => \AmpProject\AmpWP\Validation\URLValidationRESTController::class,
'validated_url_stylesheet_gc' => \AmpProject\AmpWP\BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class,
'validation.scannable_url_provider' => \AmpProject\AmpWP\Validation\ScannableURLProvider::class,
'validation.url_validation_provider' => \AmpProject\AmpWP\Validation\URLValidationProvider::class,
] )
);

Expand Down
77 changes: 52 additions & 25 deletions assets/src/onboarding-wizard/pages/done/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,14 @@ export function Done() {
const { didSaveDeveloperToolsOption, saveDeveloperToolsOption, savingDeveloperToolsOption } = useContext( User );
const { canGoForward, setCanGoForward } = useContext( Navigation );
const { downloadedTheme, downloadingTheme, downloadingThemeError } = useContext( ReaderThemes );
const { previewLinks, setActivePreviewLink, previewUrl, isPreviewingAMP, toggleIsPreviewingAMP } = usePreview();
const {
hasPreview,
isPreviewingAMP,
previewLinks,
previewUrl,
setActivePreviewLink,
toggleIsPreviewingAMP,
} = usePreview();

/**
* Allow the finish button to be enabled.
Expand Down Expand Up @@ -104,55 +111,75 @@ export function Done() {
{ __( 'Your site is ready to bring great experiences to your users!', 'amp' ) }
</p>
{ STANDARD === themeSupport && (
<p>
{ __( 'In Standard mode there is a single AMP version of your site. Browse your site here to ensure it meets your expectations.', 'amp' ) }
</p>
<>
<p>
{ __( 'In Standard mode there is a single AMP version of your site.', 'amp' ) }
</p>
{ hasPreview && (
<p>
{ __( 'Browse your site here to ensure it meets your expectations.', 'amp' ) }
</p>
) }
</>
) }
{ TRANSITIONAL === themeSupport && (
<>
<p>
{ __( 'In Transitional mode AMP and non-AMP versions of your site are served using your currently active theme.', 'amp' ) }
</p>
<p>
{ __( 'Browse your site here to ensure it meets your expectations, and toggle the AMP setting to compare both versions.', 'amp' ) }
</p>
{ hasPreview && (
<p>
{ __( 'Browse your site here to ensure it meets your expectations, and toggle the AMP setting to compare both versions.', 'amp' ) }
</p>
) }
</>
) }
{ READER === themeSupport && (
<>
<p>
{ __( 'In Reader mode AMP is served using your selected Reader theme, and pages for your non-AMP site are served using your primary theme. Browse your site here to ensure it meets your expectations, and toggle the AMP setting to compare both versions.', 'amp' ) }
{ __( 'In Reader mode AMP is served using your selected Reader theme, and pages for your non-AMP site are served using your primary theme.', 'amp' ) }
</p>
{ hasPreview && (
<p>
{ __( 'Browse your site here to ensure it meets your expectations, and toggle the AMP setting to compare both versions.', 'amp' ) }
</p>
) }
<p>
{ __( 'As a last step, use the Customizer to tailor the Reader theme as needed.', 'amp' ) }
</p>
</>
) }
<div className="done__links-container">
<NavMenu
links={ previewLinks }
onClick={ ( e, link ) => {
e.preventDefault();
setActivePreviewLink( link );
} }
/>
</div>
{ hasPreview && (
<div className="done__links-container">
<NavMenu
links={ previewLinks }
onClick={ ( e, link ) => {
e.preventDefault();
setActivePreviewLink( link );
} }
/>
</div>
) }
</div>
<div className="done__preview-container">
{ READER === themeSupport && downloadingThemeError && (
<AMPNotice size={ NOTICE_SIZE_LARGE } type={ NOTICE_TYPE_INFO }>
{ __( 'There was an error downloading your Reader theme. As a result, your site is currently using the legacy reader theme. Please install your chosen theme manually.', 'amp' ) }
</AMPNotice>
) }
{ STANDARD !== themeSupport && (
<AMPSettingToggle
text={ __( 'AMP', 'amp' ) }
checked={ isPreviewingAMP }
onChange={ toggleIsPreviewingAMP }
compact={ true }
/>
{ hasPreview && (
<>
{ STANDARD !== themeSupport && (
<AMPSettingToggle
text={ __( 'AMP', 'amp' ) }
checked={ isPreviewingAMP }
onChange={ toggleIsPreviewingAMP }
compact={ true }
/>
) }
<Preview url={ previewUrl } />
</>
) }
<Preview url={ previewUrl } />
</div>
<div className="done__content done__content--secondary">
<h2 className="done__icon-title">
Expand Down
9 changes: 6 additions & 3 deletions assets/src/onboarding-wizard/pages/done/use-preview.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ import { Options } from '../../../components/options-context-provider';
import { STANDARD } from '../../../common/constants';

export function usePreview() {
const hasPreview = PREVIEW_URLS.length > 0;

const { editedOptions: { theme_support: themeSupport } } = useContext( Options );
const [ isPreviewingAMP, setIsPreviewingAMP ] = useState( themeSupport !== STANDARD );
const [ previewedPageType, setPreviewedPageType ] = useState( PREVIEW_URLS[ 0 ].type );
const [ previewedPageType, setPreviewedPageType ] = useState( hasPreview ? PREVIEW_URLS[ 0 ].type : null );

const toggleIsPreviewingAMP = () => setIsPreviewingAMP( ( mode ) => ! mode );
const setActivePreviewLink = ( link ) => setPreviewedPageType( link.type );
Expand All @@ -32,10 +34,11 @@ export function usePreview() {
const previewUrl = useMemo( () => previewLinks.find( ( link ) => link.isActive )?.url, [ previewLinks ] );

return {
hasPreview,
isPreviewingAMP,
previewLinks,
setActivePreviewLink,
previewUrl,
isPreviewingAMP,
setActivePreviewLink,
toggleIsPreviewingAMP,
};
}
2 changes: 1 addition & 1 deletion assets/src/settings-page/site-review.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export function SiteReview() {
return null;
}

const previewPermalink = STANDARD === themeSupport ? HOME_URL : pairedUrlExamples[ pairedUrlStructure ][ 0 ];
const previewPermalink = STANDARD === themeSupport ? HOME_URL : pairedUrlExamples?.[ pairedUrlStructure ]?.[ 0 ] ?? HOME_URL;
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 in another PR, but even when in Standard mode we shouldn't assume that the HOME_URL has AMP enabled for it. The user could turn off the homepage template.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, the way we get the preview permalink changes in #6610 quite a bit. I guess this will likely need to be changed in context of your comment.

const previewPermalink = useMemo( () => {
const pageTypes = themeSupport === READER ? [ 'post', 'page' ] : [ 'home' ];
return scannableUrls.find( ( { type } ) => pageTypes.includes( type ) )?.[ urlType ] || homeUrl;
}, [ homeUrl, scannableUrls, themeSupport, urlType ] );

Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact, since scannableUrls that we're now getting are context-aware, I think we won't have to guess which page to use for a preview. We could just grab the first one from the list, correct?

Copy link
Member

Choose a reason for hiding this comment

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

That's right!


return (
<AMPDrawer
Expand Down
3 changes: 3 additions & 0 deletions includes/admin/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ function amp_init_customizer() {
/**
* Get permalink for the first AMP-eligible post.
*
* @todo Eliminate this in favor of ScannableURLProvider::get_posts_by_type().
* @see \AmpProject\AmpWP\Validation\ScannableURLProvider::get_posts_by_type()
*
* @internal
* @return string|null URL on success, null if none found.
*/
Expand Down
12 changes: 9 additions & 3 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -686,9 +686,15 @@ public static function get_template_availability( $query = null ) {
/**
* Get the templates which can be supported.
*
* @param array $options Optional AMP options to override what has been saved.
* @return array Supportable templates.
*/
public static function get_supportable_templates() {
public static function get_supportable_templates( $options = [] ) {
$options = array_merge(
AMP_Options_Manager::get_options(),
$options
);

$templates = [
'is_singular' => [
'label' => __( 'Singular', 'amp' ),
Expand Down Expand Up @@ -798,8 +804,8 @@ public static function get_supportable_templates() {
*/
$templates = apply_filters( 'amp_supportable_templates', $templates );

$supported_templates = AMP_Options_Manager::get_option( Option::SUPPORTED_TEMPLATES );
$are_all_supported = AMP_Options_Manager::get_option( Option::ALL_TEMPLATES_SUPPORTED );
$supported_templates = $options[ Option::SUPPORTED_TEMPLATES ];
$are_all_supported = $options[ Option::ALL_TEMPLATES_SUPPORTED ];

$did_filter_supply_supported = false;
$did_filter_supply_immutable = false;
Expand Down
1 change: 1 addition & 0 deletions includes/uninstall-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function delete_options() {
delete_option( 'amp-options' );
delete_option( 'amp_css_transient_monitor_time_series' );
delete_option( 'amp_customize_setting_modified_timestamps' );
delete_option( 'amp_url_validation_queue' ); // See Validation\URLValidationCron::OPTION_KEY.

$theme_mod_name = 'amp_customize_setting_modified_timestamps';
remove_theme_mod( $theme_mod_name );
Expand Down
22 changes: 18 additions & 4 deletions includes/validation/class-amp-validated-url-post-type.php
Original file line number Diff line number Diff line change
Expand Up @@ -1084,9 +1084,16 @@ public static function get_validated_environment() {
return [
'theme' => $theme,
'plugins' => wp_list_pluck( $plugin_registry->get_plugins( true, false ), 'Version' ), // @todo What about multiple plugins being in the same directory?
'options' => [
Option::THEME_SUPPORT => AMP_Options_Manager::get_option( Option::THEME_SUPPORT ),
],
'options' => wp_array_slice_assoc(
AMP_Options_Manager::get_options(),
[
Option::ALL_TEMPLATES_SUPPORTED,
Option::READER_THEME,
Option::SUPPORTED_POST_TYPES,
Option::SUPPORTED_TEMPLATES,
Option::THEME_SUPPORT,
]
),
];
}

Expand Down Expand Up @@ -1149,7 +1156,14 @@ public static function get_post_staleness( $post ) {
} else {
$old_options = [];
}
$option_differences = array_diff_assoc( $old_options, $new_validated_environment['options'] );
$option_differences = [];
foreach ( $new_validated_environment['options'] as $option => $value ) {
if ( ! isset( $old_options[ $option ] ) ) {
$option_differences[ $option ] = null;
} elseif ( $old_options[ $option ] !== $value ) {
$option_differences[ $option ] = $old_options[ $option ];
}
}
if ( ! empty( $option_differences ) ) {
$staleness['options'] = $option_differences;
}
Expand Down
4 changes: 4 additions & 0 deletions src/AmpWpPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
use AmpProject\AmpWP\Support\SupportCliCommand;
use AmpProject\AmpWP\Support\SupportRESTController;
use AmpProject\AmpWP\Validation\SavePostValidationEvent;
use AmpProject\AmpWP\Validation\ScannableURLProvider;
use AmpProject\AmpWP\Validation\URLValidationCron;
use AmpProject\AmpWP\BackgroundTask\BackgroundTaskDeactivator;
use AmpProject\AmpWP\Validation\URLValidationProvider;
use AmpProject\Optimizer;

use AmpProject\RemoteGetRequest;
Expand Down Expand Up @@ -121,6 +123,8 @@ final class AmpWpPlugin extends ServiceBasedPlugin {
'url_validation_cron' => URLValidationCron::class,
'url_validation_rest_controller' => Validation\URLValidationRESTController::class,
'validated_url_stylesheet_gc' => BackgroundTask\ValidatedUrlStylesheetDataGarbageCollection::class,
'validation.scannable_url_provider' => ScannableURLProvider::class,
'validation.url_validation_provider' => URLValidationProvider::class,
];

/**
Expand Down
81 changes: 77 additions & 4 deletions src/BackgroundTask/RecurringBackgroundTask.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,86 @@ final public function schedule_event( ...$args ) {
}

$event_name = $this->get_event_name();
$timestamp = wp_next_scheduled( $event_name, $args );
$recurrence = $this->get_interval();

if ( $timestamp ) {
return;
$scheduled_event = $this->get_scheduled_event( $event_name, $args );

// Unschedule any existing event which had a differing recurrence.
if ( $scheduled_event && $scheduled_event->schedule !== $recurrence ) {
wp_unschedule_event( $scheduled_event->timestamp, $event_name, $args );
$scheduled_event = null;
}

if ( ! $scheduled_event ) {
wp_schedule_event( time(), $recurrence, $event_name, $args );
}
}

/**
* Retrieve a scheduled event.
*
* This uses a copied implementation from WordPress core if `wp_get_scheduled_event()` does not exist, as it was
* introduced in WordPress 5.1.
*
* @link https://github.com/WordPress/wordpress-develop/blob/ba943e113d3b31b121f7/src/wp-includes/cron.php#L753-L793
* @see \wp_get_scheduled_event()
* @codeCoverageIgnore
*
* @param string $hook Action hook of the event.
* @param array $args Optional. Array containing each separate argument to pass to the hook's callback function.
* Although not passed to a callback, these arguments are used to uniquely identify the
* event, so they should be the same as those used when originally scheduling the event.
* Default empty array.
* @param int|null $timestamp Optional. Unix timestamp (UTC) of the event. If not specified, the next scheduled event
* is returned. Default null.
* @return object|false The event object. False if the event does not exist.
*/
final public function get_scheduled_event( $hook, $args = [], $timestamp = null ) {
if ( function_exists( 'wp_get_scheduled_event' ) ) {
return wp_get_scheduled_event( $hook, $args, $timestamp );
}

if ( null !== $timestamp && ! is_numeric( $timestamp ) ) {
return false;
}

$crons = _get_cron_array();
if ( empty( $crons ) ) {
return false;
}

$key = md5( serialize( $args ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize -- This is copied from WP core.

if ( ! $timestamp ) {
// Get next event.
$next = false;
foreach ( $crons as $timestamp => $cron ) {
if ( isset( $cron[ $hook ][ $key ] ) ) {
$next = $timestamp;
break;
}
}
if ( ! $next ) {
return false;
}

$timestamp = $next;
} elseif ( ! isset( $crons[ $timestamp ][ $hook ][ $key ] ) ) {
return false;
}

$event = (object) [
'hook' => $hook,
'timestamp' => $timestamp,
'schedule' => $crons[ $timestamp ][ $hook ][ $key ]['schedule'],
'args' => $args,
];

if ( isset( $crons[ $timestamp ][ $hook ][ $key ]['interval'] ) ) {
$event->interval = $crons[ $timestamp ][ $hook ][ $key ]['interval'];
}

wp_schedule_event( time(), $this->get_interval(), $event_name, $args );
return $event;
}

/**
Expand Down
Loading