Add source information for script/style dependencies and inline scripts/styles#4135
Add source information for script/style dependencies and inline scripts/styles#4135westonruter merged 22 commits intodevelopfrom
Conversation
|
The
This is fine because it comes from core, but technically it could have the current theme attributed as a source because |
…e which enqueue Add support for wrapping callbacks for functions that pass the first argument by reference (e.g. wp_default_scripts)
| esc_html( basename( wp_parse_url( $validation_error['node_attributes']['src'], PHP_URL_PATH ) ) ) | ||
| ); | ||
| } else { | ||
| $title = esc_html__( 'Invalid inline script', 'amp' ); |
There was a problem hiding this comment.
Would be nice to add the handle (or basename) that the inline script is for.
This comment has been minimized.
This comment has been minimized.
| flush_rewrite_rules( false ); | ||
| } | ||
|
|
||
| // The plugins_loaded action is the earliest we can run this since that is when pluggable.php has been required and wp_hash() is available. |
There was a problem hiding this comment.
Why not replace wp_hash with something else, then? It doesn't need to be cryptographically secure, so any type of hash will work, really.
There was a problem hiding this comment.
I think it would even make sense to include a hash function in the amp/common library, so that we have consistent hashing across all libraries.
There was a problem hiding this comment.
Good question. It's not just purely a hash that we need. It's hashing using one of the WordPress secrets, which entails using wp_salt() which is in turn another pluggable function. Then the input for hashing is wp_nonce_tick() which is yet another pluggable function. So these three functions all require this to be run at plugins_loaded and I believe we need to depend on them specifically.
amp-wp/includes/validation/class-amp-validation-manager.php
Lines 1773 to 1775 in 1c3c853
…ued-dependency-sourcing * 'develop' of github.com:ampproject/amp-wp: (42 commits) Use static closure since $this is not needed Revert change in some tests, as they're not needed anymore Use suggested normalization, update tests Add a new method to normalize URLs Remove test case that is no longer handled Move urldecode() lower in the function Revert change to regex, in favor of trimming before it's passed Fix some phpcs errors, mainly => alignment Align => operators, from a recent commit Add a since tag to the new filter amp_to_amp_excluded_urls Commit Weston's suggestion to prevent conditional in Standard mode Handle case of something like #heading at end of URL Rename parameter 'excluded_amp_links' to 'excluded_urls' Change filter name to 'amp_to_amp_excluded_urls' Change DocBlock of 'amp_to_amp_excluded_links' As there's now a capturing group, remove lookahead Replace 2 calls of trim() with longer regex in parse_protocol() Commit Weston's suggestion for the Reader mode template Allow a URL to have a leading space without a validation error For WordPress.tv embed, Use an oembed filter instead of block filter (#4164) ...
|
Todo:
|
Done in e54bb61. Before: After: |
…ued-dependency-sourcing * 'develop' of github.com:ampproject/amp-wp: (124 commits) Copy patches folder when building Re-run amphtml-update.sh to remove duplicate Use travis_retry on PHPUnit for external-http tests (#4298) Improve display of anyof/oneof attributes in details Add error code for DUPLICATE_ONEOF_ATTRS; add error messages Sort the attributes from GetMandatoryOf() Simplify amphtml-update.py, using Weston's suggestiony Improve conversion of resizable iframes and those containing placeholder/overflow Add tests Update patch Bump CSS cache group Add patch from external PR that resolves the issue In amphtml-update.py, remove extra spaces inside function Rename method to get_unsatisfied_number_of_rule() Add more unit tests, including allowed mandatory_*of Update the PHP logic for the changed mandator_*of in the spec Move the mandatory_*of rule to the 'tag_spec' Bump stable tag to 1.4.3 Update readme and screenshots for Stories removal (#4259) Update dependency @wordpress/core-data to v2.12.0 (#4265) ...
1173e2f to
0861035
Compare
…ued-dependency-sourcing * 'develop' of github.com:ampproject/amp-wp: (38 commits) Revert "Use esc_html_e() rather than echoing esc_html()" Use esc_html_e() rather than echoing esc_html() Use more specific labels for MISSING_REQUIRED_PROPERTY_VALUE error Fix phpdoc Test robustness of AMP_Core_Theme_Sanitizer::guess_modal_role Add missing phpdocs for data providers Use `has_filter` function Make modal role guessing more robust Improve validation error details for invalid properties; improve tests Remove required-property logic from meta sanitizer and flesh out in tag and attribute sanitizer Handle case where multiple meta properties are invalid Only update attribute if properties changed Ignore code coverage for deprecated classes and methods Skip sending Content-Type header if headers already sent Make phpcs happy Rename 'property_value' to 'meta_property_value' Enfore UTF-8 content-type header Add more tests for AMP_Style_Sanitizer Add tests for AMP_Nav_Menu_Dropdown_Sanitizer Add tests for AMP_Customizer_Design_Settings ...
…tion_Callback_Wrapper
|
With the
The |
|
@pierlon I think that is expected because |
|
The patch for the |
Co-Authored-By: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
How do you recommend doing this? We don't directly have |
|
The patch being applied by Index: composer.json
===================================================================
--- composer.json (revision 6059817247734c2e7c1f15c39450e152100b0e6a)
+++ composer.json (date 1581998688688)
@@ -44,6 +44,13 @@
"Fix parsing CSS selectors which contain commas <https://github.com/sabberworm/PHP-CSS-Parser/pull/138>": "https://github.com/sabberworm/PHP-CSS-Parser/commit/fa139f65c5b098ae652c970b25e6eb03fc495eb4.diff",
"Validate name-start code points for identifier <https://github.com/sabberworm/PHP-CSS-Parser/pull/185>": "patches/php-css-parser-pull-185.patch"
}
+ },
+ "patches-ignore": {
+ "wp-cli/wp-cli": {
+ "mustache/mustache": {
+ "Patch no longer relevant as of mustache/mustache v2.13.0": "https://patch-diff.githubusercontent.com/raw/bobthecow/mustache.php/pull/349.patch"
+ }
+ }
}
},
"autoload": { |
Summary
Fixes #4134
Fixes #4133
wp-includesentries from appearing as the validation error source. The handle of the dependent script/style is also listed among the sources.wp_localize_script()orwp_add_inline_script(). The source stack also includes the specific inline script that was added.wp_add_inline_style()are now attributed to theme/plugin sources for the first time. As with scripts, all plugins and theme(s) that contribute to a given inline script will now be listed among the sources.wp_enqueue_scriptswill now be properly attributed. This includes styles registered atwp_default_stylesand scripts atwp_default_scripts.Testing
wp-content/pluginsdirectory. These are separate plugins so they must be added as separate files in thepluginsdirectory. Then dowp plugin activate add-inline-scripts add-more-inline-scripts.enqueue-assets-with-core-dependenciesand activate viawp plugin activate enqueue-assets-with-core-dependencies.wp-content/pluginswith the directory name ofadd-assets-before-enqueue-scripts. Then dowp plugin activate add-assets-before-enqueue-scripts.Screenshots
Compare before and after:
The stylesheets have the same sourcing:
The source stack now has information about how a given inline script was added, either via
wp_add_inline_script()orwp_localize_script(). For example:If I activate a theme (like Hestia) that also has scripts that depend on jQuery, then I see both the plugin and the theme as sources:
Todo
AMP_Validation_Callback_Wrapper.AMP_Validation_Manager::locate_sources()dlitems? At least linkify the URL in the<link>original markup.