Eliminate manual construction of `script` tags #58

westonruter · 2023-04-27T19:59:00Z

This eliminates manual construction of script tags from WP_Scripts methods. It replaces manual construction with wp_print_inline_script_tag(), wp_get_inline_script_tag(), and wp_get_script_tag() (which were added in WP 5.7). Doing so makes the code much more readable as well as more robust by automatically escaping attribute values and allowing the wp_script_attributes and wp_inline_script_attributes filters to apply to the attributes being printed. It also ensures the non-HTML5 CDATA wrapper comments are added consistently. This would seem to be a logical follow-up to Core-39941 which introduced these functions but didn't make use of them in WP_Scripts. This will facilitate adding CSP attributes to scripts that core prints. (I'm not sure why this wasn't done as part of the aforementioned Trac ticket.)

Update tests to account for changes in output (where double quotes are used instead of single, attribute order is changed, and CDATA wrappers are added when not in HTML5)

joemcgill · 2023-04-27T20:29:08Z

Thanks @westonruter, I'm a big fan of this idea but think it would be good to have time for discussion and feedback before deciding to make this change. Our plan is to wrap up the open feedback on #54 and then open a PR against the WordPress repo. Once we do, would you be willing to post a new comment on that PR with this suggestion (your PR to our fork/branch should still remain the same).

westonruter · 2023-04-27T21:26:05Z

@joemcgill You got it!

… of https://github.com/10up/wordpress-develop into update/print-script-tags * 'feature/enhance-wp-scripts-api-with-a-loading-strategy' of https://github.com/10up/wordpress-develop: Updated formatting of wpLoadAfterScripts js function to satisfy WP coding standards Update js formatting within wpLoadAfterScripts function used in test cases Update js formatting within wpLoadAfterScripts function Updated function body for wpLoadAfterScripts to reflect updated function body in script-loader.php Rewrite function body for wpLoadAfterScripts to be ES5 and not require the use of eval()

joemcgill · 2023-04-27T22:24:32Z

Thanks @westonruter. A new PR is now available for continued feedback/discussion here WordPress#4391

swissspidy · 2023-05-03T19:32:05Z

I'm not sure why this wasn't done as part of the aforementioned Trac ticket

That was a separate PR but also a massive undertaking. See WordPress#498

westonruter · 2023-05-03T20:11:15Z

@swissspidy Thanks for that. I think that PR perhaps went too far by also attempting to become fully compliant with CSP Strict. If the scope is reduced to just utilize the helper functions for script construction, then a subsequent PR could then add the scope of eliminating inline event handlers. (Nevertheless, WordPress#4391 currently relies on onload event handler attributes for detecting when async/defer scripts have loaded to evaluate after scripts, which would also violate Strict CSP unless unsafe-hashes were added.)

westonruter · 2023-05-04T21:24:16Z

src/wp-includes/class-wp-scripts.php

 			if ( 'after-non-standalone' === $position ) {
-				$script_output = "<script%1\$s id='%2\$s-js-after' data-wp-executes-after='%2\$s'>\n%4\$s\n</script>\n";
+				$attributes['id']                     = "{$handle}-js-after";
+				$attributes['data-wp-executes-after'] = $handle;


I mean:

Suggested change

$attributes['data-wp-executes-after'] = $handle;

$attributes['data-wp-executes-after'] = $handle;

$attributes['type'] = 'text/template';

WordPress#4391 (comment)

10upsimon

@westonruter (cc @joemcgill ) this looks solid to me, thank you. Approving, although it's in draft form I assume you're good to release it into non draft @westonruter ?

I was also wondering of this has implications for any tests we've written. It might as we'll be outputting loading strategy attributes as async="true" and defer="true" as opposed to just async or defer from what I can see, so as a spin off I'll probably need to update the tests where script tags are manually constructed to reference the script functions used herein.

If you feel like we should do this as part of this branch, I can pick that up herein.

westonruter · 2023-05-17T16:45:05Z

@10upsimon yes, I mainly left it as a draft because the tests haven't been updated yet. Also, there is that suggestion corresponding with a suggestion in the main PR that hasn't been applied yet.

So yeah, tests will need to be updated. Implications of this are:

If html5 support is not present, then the attributes will be rendered XHTML-compatible as async="async" and defer="defer".
Double-quoted attributes will be used instead of single-quoted ones.
The attribute order won't necessarily be the same as before.

These may complicate themes & plugins that are filtering script_loader_tag to mutate the HTML if using naive pattern matching. But they really should be updated to use WP_HTML_Tag_Processor now regardless. And with the introduction of the script loading strategies, they could eliminate use of the filter altogether. This will be important to include in the Dev Note.

Update: See #55 (comment) which further addresses the back-compat issues related to the clean_url filter.

westonruter · 2023-05-17T17:27:30Z

Aside: It's too bad that the GHA workflows aren't running on these intra-PR branches:

wordpress-develop/.github/workflows/phpunit-tests.yml

Lines 12 to 16 in 860d506

    
           pull_request: 
        
             branches: 
        
               - trunk 
        
               - '3.[7-9]' 
        
               - '[4-9].[0-9]'

joemcgill · 2023-05-17T18:46:27Z

Aside: It's too bad that the GHA workflows aren't running on these intra-PR branches

There is an open Trac ticket for this, because coming up with an official way for PRs into feature branches to be checked would be nice to have.

joemcgill · 2023-06-12T16:42:01Z

@westonruter is this something that you still intend on picking up, or did you determine that it was not feasible without backward compatibility breakage?

westonruter · 2023-06-12T16:56:36Z

@joemcgill The main hangup was updating all the tests, which now actually would be greatly simplified by assertEqualMarkup in our main PR (see #73). But I recall we decided to postpone this to 6.4 to avoid potential back-compat issues with this change being conflated with the introduction of script loading strategies.

spacedmonkey · 2023-06-19T11:57:38Z

@westonruter I think this change is a good, I think for the sake of testing, commit size and revertablity, lets do this in another ticket / commit.

westonruter · 2023-06-28T20:30:24Z

I've created a core ticket to move this forward in 6.4: https://core.trac.wordpress.org/ticket/58664

westonruter added 4 commits April 27, 2023 11:05

Rename type_attr variable to is_html5

43febec

Eliminte manual construction of script tags

9737542

Remove now-unused is_html5/type_attr member variable

10b0572

Fix printing async/defer strategy

1079fe5

westonruter mentioned this pull request Apr 27, 2023

DRAFT: PR for enhancing WP_Scripts with a loading strategy #54

Closed

joemcgill requested review from adamsilverstein, joemcgill and 10upsimon April 27, 2023 20:35

westonruter added 2 commits April 27, 2023 14:28

Consistently format single-item associative arrays

ee0abab

westonruter mentioned this pull request Apr 27, 2023

Enhancing the WP_Scripts API With a Loading Strategy WordPress/wordpress-develop#4391

Closed

westonruter commented May 4, 2023

View reviewed changes

10upsimon approved these changes May 17, 2023

View reviewed changes

westonruter marked this pull request as ready for review May 17, 2023 16:41

westonruter marked this pull request as draft May 19, 2023 21:02

westonruter mentioned this pull request May 19, 2023

Investigate Current Solutions for Deferred Scripts & Potential Interoperability Issues or Considerations #55

Closed

1 task

10upsimon self-requested a review May 25, 2023 12:39

westonruter closed this Jun 28, 2023

westonruter mentioned this pull request Jul 1, 2023

Eliminate manual construction of script tags in WP_Scripts and pass other scripts through wp_print_inline_script_tag() WordPress/wordpress-develop#4773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate manual construction of `script` tags #58

Eliminate manual construction of `script` tags #58

westonruter commented Apr 27, 2023 •

edited

Loading

joemcgill commented Apr 27, 2023

westonruter commented Apr 27, 2023

joemcgill commented Apr 27, 2023

swissspidy commented May 3, 2023

westonruter commented May 3, 2023

westonruter May 4, 2023 •

edited

Loading

westonruter May 4, 2023 •

edited

Loading

10upsimon left a comment

westonruter commented May 17, 2023 •

edited

Loading

westonruter commented May 17, 2023

joemcgill commented May 17, 2023

joemcgill commented Jun 12, 2023

westonruter commented Jun 12, 2023 •

edited

Loading

spacedmonkey commented Jun 19, 2023

westonruter commented Jun 28, 2023

	$attributes['data-wp-executes-after'] = $handle;
	$attributes['data-wp-executes-after'] = $handle;
	$attributes['type'] = 'text/template';

Eliminate manual construction of script tags #58

Eliminate manual construction of script tags #58

Conversation

westonruter commented Apr 27, 2023 • edited Loading

joemcgill commented Apr 27, 2023

westonruter commented Apr 27, 2023

joemcgill commented Apr 27, 2023

swissspidy commented May 3, 2023

westonruter commented May 3, 2023

westonruter May 4, 2023 • edited Loading

Choose a reason for hiding this comment

westonruter May 4, 2023 • edited Loading

Choose a reason for hiding this comment

10upsimon left a comment

Choose a reason for hiding this comment

westonruter commented May 17, 2023 • edited Loading

westonruter commented May 17, 2023

joemcgill commented May 17, 2023

joemcgill commented Jun 12, 2023

westonruter commented Jun 12, 2023 • edited Loading

spacedmonkey commented Jun 19, 2023

westonruter commented Jun 28, 2023

Eliminate manual construction of `script` tags #58

Eliminate manual construction of `script` tags #58

westonruter commented Apr 27, 2023 •

edited

Loading

westonruter May 4, 2023 •

edited

Loading

westonruter May 4, 2023 •

edited

Loading

westonruter commented May 17, 2023 •

edited

Loading

westonruter commented Jun 12, 2023 •

edited

Loading