Skip to content

wp_get_script_tag() and wp_get_inline_script_tag()#591

Closed
enricocarraro wants to merge 4 commits intoWordPress:masterfrom
enricocarraro:wp_script_functions
Closed

wp_get_script_tag() and wp_get_inline_script_tag()#591
enricocarraro wants to merge 4 commits intoWordPress:masterfrom
enricocarraro:wp_script_functions

Conversation

@enricocarraro
Copy link

I introduced two new functions that print <script> tags and enable attribute injection:

  • wp_print_script_tag and wp_get_script_tag: for script tags that load JavaScript files through the src attribute;
  • wp_print_inline_script_tag and wp_get_inline_script_tag: for inline scripts.
    All these functions filter the attributes passed to them through wp_script_attributes so that plugins can change script attributes in a controlled manner.

Instead of directly printing <script> tags, these functions should be used to ensure that every <script> tag is controllable.

Trac ticket: https://core.trac.wordpress.org/ticket/39941

@adamsilverstein
Copy link
Member

@enricocarraro Thank you for working on this!

I left a few comments and questions. It would be good explain what the filters do in their inline docs, and also to add a test that covers the wp_get_inline_script_tag action (the test could add add an action and verify it was called with the correct data).

Since this is a bit complex to use it will definitely need a "Dev Note" that explains how the API can be used, including stuff like this: https://github.com/WordPress/wordpress-develop/pull/498/files#r499066268.

Would you say merging this PR fixes https://core.trac.wordpress.org/ticket/39941?

@enricocarraro
Copy link
Author

@adamsilverstein thank you for taking the time to look at this.

I left a few comments and questions. It would be good explain what the filters do in their inline docs, and also to add a test that covers the wp_get_inline_script_tag action (the test could add add an action and verify it was called with the correct data).

I will shortly add tests to cover wp_get_inline_script_tag action.

Since this is a bit complex to use it will definitely need a "Dev Note" that explains how the API can be used, including stuff like this: https://github.com/WordPress/wordpress-develop/pull/498/files#r499066268.

Will look into it.

Would you say merging this PR fixes https://core.trac.wordpress.org/ticket/39941?

I wouldn't say so; "allowing a Content-Security-Policy without unsafe-inline" would require all script tags to have a nonce, and not having inline event handlers and javascript: URIs.

@enricocarraro enricocarraro force-pushed the wp_script_functions branch 2 times, most recently from 00059ca to 89cb5b0 Compare October 16, 2020 10:54
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Looks solid overall, I've added some feedback.

@enricocarraro
Copy link
Author

Thanks, @felixarntz.
I made the changes you suggested. Please, let me know if I can do anything else.

@enricocarraro enricocarraro force-pushed the wp_script_functions branch 2 times, most recently from 4ec95bd to bf697d7 Compare October 20, 2020 08:16
Copy link
Member

@ocean90 ocean90 left a comment

Choose a reason for hiding this comment

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

@enricocarraro
Copy link
Author

Thanks for suggesting the changes @ocean90.
I implemented most of the changes you requested and replied to the comment about combining ifs conditions.

@enricocarraro enricocarraro requested a review from ocean90 October 23, 2020 13:57
@hellofromtonya
Copy link
Contributor

Closed with changeset https://core.trac.wordpress.org/changeset/50167

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.

5 participants