Not enough information given when fetch_external_stylesheet failure happened

[pentatonicfunk]

Version : 1.1.1

When fetching external stylesheet failure that indicated by WP_Error on wp_remote_get, there is no code and message displayed on Validations Page.

Example case : when Invalid SSL being used on external stylesheet url.

This is caused by wp_remote_retrieve_response_code and wp_remote_retrieve_response_message are empty

https://github.com/ampproject/amp-wp/blob/1.1.1/includes/sanitizers/class-amp-style-sanitizer.php#L1111

private function fetch_external_stylesheet( $url ) {
		$cache_key = md5( $url );
		$contents  = get_transient( $cache_key );
		if ( false === $contents ) {
			$r = wp_remote_get( $url );
                        // TODO: Need to handle WP_Error
			if ( 200 !== wp_remote_retrieve_response_code( $r ) ) {
				$contents = new WP_Error(
					wp_remote_retrieve_response_code( $r ),
					wp_remote_retrieve_response_message( $r )
				);
			} elseif ( ! preg_match( '#^text/css#', wp_remote_retrieve_header( $r, 'content-type' ) ) ) {
				$contents = new WP_Error(
					'no_css_content_type',
					__( 'Response did not contain the expected text/css content type.', 'amp' )
				);
			} else {
				$contents = wp_remote_retrieve_body( $r );
			}
			set_transient( $cache_key, $contents, MONTH_IN_SECONDS );
		}
		return $contents;
	}

Possible Solution : Add WP_Error handler after wp_remote_get to catch code and message, that will be usable on later stage

[pentatonicfunk]

And i dont think we should save Error on transient.
In some case ( moreover in development mode ) the error on transient will be used for MONTH, even its already fixed.

[westonruter]

@pentatonicfunk Thanks for the report. This actually was revisited a week ago in this commit fd23dc0 as part of #2357:

amp-wp/includes/sanitizers/class-amp-style-sanitizer.php

Lines 1226 to 1255 in ce430d5

	private function fetch_external_stylesheet( $url ) {
	$cache_key = md5( $url );
	$contents = get_transient( $cache_key );
	if ( false === $contents ) {
	$r = wp_remote_get( $url );
	$code = wp_remote_retrieve_response_code( $r );
	if ( $code < 200 || $code >= 300 ) {
	$message = wp_remote_retrieve_response_message( $r );
	if ( ! $code ) {
	$code = 'http_error';
	} else {
	$code = "http_{$code}";
	}
	if ( ! $message ) {
	/* translators: %s: the fetched URL */
	$message = sprintf( __( 'Failed to fetch: %s', 'amp' ), $url );
	}
	$contents = new WP_Error( $code, $message );
	} elseif ( ! preg_match( '#^text/css#', wp_remote_retrieve_header( $r, 'content-type' ) ) ) {
	$contents = new WP_Error(
	'no_css_content_type',
	__( 'Response did not contain the expected text/css content type.', 'amp' )
	);
	} else {
	$contents = wp_remote_retrieve_body( $r );
	}
	set_transient( $cache_key, $contents, MONTH_IN_SECONDS );
	}
	return $contents;
	}

This change didn't make it into the 1.2-beta1 release, but here is the latest build from develop with the change: amp.zip (v1.2-beta1-20190528T235705Z-ce430d54)

Could you test that and review?

A couple things that are missing based on your feedback is:

• The WP_Error should not be saved for MONTH_IN_SECONDS but rather much shorter, like DAY_IN_SECONDS.
• In the case of a successful response, we should use the Cache-Control max-age or Expires header to determine the expiration time.

[westonruter]

I fixed this in #4257, other than the reduction in the of the expiration time.

[westonruter]

And #4293 captures the improvement to reduce the expiration time for cached errors.