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.

Sign up for GitHub

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

Introduce AMP_WP_Styles for doing enqueued scripts inline #887

Merged
merged 4 commits into from
Jan 23, 2018
Merged

Introduce AMP_WP_Styles for doing enqueued scripts inline #887

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2dbae4
Introduce AMP_WP_Styles for doing enqueued scripts inline
westonruter Jan 20, 2018
ed7d0d7
Prevent stylesheets from being done again; add tests for AMP_WP_Styles
westonruter Jan 20, 2018
9eeab51
Remove RTL test since requires a build
westonruter Jan 21, 2018
49c121e
Apply changes for code review
westonruter Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class AMP_Autoloader {
*/
private static $_classmap = array(
'AMP_Theme_Support' => 'includes/class-amp-theme-support',
'AMP_WP_Styles' => 'includes/class-amp-wp-styles',
'AMP_Template_Customizer' => 'includes/admin/class-amp-customizer',
'AMP_Post_Meta_Box' => 'includes/admin/class-amp-post-meta-box',
'AMP_Post_Type_Support' => 'includes/class-amp-post-type-support',
Expand Down
48 changes: 32 additions & 16 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,16 @@ public static function register_paired_hooks() {
public static function register_hooks() {

// Remove core actions which are invalid AMP.
remove_action( 'wp_head', 'locale_stylesheet' );
remove_action( 'wp_head', 'locale_stylesheet' ); // Replaced below in add_amp_custom_style_placeholder() method.
remove_action( 'wp_head', 'print_emoji_detection_script', 7 );
remove_action( 'wp_head', 'wp_print_styles', 8 );
remove_action( 'wp_head', 'wp_print_styles', 8 ); // Replaced below in add_amp_custom_style_placeholder() method.
remove_action( 'wp_head', 'wp_print_head_scripts', 9 );
remove_action( 'wp_head', 'wp_custom_css_cb', 101 );
remove_action( 'wp_head', 'wp_custom_css_cb', 101 ); // Replaced below in add_amp_custom_style_placeholder() method.
remove_action( 'wp_footer', 'wp_print_footer_scripts', 20 );
remove_action( 'wp_print_styles', 'print_emoji_styles' );

add_action( 'wp_enqueue_scripts', array( __CLASS__, 'override_wp_styles' ), -1 );

/*
* Replace core's canonical link functionality with one that outputs links for non-singular queries as well.
* See WP Core #18660.
Expand All @@ -169,11 +171,11 @@ public static function register_hooks() {
// @todo Add add_schemaorg_metadata(), add_analytics_data(), etc.
// Add additional markup required by AMP <https://www.ampproject.org/docs/reference/spec#required-markup>.
add_action( 'wp_head', array( __CLASS__, 'add_meta_charset' ), 0 );
add_action( 'wp_head', array( __CLASS__, 'add_meta_viewport' ), 2 );
add_action( 'wp_head', 'amp_print_boilerplate_code', 3 );
add_action( 'wp_head', array( __CLASS__, 'add_amp_component_scripts' ), 4 );
add_action( 'wp_head', array( __CLASS__, 'add_amp_custom_style_placeholder' ), 5 );
add_action( 'wp_head', 'amp_add_generator_metadata', 6 );
add_action( 'wp_head', array( __CLASS__, 'add_meta_viewport' ), 5 );
add_action( 'wp_head', 'amp_print_boilerplate_code', 7 );
add_action( 'wp_head', array( __CLASS__, 'add_amp_custom_style_placeholder' ), 8 ); // Because wp_print_styles() normally happens at 8.
add_action( 'wp_head', array( __CLASS__, 'add_amp_component_scripts' ), 10 );
add_action( 'wp_head', 'amp_add_generator_metadata', 20 );

/*
* Disable admin bar because admin-bar.css (28K) and Dashicons (48K) alone
Expand All @@ -192,6 +194,17 @@ public static function register_hooks() {
// @todo Add character conversion.
}

/**
* Preemtively define $wp_styles as AMP_WP_Styles.
*
* @see wp_styles()
* @global AMP_WP_Styles $wp_styles
*/
public static function override_wp_styles() {
global $wp_styles;
$wp_styles = new AMP_WP_Styles(); // WPCS: global override ok.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;

}

/**
* Register content embed handlers.
*
Expand Down Expand Up @@ -376,6 +389,16 @@ public static function add_amp_custom_style_placeholder() {
echo '<style amp-custom>';
echo self::CUSTOM_STYLES_PLACEHOLDER; // WPCS: XSS OK.
echo '</style>';

$wp_styles = wp_styles();
if ( ! ( $wp_styles instanceof AMP_WP_Styles ) ) {
trigger_error( esc_html__( 'wp_styles() does not return an instance of AMP_WP_Styles as required.', 'amp' ), E_USER_WARNING ); // phpcs:ignore
return;
}

$wp_styles->do_items(); // Normally done at wp_head priority 8.
$wp_styles->do_locale_stylesheet(); // Normally done at wp_head priority 10.
$wp_styles->do_custom_css(); // Normally done at wp_head priority 101.
}

/**
Expand All @@ -385,11 +408,7 @@ public static function add_amp_custom_style_placeholder() {
* @return string Styles.
*/
public static function get_amp_custom_styles() {

// @todo Grab source of all enqueued styles and concatenate here?
// @todo Print contents of get_locale_stylesheet_uri()?
$path = get_template_directory() . '/style.css'; // @todo Honor filter in get_stylesheet_directory_uri()? Style must be local.
$css = file_get_contents( $path ); // phpcs:ignore WordPress.WP.AlternativeFunctions -- It's not a remote file.
$css = wp_styles()->print_code;

// Add styles gleaned from sanitizers.
foreach ( self::$amp_styles as $selector => $properties ) {
Expand All @@ -400,9 +419,6 @@ public static function get_amp_custom_styles() {
);
}

// Do AMP version of wp_custom_css_cb().
$css .= wp_get_custom_css();

/**
* Filters AMP custom CSS before it is injected onto the output buffer for the response.
*
Expand Down
216 changes: 216 additions & 0 deletions includes/class-amp-wp-styles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
<?php
/**
* Class AMP_WP_Styles
*
* @package AMP
*/

/**
* Extend WP_Styles with handling of CSS for AMP.
*
* @since 0.7
*/
class AMP_WP_Styles extends WP_Styles {

/**
* Concatenation is always enabled for AMP.
*
* @since 0.7
* @var bool
*/
public $do_concat = true;

/**
* Generates an enqueued style's fully-qualified file path.
*
* @since 0.7
* @see WP_Styles::_css_href()
*
* @param string $src The source URL of the enqueued style.
* @param string $handle The style's registered handle.
* @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 ) ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be a bit more elegant to have the conditions with line breaks assigned to a variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

$src = $this->base_url . $src;
}

/** This filter is documented in wp-includes/class.wp-styles.php */
$src = apply_filters( 'style_loader_src', $src, $handle );

// Strip query and fragment from URL.
$src = preg_replace( ':[\?#].+:', '', $src );

$src = esc_url_raw( $src );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove empty line above.


// @todo Explicitly not using includes_url() or content_url() since filters may point outside filesystem?
$includes_url = includes_url( '/' );
$content_url = content_url( '/' );
$admin_url = get_admin_url( null, '/' );
$css_path = null;
if ( 0 === strpos( $src, $content_url ) ) {
Copy link
Collaborator

@ThierryA ThierryA Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

$css_path = WP_CONTENT_DIR . substr( $src, strlen( $content_url ) - 1 );
} elseif ( 0 === strpos( $src, $includes_url ) ) {
$css_path = ABSPATH . WPINC . substr( $src, strlen( $includes_url ) - 1 );
} elseif ( 0 === strpos( $src, $admin_url ) ) {
$css_path = ABSPATH . 'wp-admin' . substr( $src, strlen( $admin_url ) - 1 );
}

if ( ! preg_match( '/\.(css|less|scss|sass)$/i', $css_path ) ) {
/* translators: %1$s is stylesheet handle, %2$s is stylesheet URL */
return new WP_Error( 'amp_css_bad_file_extension', sprintf( __( 'Skipped stylesheet %1$s which does not have recognized CSS file extension (%2$s).', 'amp' ), $handle, $src ) );
}

if ( ! $css_path || false !== strpos( '../', $css_path ) || 0 !== validate_file( $css_path ) || ! file_exists( $css_path ) ) {
/* translators: %1$s is stylesheet handle, %2$s is stylesheet URL */
return new WP_Error( 'amp_css_path_not_found', sprintf( __( 'Unable to locate filesystem path for stylesheet %1$s (%2$s).', 'amp' ), $handle, $src ) );
}

return $css_path;
}

/**
* Processes a style dependency.
*
* @since 0.7
* @see WP_Styles::do_item()
*
* @param string $handle The style's registered handle.
* @return bool True on success, false on failure.
*/
public function do_item( $handle ) {
if ( ! WP_Dependencies::do_item( $handle ) ) {
return false;
}
$obj = $this->registered[ $handle ];

// Conditional styles and alternate styles aren't supported in AMP.
if ( isset( $obj->extra['conditional'] ) || isset( $obj->extra['alt'] ) ) {
return false;
}

if ( isset( $obj->args ) ) {
$media = esc_attr( $obj->args );
} else {
$media = 'all';
}

// A single item may alias a set of items, by having dependencies, but no source.
if ( ! $obj->src ) {
$inline_style = $this->print_inline_style( $handle, false );
if ( $inline_style ) {
$this->print_code .= $inline_style;
}
return true;
}

$css_file_path = $this->get_validated_css_file_path( $obj->src, $handle );
if ( is_wp_error( $css_file_path ) ) {
trigger_error( esc_html( $css_file_path->get_error_message() ), E_USER_WARNING ); // phpcs:ignore
return false;
}
$css_rtl_file_path = '';

// Handle RTL styles.
if ( 'rtl' === $this->text_direction && isset( $obj->extra['rtl'] ) && $obj->extra['rtl'] ) {
if ( is_bool( $obj->extra['rtl'] ) || 'replace' === $obj->extra['rtl'] ) {
$suffix = isset( $obj->extra['suffix'] ) ? $obj->extra['suffix'] : '';
$css_rtl_file_path = $this->get_validated_css_file_path(
str_replace( "{$suffix}.css", "-rtl{$suffix}.css", $obj->src ),
"$handle-rtl"
);
} else {
$css_rtl_file_path = $this->get_validated_css_file_path( $obj->extra['rtl'], "$handle-rtl" );
}

if ( is_wp_error( $css_rtl_file_path ) ) {
trigger_error( esc_html( $css_rtl_file_path->get_error_message() ), E_USER_WARNING ); // phpcs:ignore
$css_rtl_file_path = null;
} elseif ( 'replace' === $obj->extra['rtl'] ) {
$css_file_path = null;
}
}

// Load the CSS from the filesystem.
foreach ( array_filter( array( $css_file_path, $css_rtl_file_path ) ) as $css_path ) {
$css = file_get_contents( $css_path ); // phpcs:ignore -- It's a local filesystem path not a remote request.
if ( 'all' !== $media ) {
$css = sprintf( '@media %s { %s }', $media, $css );
}
$this->print_code .= $css;
}

// Add inline styles.
$inline_style = $this->print_inline_style( $handle, false );
if ( $inline_style ) {
$this->print_code .= $inline_style;
}

return true;
}

/**
* Whether the locale stylesheet was done.
*
* @since 0.7
* @var bool
*/
protected $did_locale_stylesheet = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


/**
* Get the locale stylesheet if it exists.
*
* @since 0.7
* @see locale_stylesheet()
* @return bool Whether locale stylesheet was done.
*/
public function do_locale_stylesheet() {
if ( $this->did_locale_stylesheet ) {
return true;
}

$src = get_locale_stylesheet_uri();
if ( ! $src ) {
return false;
}
$path = $this->get_validated_css_file_path( $src, get_stylesheet() . '-' . get_locale() );
if ( is_wp_error( $path ) ) {
return false;
}
$this->print_code .= file_get_contents( $path ); // phpcs:ignore -- The path has been validated, and it is not a remote path.
$this->did_locale_stylesheet = true;
return true;
}

/**
* Whether the Custom CSS was done.
*
* @since 0.7
* @var bool
*/
protected $did_custom_css = false;

/**
* Append Customizer Custom CSS.
*
* @since 0.7
* @see wp_custom_css()
* @see wp_custom_css_cb()
* @return bool Whether locale Custom CSS was done.
*/
public function do_custom_css() {
if ( $this->did_custom_css ) {
return true;
}

$css = trim( wp_get_custom_css() );
if ( ! $css ) {
return false;
}

$this->print_code .= $css;

$this->did_custom_css = true;
return true;
}
}
Loading