Skip to content

Introduce ampdevmode data flag on script/style dependencies #4001

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

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Introduce ampdevmode data flag on script/style dependencies #4001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
739a8f7
Introduce ampdevmode data flag on script/style dependencies to indica…
westonruter Dec 20, 2019
399f9b5
Obtain sanitizer args after template rendering so done script/style h…
westonruter Dec 20, 2019
9db65fc
Add assertions to test_amp_get_content_sanitizers_with_dev_mode
westonruter Dec 20, 2019
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
22 changes: 21 additions & 1 deletion includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,8 +960,28 @@ function amp_get_content_sanitizers( $post = null ) {
if ( is_admin_bar_showing() ) {
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]';
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]//*';
$dev_mode_xpaths[] = '//style[ @id = "admin-bar-inline-css" ]';
}

// Ensure script localization data gets flagged for dev mode. This only applies to wp_localize_script() as
// inline scripts added via wp_add_inline_script() get filtered by script_loader_tag and thus will have the
// data-ampdevmode attribute added via AMP_Theme_Support::filter_script_loader_tag_for_dev_mode().
foreach ( wp_scripts()->done as $script_handle ) {
if ( ! AMP_Theme_Support::dependency_needs_dev_mode( wp_scripts(), $script_handle ) ) {
continue;
}
$data = wp_scripts()->get_data( $script_handle, 'data' );
if ( preg_match( '/(\bvar\s*\w+\s+=)/', $data, $matches ) ) {
$dev_mode_xpaths[] = sprintf( '//script[ not( @src ) ][ contains( text(), "%s" ) ]', $matches[1] );
}
}

// Ensure all inline styles added via wp_add_inline_style() get the data-ampdevmode attribute.
foreach ( wp_styles()->done as $style_handle ) {
if ( AMP_Theme_Support::dependency_needs_dev_mode( wp_styles(), $style_handle ) ) {
$dev_mode_xpaths[] = sprintf( '//style[ @id = "%s" ]', "$style_handle-inline-css" );
}
}

$sanitizers = array_merge(
[
'AMP_Dev_Mode_Sanitizer' => [
Expand Down
105 changes: 64 additions & 41 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,6 @@ class AMP_Theme_Support {

const PAIRED_BROWSING_QUERY_VAR = 'amp-paired-browsing';

/**
* Sanitizer classes.
*
* @var array
*/
protected static $sanitizer_classes = [];

/**
* Embed handlers.
*
Expand Down Expand Up @@ -442,20 +435,32 @@ static function() {
}

self::add_hooks();
self::$sanitizer_classes = amp_get_content_sanitizers();
if ( ! $is_reader_mode ) {
self::$sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( self::$sanitizer_classes );
}
self::$embed_handlers = self::register_content_embed_handlers();
self::$sanitizer_classes['AMP_Embed_Sanitizer']['embed_handlers'] = self::$embed_handlers;

foreach ( self::$sanitizer_classes as $sanitizer_class => $args ) {
// @todo It is not ideal that get_sanitizer_classes() is called here before the template is rendered and after the template is rendered.
foreach ( self::get_sanitizer_classes() as $sanitizer_class => $args ) {
if ( method_exists( $sanitizer_class, 'add_buffering_hooks' ) ) {
call_user_func( [ $sanitizer_class, 'add_buffering_hooks' ], $args );
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@schlessera This relates to the middleware (#3662). I realized that it is not ideal to collect the sanitizers for the document before the template is rendered because some of the args could very well be dependent on what is rendered on the template.

This would open up an optimization where certain sanitizers could be omitted entirely from running if we don't detect they would be needed. For example, no need to run the AMP_Gallery_Block_Sanitizer if no gallery was added to the page.

However, the sanitizers were relatively recently extended to allow them to run certain logic before the template is rendered. This logic is stored in the add_buffering_hooks static method. Only two sanitizers make use of this:

  • AMP_Core_Theme_Sanitizer
  • AMP_Nav_Menu_Dropdown_Sanitizer

The current state of this PR ends up calling self::get_sanitizer_classes() both before and after template rendering, which is not ideal.

The thing I'm thinking right now is the add_buffering_hooks methods of these two classes should be extracted into entirely separate classes which are not called sanitizers at all, but rather something like “output preparers”. Separating them out from the sanitizers would eliminate an oddity of AMP_Core_Theme_Sanitizer where there static methods are invoked before template rendering and non-static methods are invoked after template rendering during post-processing.

Going classic WordPress route, there could just be a do_action( 'amp_before_render_template' ) here which the plugin hooks into via something like:

add_action( 'amp_before_render_template', 'amp_prepare_template_rendering_for_core_theme' );
add_action( 'amp_before_render_template', 'amp_prepare_template_rendering_for_nav_menu_dropdowns' );

But I figured you would not approve of this idea 😄

What do you think about architecting this? We should be free to extract add_buffering_hooks method from the AMP_Base_Sanitizer without worrying about backwards-compatibility since it is not being used by any other plugins or themes.

Something else which should be touched by such changes is:

amp-wp/includes/class-amp-theme-support.php

Lines 187 to 190 in ebead4f

if ( self::READER_MODE_SLUG !== self::get_support_mode() ) {
// Ensure extra theme support for core themes is in place.
AMP_Core_Theme_Sanitizer::extend_theme_support();
}

amp-wp/includes/sanitizers/class-amp-core-theme-sanitizer.php

Lines 267 to 289 in ebead4f

/**
* Adds extra theme support arguments on the fly.
*
* This method is neither a buffering hook nor a sanitization callback and is called manually by
* {@see AMP_Theme_Support}. Typically themes will add theme support directly and don't need such
* a method. In this case, it is a workaround for adding theme support on behalf of external themes.
*
* @since 1.1
*/
public static function extend_theme_support() {
$args = self::get_theme_support_args( get_template() );
if ( empty( $args ) ) {
return;
}
$support = AMP_Theme_Support::get_theme_support_args();
if ( ! is_array( $support ) ) {
$support = [];
}
add_theme_support( AMP_Theme_Support::SLUG, array_merge( $support, $args ) );
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Forgetting about the WordPress and/or AMP context first, let me briefly describe how I generally go about architecting something like this.

For dealing with the different times at which a sanitizer might need to act, I'd have lifecycle methods/interfaces.

So, if a sanitizer needs preparation beforehand, for example, this could look something like this (naming TBD):

interface NeedsPreparation {
	public function prepare( $context );
}
final class AMP_Core_Theme_Sanitizer
	extends AMP_Base_Sanitizer
	implements NeedsPreparation {

	public function prepare( $context ) {
		// Do something here.
	}
}
foreach ( $sanitizers as $sanitizer ) {
	if ( $sanitizer instanceof NeedsPreparation ) {
		$sanitizer->prepare( $context );
	}
}

I find this to be cleaner, faster and more flexible than the method_exists() way of running such methods, or having a base class with empty methods as placeholders to override.

This is typically used to hook a given object into different parts of the application lifecycle without the application needing to hardcode any knowledge about it. You can have interfaces like Registerable, Bootable, Activateable, ...

So a construct like this would replace the add_buffering_hooks() logic we have right now. You can still attach such logic to WordPress hooks if you want, but they are decoupled then, and the sanitizers can still be used outside of WordPress.

If instantiation should be conditional, I tend to actually use a Conditional interface with a static method:

interface Conditional {
	public static function is_needed( $context );
}
foreach ( $sanitizer_classes as $class ) {
	if ( is_a( $class, Conditional::class, true )
		&& ( ! $class::is_needed( $context ) ) {
		continue;
	}
	$sanitizers[ $class ] = $this->instantiate_sanitizer( $class );
}

However, in this instance, I'm not sure we need to deal with conditional instantiation, as all of the sanitizers are cheap to instantiate. We are sharing a $dom object across sanitizers, and with some of the refactorings I made, most of the operations that are not exclusive to a single sanitizer are done in a centralized and shared way, instead of multiple times across sanitizers. Adding conditional instantiation might add complexity here that is not warranted.

I think we should just look into fast and efficient ways to bail early for any given sanitizer at each point in their lifecycle instead. Also, if more shared code and/or data is needed, we could look into having a shared Amp\Sanitizer\Context object that we inject into each sanitizer, instead of the $dom and other things individually. This could also open up the possibility for inter-dependencies if we want.

So if a sanitizer doesn't have anything to do for a given template, the simplest way is to start by checking the template and bailing if no processing is needed. This can be made pretty efficient with the new $dom and other shared data/logic.

Copy link
Collaborator

@schlessera schlessera Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking through your idea of splitting the logic into separate objects, we could think in more general terms of what we need to have in place for a complete roundtrip:

  • preparation
  • normalization
  • sanitization
  • optimization
  • validation

In terms of separate packages as the targeted result, I think we could then end up with:

  • Amp\Common (common base code like the Dom\Document abstraction)
  • Amp\Sanitizer (this would include preparation and normalization)
  • Amp\Optimizer (this assumes sanitized Amp markup)
  • Amp\Validator (this can validate both optimized and unoptimized Amp markup).
  • [ Amp\Middleware (this would be a complete PSR-15 request handler that combines all of the above in a middleware that can just be hooked into a standard PSR-7/PSR-15 stack) ]

These five packages would be pure PHP PSR-12 packages. So all integrations with WordPress should be handled in addition to the OOP mechanisms we would already have in place within these to control the lifecycle. This is why I was talking about "decoupling from WordPress" in the previous comment.

@westonruter Does this separation/categorization make sense to you? If so, I suggest I start a design document with the above where we can then discuss the lifecycles of each of these.

}

/**
* Get sanitizer classes.
*
* @see amp_get_content_sanitizers()
*
* @return array Mapping of sanitizer class name to constructor args array.
*/
protected static function get_sanitizer_classes() {
$sanitizer_classes = amp_get_content_sanitizers();
if ( self::READER_MODE_SLUG !== self::get_support_mode() ) {
$sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( $sanitizer_classes );
}
$sanitizer_classes['AMP_Embed_Sanitizer']['embed_handlers'] = self::$embed_handlers;
return $sanitizer_classes;
}

/**
* Ensure that the current AMP location is correct.
*
Expand Down Expand Up @@ -1098,6 +1103,8 @@ static function( $html ) {
);

add_action( 'admin_bar_init', [ __CLASS__, 'init_admin_bar' ] );
add_filter( 'style_loader_tag', [ __CLASS__, 'filter_style_loader_tag_for_dev_mode' ], 10, 2 );
add_filter( 'script_loader_tag', [ __CLASS__, 'filter_script_loader_tag_for_dev_mode' ], 10, 2 );
add_action( 'wp_head', 'amp_add_generator_metadata', 20 );
add_action( 'wp_enqueue_scripts', [ __CLASS__, 'enqueue_assets' ], 0 ); // Enqueue before theme's styles.
add_action( 'wp_enqueue_scripts', [ __CLASS__, 'dequeue_customize_preview_scripts' ], 1000 );
Expand Down Expand Up @@ -1427,8 +1434,6 @@ public static function filter_cancel_comment_reply_link( $formatted_link, $link,
* @since 1.0
*/
public static function init_admin_bar() {
add_filter( 'style_loader_tag', [ __CLASS__, 'filter_admin_bar_style_loader_tag' ], 10, 2 );
add_filter( 'script_loader_tag', [ __CLASS__, 'filter_admin_bar_script_loader_tag' ], 10, 2 );

// Inject the data-ampdevmode attribute into the admin bar bump style. See \WP_Admin_Bar::initialize().
if ( current_theme_supports( 'admin-bar' ) ) {
Expand Down Expand Up @@ -1551,40 +1556,53 @@ protected static function is_exclusively_dependent( WP_Dependencies $dependencie
}

/**
* Add data-ampdevmode attribute to any enqueued style that depends on the admin-bar.
* Determine whether a given dependency handle needs dev mode.
*
* @since 1.3
* @since 1.5
*
* @param WP_Dependencies $dependencies Dependencies (wither WP_Scripts or WP_Styles).
* @param string $handle Dependency handle (for script or style).
* @return bool Whether the <script>, <link>, or <style> needs dev mode.
*/
public static function dependency_needs_dev_mode( WP_Dependencies $dependencies, $handle ) {
return (
$dependencies->get_data( $handle, 'ampdevmode' )
||
(
in_array( $handle, $dependencies->registered['admin-bar']->deps, true ) ?
self::is_exclusively_dependent( $dependencies, $handle, 'admin-bar' ) :
self::has_dependency( $dependencies, $handle, 'admin-bar' )
)
);
}

/**
* Add data-ampdevmode attribute to any enqueued style that is flagged for dev mode or which depends on the admin-bar.
*
* @since 1.5
*
* @param string $tag The link tag for the enqueued style.
* @param string $handle The style's registered handle.
* @return string Tag.
*/
public static function filter_admin_bar_style_loader_tag( $tag, $handle ) {
if (
in_array( $handle, wp_styles()->registered['admin-bar']->deps, true ) ?
self::is_exclusively_dependent( wp_styles(), $handle, 'admin-bar' ) :
self::has_dependency( wp_styles(), $handle, 'admin-bar' )
) {
public static function filter_style_loader_tag_for_dev_mode( $tag, $handle ) {
if ( self::dependency_needs_dev_mode( wp_styles(), $handle ) ) {
$tag = preg_replace( '/(?<=<link)(?=\s|>)/i', ' ' . AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, $tag );
}
return $tag;
}

/**
* Add data-ampdevmode attribute to any enqueued script that depends on the admin-bar.
* Add data-ampdevmode attribute to any enqueued script that is flagged for dev mode or which depends on the admin-bar.
*
* @since 1.3
* @since 1.5
*
* @param string $tag The `<script>` tag for the enqueued script.
* @param string $handle The script's registered handle.
* @return string Tag.
*/
public static function filter_admin_bar_script_loader_tag( $tag, $handle ) {
if (
in_array( $handle, wp_scripts()->registered['admin-bar']->deps, true ) ?
self::is_exclusively_dependent( wp_scripts(), $handle, 'admin-bar' ) :
self::has_dependency( wp_scripts(), $handle, 'admin-bar' )
) {
public static function filter_script_loader_tag_for_dev_mode( $tag, $handle ) {
if ( self::dependency_needs_dev_mode( wp_scripts(), $handle ) ) {
$tag = preg_replace( '/(?<=<script)(?=\s|>)/i', ' ' . AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, $tag );
}
return $tag;
Expand Down Expand Up @@ -1972,7 +1990,7 @@ public static function filter_customize_partial_render( $partial ) {
'allow_dirty_styles' => true,
'allow_dirty_scripts' => false,
];
AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args ); // @todo Include script assets in response?
AMP_Content_Sanitizer::sanitize_document( $dom, self::get_sanitizer_classes(), $args ); // @todo Include script assets in response?
$partial = AMP_DOM_Utils::get_content_from_dom( $dom );
}
return $partial;
Expand Down Expand Up @@ -2082,6 +2100,8 @@ public static function prepare_response( $response, $args = [] ) {
$current_url = amp_get_current_url();
$non_amp_url = amp_remove_endpoint( $current_url );

$sanitizers = self::get_sanitizer_classes();

/*
* Set response cache hash, the data values dictates whether a new hash key should be generated or not.
* This is also used as the ETag.
Expand All @@ -2091,7 +2111,7 @@ public static function prepare_response( $response, $args = [] ) {
[
$args,
$response,
self::$sanitizer_classes,
$sanitizers,
self::$embed_handlers,
AMP__VERSION,
]
Expand Down Expand Up @@ -2255,7 +2275,7 @@ public static function prepare_response( $response, $args = [] ) {
$dom->documentElement->setAttribute( 'amp', '' );
}

$assets = AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args );
$assets = AMP_Content_Sanitizer::sanitize_document( $dom, $sanitizers, $args );

// Respond early with results if performing a validate request.
if ( AMP_Validation_Manager::$is_validate_request ) {
Expand Down Expand Up @@ -2467,23 +2487,26 @@ public static function setup_paired_browsing_client() {
$asset = require $asset_file;
$dependencies = $asset['dependencies'];
$version = $asset['version'];
$handle = 'amp-paired-browsing-client';

wp_enqueue_script(
'amp-paired-browsing-client',
$handle,
amp_get_asset_url( '/js/amp-paired-browsing-client.js' ),
$dependencies,
$version,
true
);
wp_script_add_data( $handle, 'ampdevmode', true );
foreach ( $dependencies as $dependency ) {
wp_script_add_data( $dependency, 'ampdevmode', true );
}

// Whitelist enqueued script for AMP dev mdoe so that it is not removed.
// @todo Revisit with <https://github.com/google/site-kit-wp/pull/505#discussion_r348683617>.
add_filter(
'script_loader_tag',
static function( $tag, $handle ) {
if ( is_amp_endpoint() && self::has_dependency( wp_scripts(), 'amp-paired-browsing-client', $handle ) ) {
$attrs = [ AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, 'async' ];
$tag = preg_replace( '/(?<=<script)(?=\s|>)/i', ' ' . implode( ' ', $attrs ), $tag );
static function( $tag, $filtered_handle ) use ( $handle ) {
if ( is_amp_endpoint() && self::has_dependency( wp_scripts(), $handle, $filtered_handle ) ) {
// Inject async attribute into script tag.
$tag = preg_replace( '/(<script[^>]*)(?=\ssrc=)/i', '$1 async ', $tag );
}
return $tag;
},
Expand Down
6 changes: 5 additions & 1 deletion tests/php/test-amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,10 @@ function ( $xpaths ) use ( $element_xpaths ) {
// Check that AMP_Dev_Mode_Sanitizer is registered once in dev mode, and now also with admin bar showing.
add_filter( 'amp_dev_mode_enabled', '__return_true' );
add_filter( 'show_admin_bar', '__return_true' );
$sanitizers = amp_get_content_sanitizers();
wp_localize_script( 'admin-bar', 'myAdminBarStrings', [ 'hello' => 'world' ] );
wp_scripts()->done[] = 'admin-bar';
wp_styles()->done[] = 'admin-bar';
$sanitizers = amp_get_content_sanitizers();
$this->assertTrue( is_admin_bar_showing() );
$this->assertTrue( amp_is_dev_mode() );
$this->assertArrayHasKey( 'AMP_Dev_Mode_Sanitizer', $sanitizers );
Expand All @@ -794,6 +797,7 @@ function ( $xpaths ) use ( $element_xpaths ) {
[
'//*[ @id = "wpadminbar" ]',
'//*[ @id = "wpadminbar" ]//*',
'//script[ not( @src ) ][ contains( text(), "var myAdminBarStrings =" ) ]',
'//style[ @id = "admin-bar-inline-css" ]',
]
),
Expand Down
Loading