Skip to content

Use Cache-Control directive for external stylesheet caching#4404

Merged
westonruter merged 15 commits intodevelopfrom
4293-improve-external-stylesheet-caching
Mar 21, 2020
Merged

Use Cache-Control directive for external stylesheet caching#4404
westonruter merged 15 commits intodevelopfrom
4293-improve-external-stylesheet-caching

Conversation

@schlessera
Copy link
Copy Markdown
Collaborator

@schlessera schlessera commented Mar 18, 2020 •
edited by westonruter
Loading

Summary

Fixes #2092

This PR does the following:

  • Moves CachedRemoteGetRequest and CachedData into a subnamespace AmpProject\AmpWP\RemoteRequest
  • Adds a WpHttpRemoteGetRequest implementation using the WordPress WP_Http abstraction to implement RemoteGetRequest
  • Makes the CachedRemoteGetRequest implementation more robust
  • Uses WpHttpRemoteGetRequest for remote requests in the optimizer
  • Uses WpHttpRemoteGetRequest to fetch and cache external stylesheets

The WpHttpRemoteGetRequest implementation will respect the max-age directive of the Cache-Control header and will automatically retry failed remote requests for the following status codes:

  • WP_Http::REQUEST_TIMEOUT (408)
  • WP_Http::LOCKED (423)
  • WP_Http::TOO_MANY_REQUESTS (429)
  • WP_Http::INTERNAL_SERVER_ERROR (500)
  • WP_Http::SERVICE_UNAVAILABLE (503)
  • WP_Http::GATEWAY_TIMEOUT (504)

Fixes #4293

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • 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).

@googlebot googlebot added the cla: yes Signed the Google CLA label Mar 18, 2020
@schlessera schlessera requested review from kienstra, pierlon and westonruter and removed request for westonruter March 18, 2020 07:07
@schlessera schlessera added the CSS label Mar 18, 2020
@westonruter
Copy link
Copy Markdown
Member

westonruter commented Mar 18, 2020

@schlessera I'm getting warnings still, but different than before:

[18-Mar-2020 16:55:25 UTC] PHP Warning:  array_keys() expects parameter 1 to be array, object given in /app/public/content/plugins/amp/lib/common/src/RemoteRequest/RemoteGetRequestResponse.php on line 178
[18-Mar-2020 16:55:25 UTC] PHP Warning:  array_combine() expects parameter 1 to be array, null given in /app/public/content/plugins/amp/lib/common/src/RemoteRequest/RemoteGetRequestResponse.php on line 178
[18-Mar-2020 16:55:25 UTC] PHP Warning:  array_change_key_case() expects parameter 1 to be array, null given in /app/public/content/plugins/amp/lib/common/src/RemoteRequest/RemoteGetRequestResponse.php on line 179
[18-Mar-2020 16:55:25 UTC] PHP Warning:  array_key_exists() expects parameter 2 to be array, null given in /app/public/content/plugins/amp/lib/common/src/RemoteRequest/RemoteGetRequestResponse.php on line 98

I can reproduce this each time after I wp transient delete --all and then access an AMP page.

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Mar 18, 2020 •
edited
Loading

I've determined the cause to be attempted passing headers as Requests_Utility_CaseInsensitiveDictionary instead of as an array. Fixed in 2f0652a 24c295f by converting to an array. You may have a better way to do that however.

@schlessera
Copy link
Copy Markdown
Collaborator Author

schlessera commented Mar 18, 2020

I'll add a regression test and fix it.

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Mar 18, 2020

Maybe check first whether it actually is an iterator object, as it could also still be an array instead.

Good point. Since is_iterable() is only available in PHP 7, what about:

$headers = is_object( $response['headers'] ) ? iterator_to_array( $response['headers'] ) : (array) $response['headers'];

Anyway, I'll leave you to it 😄

@schlessera
Copy link
Copy Markdown
Collaborator Author

schlessera commented Mar 18, 2020

The correct interface to check would be https://www.php.net/manual/en/class.traversable.php

@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into 4293-impr…
97988e6 
…ove-external-stylesheet-caching

* 'develop' of github.com:ampproject/amp-wp:
  Use static variable for computed constant string
  Improve phpdoc for Fonts
  Use AssertContainsCompatibility
  Ensure color emoji are used for warning symbol
  Add test for amp-inline-gallery with thumbnails
  Add test for amp-nested-menu with amp-list
  Revert "Add (failing) tests for amp-mega-menu reference points"
  Add (failing) tests for amp-mega-menu reference points
  Add tests for non-reference point amp-mega-menu markup
  Add test for amp-subscriptions and note what is lacking
  Add test for form validation and amp-next-page
  Add test for amp-story-page[next-page-no-ad], input[type=button], and LTS AMP runtime
  Add tests for amp-script, amp-next-page, amp-date-picker, and amp-redbull-player
  Add tests for amp-nested-menu, amp-mega-menu, amp-autocomplete, dock on amp-brid-player, link protocol/type
  Add test for amp-inline-gallery
  Remove test for obsolete component amp-user-location
  Ensure unique GetMandatoryOf parsed from spec
  Update allowed tags/attributes from spec in amphtml 2003031842100
@westonruter
Only do iterator_to_array() if Traversable
f7034c1
@westonruter
Copy link
Copy Markdown
Member

westonruter commented Mar 18, 2020

Build for testing: amp.zip (v1.5.0-alpha-20200318T200637Z-f7034c1c7)

westonruter
westonruter reviewed Mar 19, 2020
View reviewed changes
westonruter
westonruter reviewed Mar 19, 2020
View reviewed changes
@schlessera schlessera requested a review from westonruter March 20, 2020 10:34
westonruter
westonruter reviewed Mar 20, 2020
View reviewed changes
@westonruter westonruter added this to the v1.5 milestone Mar 20, 2020
@kienstra
Copy link
Copy Markdown
Contributor

kienstra commented Mar 20, 2020 •
edited
Loading

Testing looks good

Hi @westonruter and @schlessera,
This looks good!

  1. An external URL that does exist (200 response) is cached as expected. It stores an expiration, and successive calls to fetch_external_stylesheet() return the cached value.

expiry-stored

  1. An external URL that doesn't exist (404 response) is cached as expected, and this doesn't retry the request again.

Here's a snippet I used to test this:

add_action(
	'wp_enqueue_scripts',
	static function() {
		wp_enqueue_style( 'example-valid-external-style', 'https://cdnjs.cloudflare.com/ajax/libs/modern-normalize/0.6.0/modern-normalize.min.css' );
		wp_enqueue_style( 'non-existent-external-style', 'https://example.com/non-existent.css' );
	}
);

Sorry for the delay on this.

westonruter
westonruter reviewed Mar 21, 2020
View reviewed changes
@westonruter westonruter merged commit 182634c into develop Mar 21, 2020
@westonruter westonruter deleted the 4293-improve-external-stylesheet-caching branch March 21, 2020 03:56
@westonruter westonruter mentioned this pull request Mar 23, 2020
Closed
@conwaydev conwaydev mentioned this pull request Apr 1, 2020
Closed
@schlessera schlessera added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonruter westonruter westonruter approved these changes

@kienstra kienstra Awaiting requested review from kienstra

@pierlon pierlon Awaiting requested review from pierlon

Assignees

No one assigned

Labels

Caching cla: yes Signed the Google CLA CSS Optimizer Sanitizers WS:Perf Work stream for Metrics, Performance and Optimizer

Projects

None yet

Milestone

v1.5

Development

Successfully merging this pull request may close these issues.

Improve caching of externally-fetched stylesheets Detect high cache miss rate for obtaining parsed CSS from variable stylesheets

4 participants

@schlessera @westonruter @kienstra @googlebot