Make WordPress Core

Opened 14 years ago

Closed 8 years ago

Last modified 8 years ago

#14853 closed enhancement (fixed)

wp_add_inline_script()

Reported by: mattwiebe's profile mattwiebe Owned by: swissspidy's profile 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 14 years ago.
wp-scripts-filter-2.diff (551 bytes) - added by mattwiebe 13 years ago.
14853.3.diff (3.8 KB) - added by atimmer 11 years ago.
class.wp-scripts.php.patch (1.0 KB) - added by abiralneupane 8 years ago.
wp-includes.patch (2.5 KB) - added by abiralneupane 8 years ago.
14853.diff (8.7 KB) - added by swissspidy 8 years ago.
14853.2.diff (10.5 KB) - added by swissspidy 8 years ago.
14853.4.diff (11.8 KB) - added by swissspidy 8 years ago.
14853.5.diff (12.1 KB) - added by swissspidy 8 years ago.
14853.6.diff (14.4 KB) - added by ocean90 8 years ago.
14853.7.diff (14.8 KB) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (61)

#1 @mattwiebe
14 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
13 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
13 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
13 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
13 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
13 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
13 years ago

  • Milestone changed from Awaiting Review to Future Release

#8 @scribu
13 years ago

Related: #16494

#9 @scribu
13 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
11 years ago

#10 @atimmer
11 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.

Edit:
Note that this is on the new develop.svn.wordpress.org repository, with the test included.

Last edited 11 years ago by atimmer (previous) (diff)

#11 @nacin
10 years ago

  • Component changed from General to Script Loader

#12 @TobiasBg
9 years ago

#30617 was marked as a duplicate.

#13 follow-up: @aristath
9 years 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
9 years 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
9 years ago

  • Keywords needs-patch added; has-patch removed

#16 follow-up: @mcaskill
8 years ago

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

#17 @swissspidy
8 years ago

#34784 was marked as a duplicate.

#18 in reply to: ↑ 16 @swissspidy
8 years 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
8 years 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
8 years ago

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

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


8 years ago

#22 @abiralneupane
8 years ago

Hello,

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

@swissspidy
8 years ago

#23 follow-up: @swissspidy
8 years 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
8 years ago

That would be great feature to add

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


8 years ago

#26 @swissspidy
8 years ago

  • Owner changed from westi to swissspidy

#27 follow-up: @digamberpradhan
8 years ago

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

#28 in reply to: ↑ 27 @swissspidy
8 years 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 years 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 years ago

  • Keywords dev-feedback removed

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


8 years ago

#32 in reply to: ↑ 6 @swissspidy
8 years 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 years 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 years 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 years ago

#35 @swissspidy
8 years 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.


8 years ago

#38 follow-up: @ocean90
8 years 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
8 years 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
8 years ago

#40 follow-up: @swissspidy
8 years 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
8 years 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
8 years ago

#42 @swissspidy
8 years 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
8 years 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
8 years ago

#44 @ocean90
8 years 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
8 years ago

#45 @ocean90
8 years ago

  • Keywords commit added

#46 @swissspidy
8 years 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
8 years 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
8 years 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
8 years 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.