Test AMP compatibility of entire site #1183

kienstra · 2018-05-30T04:18:30Z

Pull Request For #1007

This crawls the site, and test for AMP validity by template/content type.

Validate the homepage, as the existing query won't include it if it's not a static page.
Should this also validate the URLs of categories and tags?
Consider whether reusing AMP_Validation_Manager::validate_queued_posts_on_frontend() is the best way way to validate these URLs. It will probably need to be changed to batch the requests to the front-end, and I'm not sure if that will interfere with the existing functionality.
Consider whether this really needs its own class. At the moment, it's probably not big enough to need it.

Define get_post_permalinks(). This gets permalinks for public posts, other than attachments. Another helper function can call AMP_Validation_Manager::validate_url() for each of these permalinks.

validate_queued_posts_on_frontend() uses IDs, which are stored in $posts_pending_frontend_validation. So simply return IDs from the helper function.

Before, $post_ids wasn't assigned as an array(). This addresses a failed Travis build.

Querying for ID was simply to make testing easier. So remove that, and fall back to the default 'orderby' => 'date'.

westonruter · 2018-05-30T07:13:02Z

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

+		foreach ( $site_post_ids as $id ) {
+			AMP_Validation_Manager::$posts_pending_frontend_validation[ $id ] = true;
+		}
+		return AMP_Validation_Manager::validate_queued_posts_on_frontend();


As you suggested, this validate_queued_posts_on_frontend method is probably not relevant. The main thing will be to:

Obtain the list of URLs on the site, including homepage, posts, pages, categories, archives, etc. A good sitemap XML generator plugin should have the logic you need for this.

If in paired mode, append ?amp to each of the URLs discovered. This will be used instead of amp_get_permalink() because the endpoint will be eliminated when AMP theme support is present. See Discontinue use of amp endpoint in favor of query var when amp theme support is present #1148. Some consideration in paired mode may be needed for whether AMP is available or not for a given post.

For each AMP URL, call \AMP_Validation_Manager::validate_url() and pass the results into \AMP_Invalid_URL_Post_Type::store_validation_errors().

Chunk the results to prevent timing out. This is particularly key in WP-Cron and Ajax. In WP-CLI this isn't relevant, and a progress meter should be shown.

Thanks for your fast reply, @westonruter! These details really help.

This class is not going to use this method anymore: validate_queued_posts_on_frontend(). So this will need the permalinks, not IDs.

For example, http://example.org/?cat=2 This includes categories and tags, and any more that are registered. But it excludes post_format links.

westonruter · 2018-05-30T21:14:54Z

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

+	 * @param int $number_links The maximum amount of links to get (optional).
+	 * @return string[] $links The term links in an array.
+	 */
+	public static function get_taxonomy_links( $number_links = 200 ) {


There should be pagination support here. The WP_Term_Query supports an $offset arg. I suggest that this method only return links for one specific taxonomy. The site crawler will probably need to make multiple calls to such a method to iterate over all of the taxonomy terms in batches. One way to do it would be to get the terms sorted by ID in ascending order. That ensures that when you paginate through the terms you won't miss terms that get added while you're processing through them.

Thanks, good ideas. These commits apply your suggestions, as I understand them. This now has $taxonomy and $offset parameters.

westonruter · 2018-05-30T21:15:43Z

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

+			'public' => true,
+		) );
+
+		// It doesn't seem necessary to get links for post format terms, like asides, galleries, or quotes.


Actually, I think it is necessary. There are archive links for post formats: https://codex.wordpress.org/Function_Reference/get_post_format_link

Thanks, commit db54a removes the exemption of post_format terms.

westonruter · 2018-05-30T21:49:04Z

I think the “Check Site” button would make sense on the “Invalid AMP Pages” screen. Clicking it should show a spinner and some progress indicator probably.

And as noted on #1184 (comment):

I've been considering that we should add a new AMP admin screen to manage settings regarding theme support, including:

Forcibly enable amp theme support for the current theme, even if it lacks add_theme_support('amp'). This will be particularly useful as the plugin will support all the core themes in AMP by default, and it would allow

Decide whether to have paired mode.

Preemptively opt-in to tree shaking, since it is going to be needed in the vast majority of cases anyway. Options here could be “Ask me” (block AMP without accepting), “As needed” (sometimes), and “Always”.

Having a “Check Site Compatibility” button would make a lot of sense on such a Theme Support admin screen. As part of this, we should allow for the amp_validate query parameter to force the plugin to behave as if amp theme support is present. In that way, a site could be checked for AMP compatibility before even enabling amp theme support via an opt-in toggle on that admin screen.

Before, I had unset() these from the array. But as Weston pointed out, post_format terms can have archive links.

Add a $taxonomy parameter, to only get the terms for a given taxonomy.

As Weston mentioned, this helps to get the links in batches. Also, simplify the test.

Props 10up Engineering Best Practices. The prevents the need to use wp_list_pluck() to get the ids.

A similar change to the one Weston proposed for get_taxonomy_links(). Get permalinks by post types, and allow paging through them. This will allow iteration, in a way similar to that in get_taxonomy_links().

@todo

Iterate over all of the terms and posts from the existing helper methods. @todo: consider self::BATCH_SIZE, and whether the validation requests should be throttled, or somehow prevented from timing out WP.

In PHP 5.3, there was: Fatal error: Cannot access self:: when no class scope is active. https://travis-ci.org/Automattic/amp-wp/jobs/386045157. This looks to be because self isn't available inside the function in 5.3. Another option might be to add $self to use(). But this seems simpler, if it works.

This will enable crawling the site in Native AMP. This seems simpler than registering Paired Mode support, and adding template_dir. But it's still an open discussion as to whether it should only be Native AMP. If this already finds theme support, it doesn't change it.

In the validate_url() helper function, find if it's in Paired Mode. If so, set the $url to the amp endpoint.

This mainly calls the existing helper function. But it also passes it $wp_cli_progress. This advances the progress bar, after each post type or taxonomy validation completes.

Success: 196 URLs were crawled, and 196 have AMP validation issue(s). Query for the validation error posts. And output the number found.

This drives to the page: Invalid AMP Pages (URLs) Also, output a message at the beginning: Crawling the entire site to test for AMP validity. This might take a while...

kienstra · 2018-06-01T23:29:55Z

[WIP] WP-CLI Output
When you run wp eval-file bin/validate-site.php

And when it finishes:

westonruter · 2018-06-05T00:02:46Z

That UI for WP-CLI looks perfect 👌

westonruter · 2018-06-05T00:04:39Z

bin/validate-site.php

+} else {
+	echo "Please run this script with WP-CLI via: wp eval-file bin/validate-site.php\n";
+	exit( 1 );
+}


Instead of adding this as a bin script it should get registered as an actual command with WP-CLI. For example, wp amp validate-site. The command needs to be included with the plugin when it is released, and the bin directory is just for development.

westonruter · 2018-06-05T00:10:57Z

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

+			}
+
+			if ( $wp_cli_progress ) {
+				$wp_cli_progress->tick();


This could get de-coupled from WP-CLI by passing a $progress_callback function instead which is called, and then you could call the method like so:

\AMP_Site_Validation::validate_entire_site_urls( function() use ( $wp_cli_progress ) { $wp_cli_progress->tick() } )

westonruter · 2018-06-05T00:11:44Z

bin/validate-site.php

+ */
+function crawl_site() {
+	\WP_CLI::log( 'Crawling the entire site to test for AMP validity. This might take a while...' );
+	$count_post_types_and_taxonomies = count( get_post_types( array( 'public' => true ), 'names' ) ) + count( get_taxonomies( array( 'public' => true ) ) );


This should count the number of posts and terms, not just the number of types.

westonruter · 2018-06-05T00:12:43Z

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

+			$offset     = 0;
+
+			while ( ! empty( $permalinks ) ) {
+				self::validate_urls( $permalinks );


Per above, the tick should be done with each URL that is processed, not after each type is processed.

Great idea. Commit 3d9a01 calls tick() for every URL validated.

kienstra · 2018-06-07T01:18:33Z

Hi @westonruter,
Thanks for reviewing this. You make good points, like registering an actual WP-CLI command.

If it's alright, I'm going to leave this work for the next phase.

3 assertions failed, so fix these both in the tested class and the PHPUnit class.

On Weston's suggestion. Before, running the validation required: wp eval-file bin/validate-site.php.

This has a much better display of the activity. On Weston's suggestion. Create a new method to count all of the URLs to be validated. This should only be used for WP-CLI, as it uses 'posts_per_page' => -1

westonruter · 2018-08-24T06:23:42Z

Also, this doesn't register the command at all in 'Classic' mode, as the class is bootstrapped in AMP_Validation_Manager. Should that change?

@kienstra Also, this is important for AC2 to be satisfied:

AC2: A validation test should be able to evaluate an entire site even if AMP theme support is not declared yet. (i.e. Be able to force the equivalent of AMP theme support when crawling.)

If I'm in Classic mode, currently the result is:

$ wp amp validate-site --limit=2 --force
Crawling the site for AMP validity.
Validating 13 URLs...  100% [==========================================================================] 0:00 / 0:00
Success: 0 crawled URLs have unaccepted issue(s) out of 0 total which AMP validation issue(s); 0 URLs were crawled.

For more details, please see:
https://src.wordpress-develop.test/wp-admin/edit.php?post_type=amp_invalid_url

+--------------------------+-----------+---------------+
| Template or content type | URL Count | Validity Rate |
+--------------------------+-----------+---------------+
+--------------------------+-----------+---------------+

So that needs to be improved so that site validity can be checked for AMP theme support prior to switching it on. To achieve that, I think the following needs to be done:

When invoking the command, check if the theme support is classic, and if so, return an error if --force is not supplied and explain why.
When --force is enabled, the force query param should cause not only AMP to be available for a URL but also for AMP theme support to be enabled.
The command will need to just-in-time call \AMP_Validation_Error_Taxonomy::register() and \AMP_Invalid_URL_Post_Type::register()

kienstra · 2018-08-24T16:46:28Z

Hi @westonruter,
That's a good point, and thanks for the steps to validate in 'Classic' mode. I'll work on that today if that's alright.

On Weston's suggestion, as this would not be able to validate any URL without the --force flag.

As Weston suggested, if in Classic mode and --force is passed, force theme support in the crawled URLs. This is now done in read_theme_support(), thought there might be a better way.

kienstra · 2018-08-24T20:33:40Z

Hi @westonruter, the 2 commits above apply your suggestions for forcing theme support when in Classic mode. I hope they capture what you had in mind.

Here's the error that appears when in 'Classic' mode and --force isn't present:

kienstra · 2018-08-24T20:36:29Z

includes/class-amp-cli.php

+			self::$force_crawl_urls = true;
+		}
+
+		if ( ! current_theme_supports( 'amp' ) ) {


This conditional will capture the Template Mode being Classic, though maybe you had in mind a more narrow check, like:

! current_theme_supports( 'amp' ) && 'disabled' === AMP_Options_Manager::get_option( 'theme_support' )

* Only show admin URL to review results if theme support initially present. * Skip showing table if no results obtained.

…sic mode

… and taxonomy * Hide the admin menu links for invalid URLs and validation errors by default in classic mode. * Invoking `wp amp validate-site` will cause the post type and taxonomy to be populated. * When in classic mode, show links to invalid pages and validation errors with template mode as opposed to in admin menu. * When viewing the admin screens for the post type and taxonomy, show the admin menu links for both.

westonruter · 2018-08-29T00:30:52Z

The last piece here seemed to me to be providing a way for the user to access the validation results for their wp amp validate-site call even while AMP theme support is turned off. This is implemented in edcb23a, where the post type and taxonomy are unconditionally registered but they are only shown in the admin menu if theme support is present. Otherwise, links to the screens are shown if there are invalid URL posts that exist:

hellofromtonya · 2018-08-29T00:37:03Z

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

+		if ( current_theme_supports( 'amp' ) ) {
+			return true;
+		}
+		if ( 'edit.php' === $pagenow && ( isset( $_GET['post_type'] ) && self::POST_TYPE_SLUG === $_GET['post_type'] ) ) { // WPCS: CSRF OK.


Why not return the result of the conditional instead of explicitedly retuning true or false?

Sure. Done in a83a569.

hellofromtonya · 2018-08-29T00:38:50Z

includes/validation/class-amp-validation-error-taxonomy.php

+		if ( current_theme_supports( 'amp' ) ) {
+			return true;
+		}
+		if ( 'edit-tags.php' === $pagenow && ( isset( $_GET['taxonomy'] ) && self::TAXONOMY_SLUG === $_GET['taxonomy'] ) ) { // WPCS: CSRF OK.


Same here....return the result of the conditional expression instead of explicit trueor false. It’s less code and more clear.

…nce with limit

kienstra · 2018-08-29T01:05:12Z

includes/class-amp-cli.php

+		WP_CLI::success(
+			sprintf(
+				/* translators: $1%d is the number of URls crawled, $2%d is the number of validation issues, $3%d is the number of unaccepted issues, $4%s is the list of validation by type, $5%s is the link for more details */
+				__( '%3$d crawled URLs have unaccepted issue(s) out of %2$d total which AMP validation issue(s); %1$d URLs were crawled.', 'amp' ),


Maybe

total which AMP validation issue(s)

should be:

total with AMP validation issue(s)

kienstra · 2018-08-29T01:13:08Z

Commands Work As Expected

Hi @westonruter,
The WP-CLI commands here work as expected. Including:

wp amp validate-site

wp amp validate-site --include=is_author

wp amp validate-site --force (when in Classic mode)

wp amp validate-site --limit=2

Also, great idea to add links for the invalid URLs and errors in Classic mode:

postphotos · 2018-08-29T19:02:16Z

Hi @westonruter - really like the progress here and thanks for tagging this in #1359.

Also wanted to call out #1273 here - the idea I spelled out there could try to surface errors next to where toggles exist (so that a user is more likely to select modes when they might be afraid to do this). I'm not entirely sure this is necessary or how straightforward implementing it might be, but it's captured in the backlog regardless.

Both that feature (and #1273) are trying to take an approach to accomplish the inverse goals of #1366: Instead of surfacing settings info in the error listings, surface the error listings in the settings.

The key decisions I still think are up for discussion (past v1.0-stable) will reside around how much information a user should see and in what context that's necessary, but based on the work presented in #1006 I'm happy with the current efforts. Thanks for your help here!

cc: @jwold

Begin testing of site urls, starting with helper method.

305c28b

Define get_post_permalinks(). This gets permalinks for public posts, other than attachments. Another helper function can call AMP_Validation_Manager::validate_url() for each of these permalinks.

kienstra changed the title ~~[WIP] Ability to test AMP compatibility of entire site~~ [WIP] Test AMP compatibility of entire site May 30, 2018

kienstra added 3 commits May 29, 2018 23:49

Change helper method to output IDs, for use with existing method.

9e9df92

validate_queued_posts_on_frontend() uses IDs, which are stored in $posts_pending_frontend_validation. So simply return IDs from the helper function.

Change method name in test, assign $post_ids.

fd50993

Before, $post_ids wasn't assigned as an array(). This addresses a failed Travis build.

Query for the latest posts, instead of by ID.

1324044

Querying for ID was simply to make testing easier. So remove that, and fall back to the default 'orderby' => 'date'.

westonruter reviewed May 30, 2018

View reviewed changes

kienstra added 2 commits May 30, 2018 13:23

Change helper method to get post permalinks.

3c4db61

This class is not going to use this method anymore: validate_queued_posts_on_frontend(). So this will need the permalinks, not IDs.

Add a method to get links for public terms.

bbfafa4

For example, http://example.org/?cat=2 This includes categories and tags, and any more that are registered. But it excludes post_format links.

westonruter reviewed May 30, 2018

View reviewed changes

kienstra added 13 commits May 30, 2018 17:25

Also get links for post_format terms.

db54a53

Before, I had unset() these from the array. But as Weston pointed out, post_format terms can have archive links.

Begin to apply review suggestion for get_taxonomy_links().

6713ff3

Add a $taxonomy parameter, to only get the terms for a given taxonomy.

Add an $offset parameter to get_taxonomy_links().

2655310

As Weston mentioned, this helps to get the links in batches. Also, simplify the test.

Add (optional) to the @param tag for $offset.

32ab635

Pass 'fields' => 'ids' to WP_Query().

1a862c7

Props 10up Engineering Best Practices. The prevents the need to use wp_list_pluck() to get the ids.

Allow paging through posts in get_post_permalinks().

c250bd5

A similar change to the one Weston proposed for get_taxonomy_links(). Get permalinks by post types, and allow paging through them. This will allow iteration, in a way similar to that in get_taxonomy_links().

Add a method to crawl the entire site.

71f7543

Iterate over all of the terms and posts from the existing helper methods. @todo: consider self::BATCH_SIZE, and whether the validation requests should be throttled, or somehow prevented from timing out WP.

Add an 'amp' query arg if it's in Paired Mode.

ae4371c

In the validate_url() helper function, find if it's in Paired Mode. If so, set the $url to the amp endpoint.

Begin WP-CLI script to crawl the site.

7b67efe

This mainly calls the existing helper function. But it also passes it $wp_cli_progress. This advances the progress bar, after each post type or taxonomy validation completes.

Display the number of validation issues in the sucess message:

15ec0ed

Success: 196 URLs were crawled, and 196 have AMP validation issue(s). Query for the validation error posts. And output the number found.

Site crawling script: output 'more detail' link.

1ef10a7

This drives to the page: Invalid AMP Pages (URLs) Also, output a message at the beginning: Crawling the entire site to test for AMP validity. This might take a while...

westonruter reviewed Jun 5, 2018

View reviewed changes

kienstra added 3 commits July 11, 2018 23:49

Update PR for latest changes to validator, merge develop.

ba08f3d

3 assertions failed, so fix these both in the tested class and the PHPUnit class.

Register a WP-CLI command: wp amp validate-site

bfe37a8

On Weston's suggestion. Before, running the validation required: wp eval-file bin/validate-site.php.

Call the WP-CLI tick() method for every URL validated.

3d9a01c

This has a much better display of the activity. On Weston's suggestion. Create a new method to count all of the URLs to be validated. This should only be used for WP-CLI, as it uses 'posts_per_page' => -1

kienstra added 2 commits August 24, 2018 14:18

In Classic Mode, call WP_CLI::error() if the --force flag isn't present

da22efc

On Weston's suggestion, as this would not be able to validate any URL without the --force flag.

Allow validating the site in 'Classic' mode

bfadbe2

As Weston suggested, if in Classic mode and --force is passed, force theme support in the crawled URLs. This is now done in read_theme_support(), thought there might be a better way.

kienstra commented Aug 24, 2018

View reviewed changes

westonruter added 6 commits August 28, 2018 15:13

Fix phpdoc for WP-CLI

30f7522

Prevent crawling search or date URLs if empty; use for loop

e1760aa

Show warning when validate_url call fails

ad7e0e2

Fix classic mode site validation by forcing native mode

7a52751

* Only show admin URL to review results if theme support initially present. * Skip showing table if no results obtained.

Re-use amp_validate query param for forcing AMP theme support in clas…

8e2d028

…sic mode

westonruter approved these changes Aug 29, 2018

View reviewed changes

westonruter mentioned this pull request Aug 29, 2018

[Compatibility Tool] Better tooling for evaluating plugins #1007

Closed

3 tasks

hellofromtonya reviewed Aug 29, 2018

View reviewed changes

westonruter added 2 commits August 28, 2018 17:55

Switch to query posts in descending order for improved recency releva…

0e9bf1b

…nce with limit

Simplify should_show_in_menu return condition

a83a569

kienstra commented Aug 29, 2018

View reviewed changes

Fix grammar typo in CLI success message

19ead16

westonruter merged commit c539a18 into develop Aug 29, 2018

westonruter deleted the valid-site branch August 29, 2018 01:41

westonruter added this to the v1.0 milestone Aug 29, 2018

westonruter mentioned this pull request Aug 29, 2018

Generate userflow based on Validation Workflow efforts #1359

Closed

1 task

kienstra mentioned this pull request Sep 4, 2018

Add filters on taxonomy and post page #1373

Merged

kienstra mentioned this pull request Sep 18, 2018

Settings callout for "view your site as amp" after initial save #1399

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test AMP compatibility of entire site #1183

Test AMP compatibility of entire site #1183

kienstra commented May 30, 2018 •

edited by westonruter

Loading

westonruter May 30, 2018

kienstra May 30, 2018

westonruter May 30, 2018

kienstra May 30, 2018

westonruter May 30, 2018

kienstra May 30, 2018

westonruter commented May 30, 2018

kienstra commented Jun 1, 2018

westonruter commented Jun 5, 2018

westonruter Jun 5, 2018

westonruter Jun 5, 2018

westonruter Jun 5, 2018

westonruter Jun 5, 2018

kienstra Jul 12, 2018

kienstra commented Jun 7, 2018

westonruter commented Aug 24, 2018

kienstra commented Aug 24, 2018

kienstra commented Aug 24, 2018

kienstra Aug 24, 2018 •

edited

Loading

westonruter commented Aug 29, 2018

hellofromtonya Aug 29, 2018

westonruter Aug 29, 2018

hellofromtonya Aug 29, 2018

kienstra Aug 29, 2018

kienstra commented Aug 29, 2018

postphotos commented Aug 29, 2018

Test AMP compatibility of entire site #1183

Test AMP compatibility of entire site #1183

Conversation

kienstra commented May 30, 2018 • edited by westonruter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented May 30, 2018

kienstra commented Jun 1, 2018

westonruter commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Jun 7, 2018

westonruter commented Aug 24, 2018

kienstra commented Aug 24, 2018

kienstra commented Aug 24, 2018

kienstra Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

westonruter commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Aug 29, 2018

postphotos commented Aug 29, 2018

kienstra commented May 30, 2018 •

edited by westonruter

Loading

kienstra Aug 24, 2018 •

edited

Loading