WordPress.org

Make WordPress Core

Opened 14 months ago

Last modified 3 weeks ago

#51124 accepted feature request

Can we get an additional parameter in wp_add_inline_script to set the script type?

Reported by: hcabrera Owned by: audrasjb
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: good-first-bug needs-patch needs-unit-tests
Focuses: Cc:

Description

Hi everyone,

This is probably a feature that not many developers out there will use but I think it would be nice to have.

One of my plugins uses wp_add_inline_script to inject an inline script into the page. Said inline script gets assigned text/javascript as type automatically and there doesn't seem to be a way to change that (if there is please let me know and I'll gladly close this ticket.)

My plugin needs said inline script to contain a JSON string and so it currently hooks into script_loader_tag to change its type to application/json which feels a bit like a hack. It would be nice if we could set the script type via parameter (eg. wp_add_inline_script( 'some-handle', '...', 'before', 'application/json' );) (or maybe via filter hook before script_loader_tag happens?)

Thoughts?

Attachments (3)

51124.diff (2.3 KB) - added by byohann6 5 months ago.
Add an additionnal parameter to the wp_add_inline_script function
51124.2.diff (2.3 KB) - added by byohann6 5 months ago.
51124.3.diff (2.3 KB) - added by audrasjb 3 weeks ago.
Script Loader: Add a type parameter to the wp_add_inline_script and wp_add_inline_script functions.

Download all attachments as: .zip

Change History (17)

#1 @Otto42
13 months ago

  • Keywords good-first-bug needs-patch added

+1

This is a good idea. Additionally, the script tag can also be used for adding structured data using type="application/ld+json".

See here for more information: https://developers.google.com/search/docs/guides/intro-structured-data

Also, given that the text/javascript is the default for scripts, having type="text/javascript" there is not needed, and the latest W3 validation tool actually returns a warning for it with "The type attribute is unnecessary for JavaScript resources."

So it might be worth removing that default as well, and allowing for registered scripts to change the type via some data attached to the handle. Then only output the type attribute when it's explicitly set by the code.

#2 @GeekPress
9 months ago

Also, given that the text/javascript is the default for scripts, having type="text/javascript" there is not needed, and the latest W3 validation tool actually returns a warning for it with "The type attribute is unnecessary for JavaScript resources."

Hi @Otto42

The type="text/javascript" is removed if we set script to the HTML5 theme support

add_theme_support( 'html5', [ 'script' ] );

Are you suggesting to totally remove by default type="text/javascript"?

#3 @mikejdent
8 months ago

Last edited 8 months ago by mikejdent (previous) (diff)

#4 @mikejdent
8 months ago

  • Keywords needs-testing added

#5 @mikejdent
8 months ago

  • Keywords needs-testing removed

@byohann6
5 months ago

Add an additionnal parameter to the wp_add_inline_script function

#6 @byohann6
5 months ago

  • Keywords has-patch added; needs-patch removed

Hi,

I make a patch for this issue.
In my patch, I add an empty "$type" parameter to the wp_add_inline_script() function.

If we don't specify the type parameter the function add the value "text/javascript" if the theme doesn't declare

add_theme_support( 'html5', array('script') );

And if we specify the type parameter, the default value is overwritten.

This is my first patch, I'm open to discussion

#7 @audrasjb
5 months ago

Hi @byohann6 congrats on your first patch!

The logic looks good to me at a glance, however there is a minor coding standards issue on src/wp-includes/functions.wp-scripts.php, line 131: ther's a missing space before the closing parenthesis :)

#8 @audrasjb
5 months ago

Also, you may want to esc_attr() the $type variable ;)

@byohann6
5 months ago

#9 @byohann6
5 months ago

Thank you @audrasjb

I updated my patch respecting your last remarks.

#10 @audrasjb
5 months ago

  • Milestone changed from Awaiting Review to 5.8
  • Owner set to audrasjb
  • Status changed from new to accepted

Thank you for the update. All looks good to me.
Moving for 5.8 consideration.

Please note that while I think it's a nice enhancement and the patch looks good and pretty straightforward, it's not 100% sure it will ship in 5.8.
Indeed, 5.8 feature freeze is next week. Let's see if it can ship or not.

#11 @desrosj
5 months ago

  • Milestone changed from 5.8 to 5.9

Unfortunately this one ran out of time. I'm punting to 5.9 as there has been good recent momentum. We should be able to get this reviewed and committed for the next release!

@audrasjb
3 weeks ago

Script Loader: Add a type parameter to the wp_add_inline_script and wp_add_inline_script functions.

#12 @audrasjb
3 weeks ago

Patch refreshed against trunk.

@azaozz as you are the Script Loader component maintainer, are you available to review the proposed patch? Thank you very much :)

#13 @azaozz
3 weeks ago

  • Keywords needs-patch has-unit-tests added; has-patch removed

Looking at the patches, it's not going to be that easy :)

To add support for type attribute to each inline script the requirements are:

  1. The attribute has to be set only for that script tag. With the current patches it is set "globally" so everything that uses $this->type_attr in WP_Scripts will use the new/special type. A solution would be to add another data on the script handle holding the type.
  1. There can be more than one inline script for each "script handle". To support custom types, each inline script should be able to have it's own type (if set), or fall back to the default, global $this->type_attr. So the above mentioned data on the script handle should be an array corresponding to the number of inline scripts that have custom types set.
  1. As this is getting pretty complex, unit tests are a must.

I've looked a bit at how this may work and there's another "gotcha", unfortunately. The public function print_inline_script( $handle, $position = 'after', $echo = true ) returns the concatenated inline script sources. Not sure what to do if one of the inline scripts has a custom type. Seems it will have to be excluded from the concatenated scripts, as it is not JavaScript, but then it gets "lost" if a function expects to use the return value ($echo is false). Any ideas on how to handle that case?

#14 @azaozz
3 weeks ago

  • Keywords needs-unit-tests added; has-unit-tests removed
Note: See TracTickets for help on using tickets.