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

Let whitelist sanitizer dictate the required AMP scripts via spec #882

Merged
merged 5 commits into from
Jan 19, 2018

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jan 19, 2018

  • Include requires_extension and also_requires_tag_warning from spec in AMP_Allowed_Tags_Generated so that the required AMP components can be determined for a given tag spec and returned via AMP_Tag_And_Attribute_Sanitizer::get_scripts().
  • Use latest versions of AMP component scripts instead of 0.1. (Discussed with @amedina.)
  • Eliminate the get_scripts() method implementations from all of the sanitizers since all of the scripts now identified by whitelist sanitizer.
  • Remove temporary duplicated/incompleted/brittle AMP script component lookups in AMP_Theme_Support::get_amp_component_scripts().

With the changes here, you can just drop in AMP components to your post content (or to the theme templates w/ theme support) and AMP component scripts will automatically be added for them. For example, try pasting the following into a new post:

<amp-gist
    data-gistid="b9bb35bc68df68259af94430f012425f"
    layout="fixed-height"
    height="225">
</amp-gist>

  <amp-ad type="a9"
    width="300"
    height="250"
    data-aax_size="300x250"
    data-aax_pubname="test123"
    data-aax_src="302">
  </amp-ad>

Automatically you'll then see these scripts added to the head:

  • <script custom-element="amp-video" src="https://cdn.ampproject.org/v0/amp-video-latest.js" async></script>
  • <script custom-element="amp-gist" src="https://cdn.ampproject.org/v0/amp-gist-latest.js" async></script>

See #875.

* Use spec to determine which script components get enqueued
* Use the latest component instead of 0.1
* Reduce duplicated logic by defining get_scripts() method on base sanitizer
* Add get_allowed_tag_data method on AMP_Allowed_Tags_Generated to reduce size of array passing.
* Define sanitized_tag class variable on sanitizer classes
@westonruter westonruter added this to the v0.7 milestone Jan 19, 2018
Include also_requires_tag_warning in AMP_Allowed_Tags_Generated
Now the scripts are obtained via the whitelist sanitizer alone, so there is no need for the redundancy of obtaining the component scripts via the individual sanitizers or to duplicate the component lookup via a get_allowed_tag_data method.
@westonruter westonruter changed the title WIP: Let whitelist sanitizer dictate the required AMP scripts via spec Let whitelist sanitizer dictate the required AMP scripts via spec Jan 19, 2018
@ThierryA
Copy link
Collaborator

@westonruter how about running the embeds through the AMP_Base_Sanitizer class that we can suppress the embeds get_scripts()?

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.

Awesome work on this, bang on! I am merging this so that @DavidCramer can use it in #871.

This comment may be implemented in another PR if we decide to go with it.

@ThierryA ThierryA merged commit 89e4227 into develop Jan 19, 2018
@ThierryA ThierryA deleted the update/amp-component-including branch January 19, 2018 14:21
@DavidCramer
Copy link
Contributor

Sweet!

@westonruter
Copy link
Member Author

how about running the embeds through the AMP_Base_Sanitizer class that we can suppress the embeds get_scripts()?

Excellent point. The embeds don't need to implement get_scripts() either.

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.

3 participants