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

Add content embedding & sanitization to theme support w/ filterable component scripts and custom styles #865

Merged
merged 8 commits into from
Jan 17, 2018

Conversation

westonruter
Copy link
Member

  • Creates new helper functions for amp_get_content_embed_handlers() and amp_get_content_sanitizers() which are used now in both the plugin's paired mode post templates as well as when theme support is present. The $post argument is not passed to the amp_content_embed_handlers and amp_content_sanitizers filters when theme support is present since the content being sanitized/embedded may not be a post at all, e.g. a Text widget.
  • It is intended that soon the \AMP_Theme_Support::filter_the_content() method could be expanded to also apply to widget output.
  • Scripts and styles that are gathered during embedding and sanitizing are now injected into the head once output buffering is finalized.
  • Scripts and styles which are injected into the head can be filtered via amp_component_scripts and amp_custom_styles. The former should normally not be needed because the plugin should be able to detect all component scripts required. The styles filter will be useful when a theme, plugin, widget, etc wants to add additional styles, such as for the Recent Comments widget (cf. Add styles for Recent Comments widget xwp/ampnews#28).
  • When WP_DEBUG is on, validation is performed on the amp theme support args, warning the user if any unrecognized args were provided.
  • The \AMP_DOM_Utils::get_dom_from_content() method has been updated to parse HTML with the actual blog_charset. This is important for eventually supporting non-UTF8 sites. See Support WordPress installs with non-UTF-8 charsets #855.

@westonruter westonruter added this to the v0.7 milestone Jan 16, 2018
Copy link
Collaborator

@ThierryA ThierryA left a comment

Choose a reason for hiding this comment

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

Great work here. Find the CR below.


// @todo Grab source of all enqueued styles and concatenate here?
// @todo Print contents of get_locale_stylesheet_uri()?
// @todo Allow this to be filtered after output buffering is complete so additional styles can be added by widgets and other components just-in-time?
$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.
echo wp_strip_all_tags( $css ); // WPCS: XSS 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 believe this should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for it is that is that tag stripping is included in Custom CSS in core to protect against malicious CSS: https://github.com/WordPress/wordpress-develop/blob/9dd8b5a2102a197e236fc05de2e47e85c03a8bd9/src/wp-includes/theme.php#L1687

At the very least we should wrap the call to wp_get_custom_css() with wp_strip_all_tags().

);
}

foreach ( $amp_components as $component => $props ) {
if ( preg_match( '#<(form|input)\b#i', $html ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason to call this in the loop, this could wrap around the loop which would save memory?

Copy link
Member Author

@westonruter westonruter Jan 16, 2018

Choose a reason for hiding this comment

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

Oops, my logic here is bad. Fixed in 745a3db.

@westonruter
Copy link
Member Author

Restarting builds…

@ThierryA
Copy link
Collaborator

ThierryA commented Jan 16, 2018

I restarted the builds a few times but they seem to be stuck. Pushed a blank commit to try triggering the builds again but that doesn't seem to work neither. It seems like there is a backlog on Travis side https://www.traviscistatus.com/
I subscribed to the incident to get notified when it is resolved.

@ThierryA ThierryA merged commit e60cb15 into develop Jan 17, 2018
@ThierryA ThierryA deleted the add/valid-canonical-content branch January 17, 2018 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants