Introduce AMP_WP_Styles for doing enqueued scripts inline #887
Conversation
westonruter
force-pushed
the
update/css-inclusion
branch
from
63d3463 to
a2dbae4
Compare
includes/class-amp-theme-support.php
Outdated
| */ | ||
| public static function override_wp_styles() { | ||
| global $wp_styles; | ||
| $wp_styles = new AMP_WP_Styles(); // WPCS: global override ok. |
There was a problem hiding this comment.
I think it would be safer to prevent the class from being initiated multiple times given that this method is public static. It would be helpful to return the styles too, for testibily for example.
global $wp_styles;
if ( ! ( $wp_styles instanceof AMP_WP_Styles ) ) {
$wp_styles = new AMP_WP_Styles();
}
return $wp_styles;
includes/class-amp-wp-styles.php
Outdated
| * @return string|WP_Error Style's absolute validated filesystem path, or WP_Error when error. | ||
| */ | ||
| public function get_validated_css_file_path( $src, $handle ) { | ||
| if ( ! is_bool( $src ) && ! preg_match( '|^(https?:)?//|', $src ) && ! ( $this->content_url && 0 === strpos( $src, $this->content_url ) ) ) { |
There was a problem hiding this comment.
Would be a bit more elegant to have the conditions with line breaks assigned to a variable.
There was a problem hiding this comment.
It's like this because it is forked/copied from core: https://github.com/WordPress/wordpress-develop/blob/4fb6c02b9961d41dcaaf0ea04e24223993fa8667/src/wp-includes/class.wp-styles.php#L327
includes/class-amp-wp-styles.php
Outdated
| // Strip query and fragment from URL. | ||
| $src = preg_replace( ':[\?#].+:', '', $src ); | ||
|
|
||
| $src = esc_url_raw( $src ); |
There was a problem hiding this comment.
Remove empty line above.
| * @since 0.7 | ||
| * @var bool | ||
| */ | ||
| protected $did_locale_stylesheet = false; |
There was a problem hiding this comment.
I personally prefer keeping all variables declarations above the methods, specifically since it is the case in all the other plugin's classes.
PS: this comment applies here too.
| $content_url = content_url( '/' ); | ||
| $admin_url = get_admin_url( null, '/' ); | ||
| $css_path = null; | ||
| if ( 0 === strpos( $src, $content_url ) ) { |
There was a problem hiding this comment.
What if the assets are in root folder? For instance could use the filesystem to create dir and cache all CSS which is then enqueued. Here is a function which converts url to path, has be perfected over the years to fix all bugs reported on all sorts of various WP sites structure. Here are all the Unit Tests for it.
There was a problem hiding this comment.
No assets from core, themes or plugins would be in the root folder. So I don't think we should accommodate them. Since we're actually getting the file source from the filesystem (rather than relying on whether the requesting user has HTTP access) and outputting it, I think we should be very restrictive about what we allow. For example, if there was a /private/secrets.js or something, we would not want this file to be enqueued. True we're talking about CSS here, but still.
There was a problem hiding this comment.
Sounds good, I attempted to test on Windows as I recalled issues when doing URL to path replacements but gave up due to issues encountered when trying to run a stable local install on my virtual machine. I will leave that task to QA (Claudio).
tests/test-class-amp-wp-styles.php
Outdated
| * | ||
| * @return WP_Styles|AMP_WP_Styles Styles. | ||
| */ | ||
| protected function amp_wp_styles() { |
There was a problem hiding this comment.
This method can be removed and AMP_Theme_Support::override_wp_styles() can be used instead if/when the comment left on the override_wp_styles() are applied.
* Let AMP_Theme_Support::override_wp_styles() re-set global if not already instantiated. * Move class variable definitions. * Break up conditional into multiple lines.
Dependent on ampproject/amp-wp#887 before working.
This makes enqueueing CSS much more seamless. The only requirement is that you enqueue CSS that exists on the filesystem in a theme/plugin, wp-admin, or wp-includes. It also handles added inline styles, locale stylesheets, RTL styles, and Custom CSS.
The
AMP_WP_Stylescan then be used to house the logic for “tree shaking” the CSS rules as well. For tree-shaking, we'd need to add a basic CSS tokenizer-parser, with a method that takes the$domobject from #875 for the buffer that is output, and then this can be used to grab all of theclassattributes; or it could just use a regexp to grab all of theclass="(.+?)"from the output buffer.Fixes #886.