WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 10 months ago

Last modified 10 months ago

#13592 closed enhancement (fixed)

script_loader_src and style_loader_tag filters

Reported by: olivM Owned by: 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 4 years ago.
Add script_loader_tag filter to match style_loader_tag.
13592.2.diff (890 bytes) - added by MikeHansenMe 10 months ago.
13592.3.diff (940 bytes) - added by MikeHansenMe 10 months ago.
added handle param and moved duplicate code to variable
13592.4.diff (1.1 KB) - added by MikeHansenMe 10 months ago.
updated with some cleanup

Download all attachments as: .zip

Change History (29)

comment:1 @nacin5 years ago

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

@Quiet Nic4 years ago

Add script_loader_tag filter to match style_loader_tag.

comment:2 @Quiet Nic4 years ago

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

comment:3 @quietnic4 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.

comment:4 @ocean903 years ago

#22245 was marked as a duplicate.

comment:5 @kawauso3 years ago

  • Cc Quiet Nic removed

comment:6 @ryanve3 years ago

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

comment:7 @scribu3 years ago

  • Type changed from feature request to enhancement

comment:8 @tollmanz3 years ago

  • Cc tollmanz@… added

comment:9 @taylorde3 years ago

  • Cc td@… added

comment:10 @retlehs3 years ago

  • Cc retlehs added

comment:11 @westonruter2 years ago

  • Cc weston@… added

comment:12 @walkinonwat3r2 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.

comment:13 @retlehs21 months 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

comment:14 @nacin20 months ago

  • Component changed from General to Script Loader

comment:15 @spacedmonkey10 months ago

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

comment:16 @johnbillion10 months 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).

@MikeHansenMe10 months ago

@MikeHansenMe10 months ago

added handle param and moved duplicate code to variable

comment:17 @MikeHansenMe10 months ago

  • Keywords needs-docs removed

comment:18 @spacedmonkey10 months 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.

@MikeHansenMe10 months ago

updated with some cleanup

comment:19 @MikeHansenMe10 months 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.

comment:20 @slackbot10 months ago

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

comment:21 follow-up: @ocean9010 months 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.

comment:22 @cfoellmann10 months 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.

comment:23 in reply to: ↑ 21 @spacedmonkey10 months 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.

comment:24 @johnbillion10 months 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

comment:25 @johnbillion10 months 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.