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

Normalize 'ver' query param in script/style validation errors to prevent recurrence after accepted #1346

Merged
merged 2 commits into from Aug 19, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

westonruter commented Aug 19, 2018

A validation error for an enqueued script can look like this:

{
    "code": "invalid_element",
    "node_attributes": {
        "src": "https://src.wordpress-develop.test/wp-content/plugins/jetpack/modules/widgets/google-translate/google-translate.js?ver=4.9.7-alpha-43298-src",
        "type": "text/javascript"
    },
    "node_name": "script",
    "parent_name": "body"
}

In this case for the Google Translate widget in Jetpack, the script is enqueued without any ver and so it will re-use whatever the current WordPress version is. If this validation error gets marked as accepted, then the next time that core is updated, a brand new validation error will be reported because the ver change causes the fingerprint (md5 hash) of the validation error to change. This problem would also apply when the a plugin supplies a ver that corresponds to the plugin's actual version, as each time the plugin is updated any accepted validation errors for its scripts will then need to be re-accepted.

While a amp_validation_error filter could be used to strip the ver query param from the URLs in such validation errors, this normalization seems like something that should be the default behavior in the plugin. This plugin will replace any ver query param values with __normalized__.

@westonruter westonruter added this to the v1.0 milestone Aug 19, 2018

@westonruter westonruter requested a review from amedina Aug 19, 2018

@hellofromtonya

This comment has been minimized.

Copy link
Contributor

hellofromtonya commented Aug 19, 2018

Excellent catch @westonruter. You’re right. That will be a problem. To solve it, you are suggesting that we replace the variable value of the ver query param. That’s a solid solution.

There’s another edge case: when the version is made part of the URL as a directory or within the file name itself.

For this one, I don’t think there’s anything we can do. Why? There’s not a consistent pattern to parse for us to differentiate a valid URL vs. embedded version. Regardless, I wanted to note it for discussion.

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Aug 19, 2018

@hellofromtonya That's good to note. That would be a problem in the Gutenberg edit post screen which includes asset URLs such as:

<script type='text/javascript' src='https://src.wordpress-develop.test/wp-content/plugins/gutenberg/vendor/react.832d746b.js'></script>
<script type='text/javascript' src='https://src.wordpress-develop.test/wp-content/plugins/gutenberg/vendor/react-dom.24169eaf.js'></script>
<script type='text/javascript' src='https://src.wordpress-develop.test/wp-content/plugins/gutenberg/vendor/lodash.2cfc2504.js'></script>

However, in the case of Gutenberg at least, these aren't ever enqueued on the frontend and so it wouldn't be a concern per se. It would be problematic if another plugin tried to enqueue them. But we could address that at a later time.

@hellofromtonya

This comment has been minimized.

Copy link
Contributor

hellofromtonya commented Aug 19, 2018

Right. Plus ones that enqueue:

https://example.com/assets/v1/scripts/some-script.js

@westonruter westonruter merged commit 274971c into develop Aug 19, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/normalize-validation-error-ver-query-params branch Aug 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.