Make WordPress Core

Opened 4 years ago

Last modified 2 years ago

#51124 accepted feature request

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

Reported by: hcabrera's profile hcabrera Owned by: audrasjb's profile audrasjb
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: needs-patch needs-unit-tests early
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 3 years ago.
Add an additionnal parameter to the wp_add_inline_script function
51124.2.diff (2.3 KB) - added by byohann6 3 years ago.
51124.3.diff (2.3 KB) - added by audrasjb 2 years 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 (20)

#1 @Otto42
4 years 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
3 years 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
3 years 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 3 years ago by mikejdent (next)

#4 @mikejdent
3 years ago

  • Keywords needs-testing added

#5 @mikejdent
3 years ago

  • Keywords needs-testing removed

@byohann6
3 years ago

Add an additionnal parameter to the wp_add_inline_script function

#6 @byohann6
3 years 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
3 years 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
3 years ago

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

@byohann6
3 years ago

#9 @byohann6
3 years ago

Thank you @audrasjb

I updated my patch respecting your last remarks.

#10 @audrasjb
3 years 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
3 years 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
2 years ago

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

#12 @audrasjb
2 years 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
2 years 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
2 years ago

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

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#16 @audrasjb
2 years ago

  • Keywords early added; good-first-bug removed
  • Milestone changed from 5.9 to Future Release

As per today's bug scrub and comment 29, this ticket is not ready for shipping in 5.9.
Moving to Future Release with early keyword.

#17 @galbaras
2 years ago

Seems very related to https://core.trac.wordpress.org/ticket/40276.

Personally, I need to add attributes like data-cfasync='false', and I can't find a way to do this with the current version of wp_add_inline_script() (or wp_localize_script(), for that matter).

The following solutions will give developers absolute freedom:

  1. Allow the script tag to be passed along with the code (see https://core.trac.wordpress.org/ticket/43403)
  2. Add a filter on the script tag
  3. Add an array parameter (e.g. [ 'data-cfasync' => 'false' ]) and expand it into arbitrary script tag attributes (not just type)
Note: See TracTickets for help on using tickets.