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.

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
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
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
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
@@ -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.

$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

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;
}
}