ampproject/amp-wp

Remove restriction that external stylesheets have .css extension

westonruter opened this issue · 4 comments

Initially the inclusion of external styles were limited to those that were on the filesystem. But since #1083 we also fetch external styles over HTTP and then cache them. So now if a URL lacks the .css extension (or it is not found on the filesystem even) we can just fallback to fetching the stylesheet over the network.

This issue was noticed on WordPress.com VIP where automatic CSS concatenation results in a stylesheet URL that does not have a static filename extension.

Another related issue we can solve here. I've seen this come up twice now (e.g. #1269 (comment)) where a theme author includes a Google Font with an @import rule like:

@import 'https://fonts.googleapis.com/css?family=Merriweather:300|PT+Serif:400i|Open+Sans:800|Zilla+Slab:300,400,500|Montserrat:800|Muli:400&subset=cyrillic-ext,latin-ext,cyrillic,greek,greek-ext,vietnamese';

This causes a strange validation error message to be raised:

Skipped file which does not have an allowed file extension (//fonts.googleapis.com/css).

Using @import is not a recommended way to load CSS. It should use wp_enqueue_style() instead, similar to how Twenty Seventeen is doing. This will ensure that the stylesheet is loaded at the optimal time, and that WordPress can add the preconnect link.

Nevertheless, the AMP plugin should prevent trying to fetch such font stylesheets server-side to include in the concatenated script because AMP allows them to be referenced externally. In addition, the font should be cached according to its Cache-Control headers and not stored in a transient.

So when we come across such an @import, we should actually dynamically create a link element in the document with an href pointing to the imported URL.

igkuz commented

👍

Moving To 'Ready For Merging'

If it's alright, I'm moving this to 'Ready For Merging,' as I don't think it needs functional testing. Feel free to move it back.