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

Short-circuit validate_url() when attempting validation requests to other sites (which will always fail) #6582

Merged
merged 1 commit into from Aug 30, 2021

Conversation

westonruter
Copy link
Member

Summary

This fixes an issue I noticed when running wp amp validation run on a site which is configured to redirect permalinks for a custom post type to other sites. As a quick way to reproduce this issue, add the following plugin code:

add_filter(
	'page_link',
	function ( $url ) {
		if ( defined( 'WP_CLI' ) ) {
			$url = 'https://example.com/';
		}
		return $url;
	}
);

The output is as follows:

$ wp amp validation run --limit=1 --include=is_singular
Crawling the site for AMP validity.
Validating 3 URLs...  33 % [=================================================================>                                                                                                                                   ] 0:00 / 0:00
Warning: Validate URL error (response_not_json):  URL: https://example.com/
Validating 3 URLs...  100% [=====================================================================================================================================================================================================] 0:02 / 0:02
Success: 0 crawled URLs have invalid markup kept out of 0 total with AMP validation issue(s); 2 URLs were crawled.
+--------------------------+-----------+---------------+
| Template or content type | URL Count | Validity Rate |
+--------------------------+-----------+---------------+
| post                     | 1         | 100%          |
| attachment               | 1         | 100%          |
+--------------------------+-----------+---------------+
For more details, please see: https://wordpressdev.lndo.site/core-dev/src/wp-admin/edit.php?post_type=amp_validated_url

The output is a bit messy, but notice this line in particular:

Warning: Validate URL error (response_not_json): URL: https://example.com/

This means it's trying to fetch that other domain as if it is one of the URLs on the site. This obviously won't work. So we should short-circuit such requests from happening in the first place. This is addressed by this PR, so running the above command instead produces the following output:

$ wp amp validation run --limit=1 --include=is_singular
Crawling the site for AMP validity.
Validating 3 URLs...  33 % [=================================================================>                                                                                                                                   ] 0:00 / 0:00
Warning: Validate URL error (http_request_failed): Unable to validate a URL on another site. Attempted to validate: https://example.com/ URL: https://example.com/
Validating 3 URLs...  100% [=====================================================================================================================================================================================================] 0:02 / 0:03
Success: 0 crawled URLs have invalid markup kept out of 0 total with AMP validation issue(s); 2 URLs were crawled.
+--------------------------+-----------+---------------+
| Template or content type | URL Count | Validity Rate |
+--------------------------+-----------+---------------+
| post                     | 1         | 100%          |
| attachment               | 1         | 100%          |
+--------------------------+-----------+---------------+
For more details, please see: https://wordpressdev.lndo.site/core-dev/src/wp-admin/edit.php?post_type=amp_validated_url

Notice the message here instead:

Warning: Validate URL error (http_request_failed): Unable to validate a URL on another site. Attempted to validate: https://example.com/ URL: https://example.com/

Checklist

  • 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 westonruter added Bug Something isn't working Developer Tools labels Aug 30, 2021
@westonruter westonruter added this to the v2.1.4 milestone Aug 30, 2021
@westonruter westonruter added this to Ready for Review in Ongoing Aug 30, 2021
@github-actions
Copy link
Contributor

Plugin builds for 73059f9 are ready 🛎️!

Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

LGTM.

@westonruter westonruter merged commit a04a712 into develop Aug 30, 2021
@westonruter westonruter deleted the update/validate-url-validation branch August 30, 2021 21:31
westonruter added a commit that referenced this pull request Aug 30, 2021
@westonruter westonruter moved this from Ready for Review to Ready for QA in Ongoing Aug 30, 2021
@pierlon pierlon self-assigned this Aug 31, 2021
@pierlon
Copy link
Contributor

pierlon commented Aug 31, 2021

QA Passed

With a plugin active with the following code:

add_filter(
	'page_link',
	function ( $url ) {
		if ( defined( 'WP_CLI' ) ) {
			$url = 'https://example.com/';
		}
		return $url;
	}
);

Running the WP CLI command wp amp validation run --limit=1 --include=is_singular results in the output:

Crawling the site for AMP validity.
Validating 3 URLs...  33 % [=====================>                                           ] 0:00 / 0:00
Warning: Validate URL error (http_request_failed): Unable to validate a URL on another site. Attempted to validate: https://example.com/?amp=1 URL: https://example.com/
Validating 3 URLs...  100% [=================================================================] 0:01 / 0:01
Success: 0 crawled URLs have invalid markup kept out of 2 total with AMP validation issue(s); 2 URLs were crawled.
+--------------------------+-----------+---------------+
| Template or content type | URL Count | Validity Rate |
+--------------------------+-----------+---------------+
| post                     | 1         | 100%          |
| attachment               | 1         | 100%          |
+--------------------------+-----------+---------------+
For more details, please see: https://amp-qa.test/wp-admin/edit.php?post_type=amp_validated_url

Most notably the external URL being validated now results in a http_request_failed error and not a response_not_json.

@pierlon pierlon moved this from Ready for QA to QA Passed in Ongoing Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Developer Tools
Projects
Ongoing
  
QA Passed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants