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

Ensure only remaining AMP scripts are printed when preparing response #1097

Merged
merged 2 commits into from
Apr 26, 2018

Conversation

westonruter
Copy link
Member

This fixes a bug exposed when testing the plugin with W3 Total Cache. That plugin prints out a script at the wp_print_scripts action which is triggered when calling wp_print_scripts(). We can avoid this duplication of script printing by directly calling wp_scripts()->do_items() instead of wp_print_scripts() which is essentially a wrapper for the former. The being printed here are the AMP component scripts that were discovered during the sanitization process. As such these scripts are run after sanitization has been completed, thus it is important that only the remaining AMP scripts be output here. The changes here ensure this.

@westonruter westonruter added this to the v0.7 milestone Apr 26, 2018
@westonruter westonruter changed the base branch from develop to 0.7 April 26, 2018 04:14
@westonruter westonruter removed the request for review from amedina April 26, 2018 04:40
… AMP scripts

This is to guard against anything AMP-invalid from being output since remaining scripts are printed after the sanitizer has finished
Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

Excellent. Ship it!

@westonruter westonruter merged commit dcb882f into 0.7 Apr 26, 2018
@westonruter westonruter deleted the fix/print-remaining-scripts branch April 26, 2018 14:39
@kienstra
Copy link
Contributor

kienstra commented Apr 26, 2018

Looks Good, Works As Expected

Hi @westonruter,
Sorry for the delay here. To second @amedina's point, this looks good.

I also saw the issue of W3 Total Cache outputting that <script>.

But with this PR applied, that script doesn't appear. And as expected, the component scripts still appear, like https://cdn.ampproject.org/v0/amp-bind-latest.js

disallowed-script-cache

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