WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 4 months 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 has-patch
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 (2)

51124.diff (2.3 KB) - added by byohann6 4 months ago.
Add an additionnal parameter to the wp_add_inline_script function
51124.2.diff (2.3 KB) - added by byohann6 4 months ago.

Download all attachments as: .zip

Change History (13)

#1 @Otto42
12 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
8 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
7 months ago

I think using a parameter rather than filter makes more sense, as you specify the type as you are writing the script. I have written a patch to address this but just need to get the test suite built in npm so I can check it is working.

My thinking is, pass a $type parameter into add_inline_script() and WP_Scripts::add_inline_script() then if that parameter exists, assign it to WP_Scripts::type_attr
If it isn't set, then we revert to the default setting of 'text/javascript' unless we are using add_theme_support( 'html5', [ 'script' ] );

@GeekPress html5 has been around a while now. I wonder if we should make it default, and require developers to actively remove_theme_support( 'html5', [ 'script' ] ); if needs be?

Version 0, edited 7 months ago by mikejdent (next)

#4 @mikejdent
7 months ago

  • Keywords needs-testing added

#5 @mikejdent
7 months ago

  • Keywords needs-testing removed

@byohann6
4 months ago

Add an additionnal parameter to the wp_add_inline_script function

#6 @byohann6
4 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
4 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
4 months ago

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

@byohann6
4 months ago

#9 @byohann6
4 months ago

Thank you @audrasjb

I updated my patch respecting your last remarks.

#10 @audrasjb
4 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
4 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!

Note: See TracTickets for help on using tickets.