WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 months ago

Last modified 5 months ago

#14853 closed enhancement (fixed)

wp_add_inline_script()

Reported by: mattwiebe Owned by: swissspidy
Milestone: 4.5 Priority: normal
Severity: normal Version: 3.1
Component: Script Loader Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

There should be actions that fire after a script is printed. This would enabled this best-practices usage scenario (from http://github.com/paulirish/html5-boilerplate/blob/master/index.html ):

<!-- Grab Google CDN's jQuery. fall back to local if necessary -->
  <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script>
  <script>!window.jQuery && document.write('<script src="js/jquery-1.4.2.min.js"><\/script>')</script>

The before action isn't covered by that use-case, but if we're going to have an after action, we might as well have a before one.

I've attached a patch that would allow for this. Dead simple.

Attachments (11)

wp-scripts-filters.diff (569 bytes) - added by mattwiebe 6 years ago.
wp-scripts-filter-2.diff (551 bytes) - added by mattwiebe 6 years ago.
14853.3.diff (3.8 KB) - added by atimmer 3 years ago.
class.wp-scripts.php.patch (1.0 KB) - added by abiralneupane 9 months ago.
wp-includes.patch (2.5 KB) - added by abiralneupane 9 months ago.
14853.diff (8.7 KB) - added by swissspidy 9 months ago.
14853.2.diff (10.5 KB) - added by swissspidy 8 months ago.
14853.4.diff (11.8 KB) - added by swissspidy 6 months ago.
14853.5.diff (12.1 KB) - added by swissspidy 6 months ago.
14853.6.diff (14.4 KB) - added by ocean90 6 months ago.
14853.7.diff (14.8 KB) - added by ocean90 6 months ago.

Download all attachments as: .zip

Change History (61)

#1 @mattwiebe
6 years ago

  • Cc mattwiebe added

Another example use-case that could make use of this from Typekit:

<script src="http://use.typekit.com/typekit-id.js"></script>
<!-- make use of wp_print_scripts_after-typekit -->
<script>try{Typekit.load();}catch(e){}</script>

And I'm sure tons more. It'd be nice for plugins to make use of WP methods rather than just spitting out their code in the wp_head or wp_footer actions.

#2 @mattwiebe
6 years ago

  • Keywords has-patch added

I thought about this some more, and a filter would be more appropriate than before/after actions. In addition to the benefits outlined above, this would allow a plugin to strip the type='text/javascript' when using HTML5 syntax.

#3 @mattwiebe
6 years ago

  • Summary changed from WP_Scripts::do_item before/after actions to WP_Scripts::do_item filter

This would also be useful for caching/minifying plugins. I've looked at WP Minify and the way it goes about its business is a bit of a nightmare. This would simplify things greatly.

#4 @westi
6 years ago

  • Keywords 3.2-early added
  • Owner set to westi
  • Status changed from new to accepted

I'll look at this when I go through and improve the Script Loader L10n support

#5 follow-up: @jczorkmid
6 years ago

  • Cc jczorkmid added

This would also be useful for doing the jQuery.noConflict() call when using an external jQuery.

#6 in reply to: ↑ 5 ; follow-up: @westi
6 years ago

Replying to jczorkmid:

This would also be useful for doing the jQuery.noConflict() call when using an external jQuery.

I'm hoping to move that call out of our bundled jQuery and into a second script on of these days :-)

#7 @nacin
6 years ago

  • Milestone changed from Awaiting Review to Future Release

#8 @scribu
5 years ago

Related: #16494

#9 @scribu
5 years ago

  • Keywords 3.2-early removed

A filter on the entire output would certainly allow the most flexibility, including the ability to change the attributes of <script> tags.

Note however that 'wp_print_scripts' is already used as an action. 'wp_print_script' would be more appropriate.

Another idea would be to introduce an add_inline_script() function, similar to the newly introduced add_inline_style() in [18464]. Example usage:

add_inline_script( 'jquery', '!window.jQuery && document.write(\'<script src="js/jquery-1.4.2.min.js"></script>\')' );

It could also accept a third parameter, $position, which could be 'before' or 'after'.

@atimmer
3 years ago

#10 @atimmer
3 years ago

  • Cc atimmermans@… added

14853.3.diff is a patch to add wp_add_inline_script the way scribu proposed. I really like this idea because it is consistent with wp_add_inline_style and it is very clear for developers.

I haven't added the possibility for the third parameter, because I couldn't think of any use case for 'before', if you can I will add the possibility.

Version 0, edited 3 years ago by atimmer (next)

#11 @nacin
3 years ago

  • Component changed from General to Script Loader

#12 @TobiasBg
21 months ago

#30617 was marked as a duplicate.

#13 follow-up: @aristath
14 months ago

Is there any reason why we haven't implemented this one yet?
If we're just waiting for a patch I could just start working on it...

#14 in reply to: ↑ 13 @samuelsidler
14 months ago

Replying to aristath:

Is there any reason why we haven't implemented this one yet?
If we're just waiting for a patch I could just start working on it...

I don't see any reason, but one may come up with a new patch. :) Working up a patch would be a good way to see what other issues exist.

#15 @johnbillion
14 months ago

  • Keywords needs-patch added; has-patch removed

#16 follow-up: @mcaskill
10 months ago

Maybe I'm just unfamiliar with SVN and WordPress' workflow but isn't this the patch?

#17 @swissspidy
9 months ago

#34784 was marked as a duplicate.

#18 in reply to: ↑ 16 @swissspidy
9 months ago

Replying to mcaskill:

Maybe I'm just unfamiliar with SVN and WordPress' workflow but isn't this the patch?

It is, but it's also over 5 years old and definitely needs a refresh.

#19 @abiralneupane
9 months ago

  • Focuses javascript added
  • Keywords has-patch added; needs-patch removed

Hello,

I have submitted patches (two files: class.wp-scripts.php.patch and wp-includes.patch).

Hope to see these function in upcoming WordPress version.

Thanks

#20 @swissspidy
9 months ago

  • Focuses javascript removed
  • Keywords needs-unit-tests added

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


9 months ago

#22 @abiralneupane
9 months ago

Hello,

Can someone do a unit testing on the patches? :)

@swissspidy
9 months ago

#23 follow-up: @swissspidy
9 months ago

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

14853.diff is a renewed patch that mirrors the functionality of WP_Styles for adding a wp_add_inline_script() function.

I've added a $position parameter to it as mentioned earlier in the ticket. This would be helpful when you need to set some variables before the script is loaded. What do other devs think about that?

Note that we can always remove that param in a further patch if it seems not worth doing.

Adds unit tests.

#24 in reply to: ↑ 23 @abiralneupane
9 months ago

That would be great feature to add

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


9 months ago

#26 @swissspidy
9 months ago

  • Owner changed from westi to swissspidy

#27 follow-up: @digamberpradhan
9 months ago

looks like this function will come handy, Is there something I can get involved with?

#28 in reply to: ↑ 27 @swissspidy
9 months ago

Replying to digamberpradhan:

looks like this function will come handy, Is there something I can get involved with?

At the moment I'd say: testing, testing, testing. Apply the patch on a local install (see the handbook for details how to do this) and try to develop some things using this function.
Does it work as expected or does it break? Is it intuitive, fast? Is the inline documentation clear?

#29 @swissspidy
8 months ago

  • Milestone changed from Future Release to 4.5
  • Summary changed from WP_Scripts::do_item filter to wp_add_inline_script()

#30 @swissspidy
8 months ago

  • Keywords dev-feedback removed

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


8 months ago

#32 in reply to: ↑ 6 @swissspidy
8 months ago

This would also be useful for doing the jQuery.noConflict() call when using an external jQuery.

I'm hoping to move that call out of our bundled jQuery and into a second script on of these days :-)

With the current patch, this would be as easy as calling $scripts->add_inline_script( 'jquery-core', 'jQuery.noConflict();' ); in script-loader.php.

#33 follow-up: @ocean90
8 months ago

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

14853.diff:

  • print_inline_script() returns always a boolean, but if $this->do_concat is true the expected return value is the script because of $this->print_code .= $this->print_inline_script( $handle, 'before', false );. print_inline_style() does it right in this case.
  • When using wp_add_inline_script( 'common', 'console.log("before");', 'before' ); the script is printed after the localized data, but in concat mode it's printed before. It should be always after the localized data IMO.
  • We don't have ID attributes for all the other script handling methods, is there a use case why add_inline_script() should add one? (Note: In concat mode the ID gets ignored.)
  • Beside jQuery.noConflict(); are there any other places where core could benefit of the new methods? Maybe wp_color_scheme_settings()? (For back-compat reasons I don't think we can remove the noConflict() line for a while.)

#34 in reply to: ↑ 33 @swissspidy
8 months ago

  • print_inline_script() returns always a boolean, but if $this->do_concat is true the expected return value is the script because of $this->print_code .= $this->print_inline_script( $handle, 'before', false );. print_inline_style() does it right in this case.
  • When using wp_add_inline_script( 'common', 'console.log("before");', 'before' ); the script is printed after the localized data, but in concat mode it's printed before. It should be always after the localized data IMO.

Yeah, that makes sense. Will update the patch.

  • We don't have ID attributes for all the other script handling methods, is there a use case why add_inline_script() should add one? (Note: In concat mode the ID gets ignored.)

I copied it from add_inline_style, but I agree that it's not really useful except that you know the handle it was added for. Doesn't hurt when it's absent.

  • Beside jQuery.noConflict(); are there any other places where core could benefit of the new methods? Maybe wp_color_scheme_settings()? (For back-compat reasons I don't think we can remove the noConflict() line for a while.)

I haven't found any other place where JavaScript is printed inline besides these two. It's mainly for developers I think.

@swissspidy
8 months ago

#35 @swissspidy
8 months ago

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

14853.2.diff includes two new tests for concatenated scripts, localized script data and even conditionals. It makes sure that conditionals (e.g. <!--[if gte IE 9]>) are always wrapped around inline scripts.

Contrary to to localized script data, inline scripts are added to $wp_scripts->print_html when the output should be concatenated. That's why test_wp_add_inline_script_concat() might look strange at first, but apparently that's the way it works.

@ocean90 Wanna have another look? :-)

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


7 months ago

#38 follow-up: @ocean90
6 months ago

@swissspidy In 14853.2.diff all the code from 14853.diff which handles concatenation is gone?

For example

<?php
add_action( 'admin_init', function() {
        wp_add_inline_script( 'common', 'console.log("after");' );
});

will print nothing.

#39 in reply to: ↑ 38 @swissspidy
6 months ago

  • Keywords needs-refresh added

Replying to ocean90:

In 14853.2.diff all the code from 14853.diff which handles concatenation is gone?

Sigh. Thanks for the heads up! Will add some tests so I don't forget it in the next patch.

@swissspidy
6 months ago

#40 follow-up: @swissspidy
6 months ago

  • Keywords needs-refresh removed

14853.4.diff is a new patch properly taking into account concatenated scripts in the admin.

Inline scripts are properly inserted after and before the handle (but after l10n scripts). If a script has a conditional comment, it's not concatenated just as before, with the inline scripts printed in the right order.

#41 in reply to: ↑ 40 @ocean90
6 months ago

Replying to swissspidy:

Inline scripts are properly inserted after and before the handle (but after l10n scripts). If a script has a conditional comment, it's not concatenated just as before, with the inline scripts printed in the right order.

That raises a question:

Let's say we move jQuery.noConflict(); out of jquery.js and instead we're adding it via wp_add_inline_script():

<?php
add_action( 'admin_init', function() {
        wp_add_inline_script( 'jquery-core', 'jQuery.noConflict();' );
});

Current output without concat:

<script type='text/javascript' src='http://develop.wp.dev/wp-includes/js/jquery/jquery.js?ver=1.12.0'></script>
<script type='text/javascript'>
jQuery.noConflict();
</script>
<script type='text/javascript' src='http://develop.wp.dev/wp-includes/js/jquery/jquery-migrate.js?ver=1.3.0'></script>

Current output with concat:

<script type='text/javascript' src='http://build.wp.dev/wp-admin/load-scripts.php?c=1&amp;load%5B%5D=jquery-core,jquery-migrate,hoverIntent,…,thickbox&amp;ver=4.5-alpha-20160221.211639'></script>
<script type='text/javascript'>
jQuery.noConflict();
</script>

Is this the expected output in case of concatenation? Or shouldn't it be instead:

<script type='text/javascript' src='http://develop.wp.dev/wp-includes/js/jquery/jquery.js?ver=1.12.0'></script>
<script type='text/javascript'>
jQuery.noConflict();
</script>
<script type='text/javascript' src='http://build.wp.dev/wp-admin/load-scripts.php?c=1&amp;load%5B%5D=jquery-migrate,hoverIntent,…,thickbox&amp;ver=4.5-alpha-20160221.211639'></script>

This only affects wp-admin and it seems like the current output works anyway, but I think it's worth do mention it here.

@swissspidy
6 months ago

#42 @swissspidy
6 months ago

14853.5.diff implements the suggestion by @ocean90. When a script has some inline scripts added after it, it's not concatenated together with the other sources, but instead printed before it (using $wp_scripts->print_html_before).

If someone wants to add some tests for this, feel free to do so. I'm currently stuck, but I'll try to have another look in the upcoming days.

#43 @ocean90
6 months ago

If someone wants to add some tests for this, feel free to do so.

Just dumping this here so it doesn't get lost:

<?php
/**
 * @group test_scripts_concat
 */
public function test_scripts_concat() {
        global $wp_scripts;

        $wp_scripts->do_concat = true;
        $wp_scripts->default_dirs = array( '/directory/' );

        wp_enqueue_script( 'one', '/directory/script.js' );
        wp_enqueue_script( 'two', '/directory/script.js' );
        wp_enqueue_script( 'three', '/directory/script.js' );

        wp_print_scripts();
        $print_scripts = get_echo( '_print_scripts' );

        $ver = get_bloginfo( 'version' );
        $expected = "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&amp;load%5B%5D=one,two,three&amp;ver={$ver}'></script>\n";

        $this->assertEquals( $expected, $print_scripts );
}

@ocean90
6 months ago

#44 @ocean90
6 months ago

14853.6.diff: Added test_script_concatenation(), test_wp_add_inline_script_before_with_concat(), and test_wp_add_inline_script_after_with_concat().

@ocean90
6 months ago

#45 @ocean90
6 months ago

  • Keywords commit added

#46 @swissspidy
6 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 36633:

Script Loader: Introduce wp_add_inline_script().

This new function can be used to add inline JavaScript before and after enqueued scripts, just like wp_add_inline_style() works for CSS.

Props atimmer, abiralneupane, ocean90, swissspidy.
Fixes #14853.

#47 @DrewAPicture
6 months ago

In 36706:

Docs: Standardize DocBlocks for two new WP_Scripts methods, add_inline_script() and print_inline_script(), introduced in [36633].

See #14853. See #32246.

#48 @DrewAPicture
6 months ago

In 36707:

Docs: Use a third-person singular verb in the DocBlock summary for wp_add_inline_script(), introduced in [36633].

See #14853. See #32246.

#49 @DrewAPicture
6 months ago

In 36917:

Docs: Add a missing DocBlock summary for the WP_Scripts->print_html_before property, added in [36633].

See #14853. See #35986.

Note: See TracTickets for help on using tickets.