-
Notifications
You must be signed in to change notification settings - Fork 385
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 ampdevmode data flag on script/style dependencies #4001
base: develop
Are you sure you want to change the base?
Conversation
c4c4274
to
21c71b0
Compare
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 ); | ||
} | ||
} |
There was a problem hiding this comment.
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 ) ); | |
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theDom\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.
6f13702
to
9db65fc
Compare
Summary
Fixes #4598.
Todo
done
handles to theamp_dev_mode_element_xpaths
filter when the template is rendered after the sanitizer args are collected.Checklist