Make WordPress Core

Opened 15 years ago

Closed 10 years ago

Last modified 10 years ago

#13592 closed enhancement (fixed)

script_loader_src and style_loader_tag filters

Reported by: olivm's profile olivM Owned by: johnbillion's profile johnbillion
Milestone: 4.1 Priority: normal
Severity: normal Version:
Component: Script Loader Keywords: has-patch
Focuses: Cc:

Description

i'm trying to build a scripts and styles minifier.
( i don't like the way wp-minify does it ... )

thanks to the style_loader_tag filters, i'm pretty proud of my work for stylesheets.

but why don't we have a script_loader_tag ?

something like this in class.wp-scripts.php :

   $src .= apply_filters( 'script_loader_tag', "<script type='text/javascript' src='$src'></script>\n", $handle );

ok i'll try to submit a real patch.

Attachments (4)

13592.patch (760 bytes) - added by Quiet Nic 14 years ago.
Add script_loader_tag filter to match style_loader_tag.
13592.2.diff (890 bytes) - added by MikeHansenMe 10 years ago.
13592.3.diff (940 bytes) - added by MikeHansenMe 10 years ago.
added handle param and moved duplicate code to variable
13592.4.diff (1.1 KB) - added by MikeHansenMe 10 years ago.
updated with some cleanup

Download all attachments as: .zip

Change History (29)

#1 @nacin
14 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

@Quiet Nic
14 years ago

Add script_loader_tag filter to match style_loader_tag.

#2 @Quiet Nic
14 years ago

  • Cc Quiet Nic added
  • Keywords has-patch added; needs-patch removed

#3 @quietnic
14 years ago

  • Cc quietnic added
  • Keywords needs-testing added

## not directly related to the ticket, but not sure where else to post this - remove if inappropriate ##

My apologies to the users Quiet and Nic (if they exist) who were inadvertently Cc'd to this ticket. It seems Trac, as with WP.org, has problems with names containing spaces.

After reading a number of WP.org support threads registering a new user seems to be the only option, so here I am, on a new user. Sorry for the mess.

#4 @ocean90
12 years ago

#22245 was marked as a duplicate.

#5 @kawauso
12 years ago

  • Cc Quiet Nic removed

#6 @ryanve
12 years ago

So what happened to the "script_loader_tag" filter? See #22245

#7 @scribu
12 years ago

  • Type changed from feature request to enhancement

#8 @tollmanz
12 years ago

  • Cc tollmanz@… added

#9 @taylorde
12 years ago

  • Cc td@… added

#10 @retlehs
12 years ago

  • Cc retlehs added

#11 @westonruter
12 years ago

  • Cc weston@… added

#12 @walkinonwat3r
11 years ago

Just voicing support for this. It's a very simple change, but I see two immediate uses for it:

1) Conditional script loading

2) Finally solving the issue of how to enqueue a jQuery fallback in WP.

Currently, you can deregister the WP-provided jQuery and register jQuery from a CDN in its place. How to do this is well documented.

However, if jQuery fails to load (i.e. Microsoft CDN is down, or Google CDN is blocked in China), scripts on the page will simply fail. The solution to this is generally to test for jQuery, and load a local version if the test fails: http://css-tricks.com/snippets/jquery/fallback-for-cdn-hosted-jquery/

This solution is currently not compatible with script enqueueing, since the testing snippet has to be included immediately after the CDN script tag, so that no scripts fail in between the CDN tag and the fallback tag.

A script_loader_tag filter would allow us to immediately follow a jQuery CDN enqueue with a backup.

#13 @retlehs
11 years ago

It's a bit odd that WP core doesn't have a script_loader_tag filter considering there's a style_loader_tag filter.

1) Conditional script loading

Yes, please. It doesn't look like this or #16024 is going anywhere, but there really should be a solution in place to add conditionals for JS.

2) Finally solving the issue of how to enqueue a jQuery fallback in WP.

FYI, you can do this right now (although it's a bit nasty) - here's an example from the Roots starter theme: https://github.com/roots/roots/blob/aa59cede7fbe2b853af9cf04e52865902d2ff1a9/lib/scripts.php#L37-L52

#14 @nacin
11 years ago

  • Component changed from General to Script Loader

#15 @spacedmonkey
10 years ago

This there any reason this ticket has gone dead? It is small change that could mean a lot to developers and performance.

#16 @johnbillion
10 years ago

  • Keywords needs-docs added; needs-testing removed
  • Milestone changed from Future Release to 4.1

No reason for this not to go in. Needs some filter docs though (similar to style_loader_tag).

@MikeHansenMe
10 years ago

added handle param and moved duplicate code to variable

#17 @MikeHansenMe
10 years ago

  • Keywords needs-docs removed

#18 @spacedmonkey
10 years ago

Just an idea, how about something like this

https://gist.github.com/spacedmonkey/c45dc6c4ede0d4b96119

Stops you repeating the filter and added the do concat variable.

@MikeHansenMe
10 years ago

updated with some cleanup

#19 @MikeHansenMe
10 years ago

@spacemonkey I had actually thought about that last night and was on my way back to update the patch. I updated mine based on yours with the exception of I am not sure we need to pass concat into the filter.

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


10 years ago

#21 follow-up: @ocean90
10 years ago

For style_loader_tag we have this:

apply_filters(
	'style_loader_tag',
	"<link rel='$rel' id='$handle-css' $title href='$href' type='text/css' media='$media' />\n",
	$handle
);

So you can filter the entire HTML link tag. But 13592.4.diff allows only to filter the script part without the src. That looks confusing to me.
We should either filter the entire HTML script tag or rename the filter to something else.

#22 @cfoellmann
10 years ago

I think the script_loader_tag filter should be as similar as possible to style_loader_tag just like @ocean90 says.

This would also prevent the usage of 2 filters if you need to modify the src on the same run.

#23 in reply to: ↑ 21 @spacedmonkey
10 years ago

Replying to ocean90:

For style_loader_tag we have this:

apply_filters(
	'style_loader_tag',
	"<link rel='$rel' id='$handle-css' $title href='$href' type='text/css' media='$media' />\n",
	$handle
);

So you can filter the entire HTML link tag. But 13592.4.diff allows only to filter the script part without the src. That looks confusing to me.
We should either filter the entire HTML script tag or rename the filter to something else.

I disagree, if you wish to modify the src tag, make the filter take 2 params which will get the src. Then when you filter the script tag, just don't return a string with %s in it.

One of the uses I filter to add a async attribute script tag in the way you suggest, you would have to do a string replace like str_replace('<script ','<script async', $script); This is not a clear as filter the markup of the tag.

#24 @johnbillion
10 years ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

In 30403:

Apply a filter to the <script> tag for enqueued scripts in the same way a filter is applied to the <link> tag for enqueued styles.

Fixes #13592
Props quietnic, MikeHansenMe

#25 @johnbillion
10 years ago

r30403 adds a script_loader_tag filter which matches the style_loader_tag filter for styles.

The complete tag is passed into the filter. It would make no difference if %s was passed as the src attribute because you still need to do a string replacement in order to avoid stomping changes made by something else using the same filter (for example, another plugin which adds a new attribute to the tag).

The handle and src attribute are passed as parameters to the filter.

Note: See TracTickets for help on using tickets.