Add text template #49

kt-12 · 2023-03-10T18:39:56Z

Trac ticket:

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

10upsimon

@kt-12 this is looking good. I am approving as I did not see the missing docblocks for test data providers as a total blocker, but if you can add them that would be great!

…nt_load_handlers

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

joemcgill

I found a couple of things here that I think we need to address before merging.

joemcgill · 2023-03-30T18:08:32Z

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

+			$after_non_standalone_handle = $this->print_inline_script( $handle, 'after-non-standalone', false );
+
+			if ( $after_non_standalone_handle ) {
+				$after_handle .= sprintf( "<script%s id='%s-js-after' type='text/template'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), $after_non_standalone_handle );


This approach seems wrong to me. Shouldn't we instead change the value of $this->type_attr rather than hard coding a second type attribute onto the script? You can even see in the unit tests below that the scripts are getting two type attributes applied to them. Alternately, if we don't want to update the main type_attr property on this class, we should at least remove it when we are hard coding a text/template type.

Thanks for the catch @joemcgill. Totally my fault here, shouldn't have slipped under my radar. I was focusing on the condition and getting text/template, I forgot to see it was injecting text/javascript too.

Fixed now.

joemcgill · 2023-03-30T18:14:06Z

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

@@ -482,7 +498,11 @@ public function print_inline_script( $handle, $position = 'after', $display = tr
 		$output = trim( implode( "\n", $output ), "\n" );

 		if ( $display ) {
-			printf( "<script%s id='%s-js-%s'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), esc_attr( $position ), $output );
+			if ( 'after-non-standalone' === $position ) {
+				printf( "<script%s id='%s-js-after' type='text/template'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), esc_attr( $position ), $output );


Since we're hard coding the position in the ID here, I think we also need to remove the $position variable that's being passed to the printf function. We should probably add a unit test to cover this case as well.

Suggested change

printf( "<script%s id='%s-js-after' type='text/template'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), esc_attr( $position ), $output );

printf( "<script%s id='%s-js-after' type='text/template'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), $output );

Thanks @joemcgill. This is fixed now.

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

…nt_load_handlers

joemcgill

Thanks @kt-12, this is looking better now.

10upsimon

@kt-12 I did another round of review on this and it 's looking good to me. Good catch on some of those issues @joemcgill thank you!

Implement load handlers

kt-12 added 13 commits March 10, 2023 18:33

standalone and non-standalone

dcd2a32

phpcs fixes

8d33fac

set defaults

e4b461d

update print_inline_script for stand alone

8421a1c

fix issue with standalone appending

371e42f

bug fix incorrect strategy check

c517bc1

numbered attributes

04aabf5

onload script

e366d5d

fix bug

0fafd88

sprintf issue with \n

386022e

phpcs fixes

fe7acc0

print template loader script

394405f

\n to EOL

cc5dace

kt-12 added this to the Milestone 3 milestone Mar 11, 2023

kt-12 linked an issue Mar 11, 2023 that may be closed by this pull request

Update the wp_add_inline_script function to ensure the inline scripts specify a type="text/template" attribute for inline scripts that are associated with deferred, when applicable #18

Closed

1 task

kt-12 added 2 commits March 13, 2023 15:52

Loading the script before other script

b558f34

temp commit

f04a246

kt-12 self-assigned this Mar 14, 2023

kt-12 added the Script Loading Strategy Issues relating to the Script Loading Strategy (WPP) label Mar 14, 2023

kt-12 added 11 commits March 15, 2023 01:41

check if any delayed inline scripts

b5f4df4

non-stanadalone after script test cases

e0e560e

test standalone and non-standalone script combined

c856372

phpcs fix

ff25567

for consistency

b2d6ec1

Merge add_text_template to implement_load_handlers

d9a0f39

add new line

d110556

backward compatibility

b6b7022

update test utility and scripts.

9e8c378

php cs fixes

ffc40c1

fix csfix issue

8af918c

10upsimon self-requested a review March 22, 2023 12:07

10upsimon approved these changes Mar 22, 2023

View reviewed changes

kt-12 and others added 5 commits March 22, 2023 12:55

Merge branch 'enhancement/add_text_template' into enhancement/impleme…

6187dd1

…nt_load_handlers

avoid removal of jquery libraries

1321d6f

Dataprovider doc block.

66ab60c

doc block change

bbdad37

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

966be0c

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

joemcgill requested changes Mar 30, 2023

View reviewed changes

kt-12 and others added 4 commits March 31, 2023 08:42

Update doc text

dcbf0af

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

bug: multiple type in a single script.

01c890e

print_display

9aaca52

Merge branch 'enhancement/add_text_template' into enhancement/impleme…

8e08454

…nt_load_handlers

kt-12 requested review from joemcgill and 10upsimon March 31, 2023 08:59

kt-12 added 6 commits March 31, 2023 11:09

fix type_attr no there issue

b3f7225

adding description

4df5288

broke standalone script test to multiple function

264853c

non-standalone after script test broken

556ae91

before standalone test broken

d4be88d

non standalone with before and defer

180a6ae

joemcgill approved these changes Mar 31, 2023

View reviewed changes

kt-12 added 3 commits March 31, 2023 14:34

used function instead of arrow function.

4d0140d

single condition

a5b83a4

put back handle

0269128

Base automatically changed from enhancement/update-wp-add-inline-script-standalone to enhancement/wp-add-inline-script-standalone April 1, 2023 00:43

10upsimon approved these changes Apr 3, 2023

View reviewed changes

Base automatically changed from enhancement/wp-add-inline-script-standalone to enhancement/wp-script-api-strategy-base April 3, 2023 05:43

Merge pull request #50 from 10up/enhancement/implement_load_handlers

4cc2502

Implement load handlers

10upsimon merged commit 0007785 into enhancement/wp-script-api-strategy-base Apr 3, 2023

10upsimon deleted the enhancement/add_text_template branch April 3, 2023 05:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add text template #49

Add text template #49

kt-12 commented Mar 10, 2023

10upsimon left a comment

joemcgill left a comment

joemcgill Mar 30, 2023

kt-12 Mar 31, 2023 •

edited

Loading

joemcgill Mar 30, 2023

kt-12 Mar 31, 2023

joemcgill left a comment

10upsimon left a comment

	printf( "<script%s id='%s-js-after' type='text/template'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), esc_attr( $position ), $output );
	printf( "<script%s id='%s-js-after' type='text/template'>\n%s\n</script>\n", $this->type_attr, esc_attr( $handle ), $output );

Add text template #49

Add text template #49

Conversation

kt-12 commented Mar 10, 2023

10upsimon left a comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill Mar 30, 2023

Choose a reason for hiding this comment

kt-12 Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

joemcgill Mar 30, 2023

Choose a reason for hiding this comment

kt-12 Mar 31, 2023

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

10upsimon left a comment

Choose a reason for hiding this comment

kt-12 Mar 31, 2023 •

edited

Loading