Make WordPress Core

Opened 11 years ago

Closed 8 years ago

#26111 closed enhancement (maybelater)

wp_localize_script array from callback for performance

Reported by: ciantic's profile ciantic Owned by: wonderboymusic's profile wonderboymusic
Milestone: Priority: high
Severity: normal Version: 2.6
Component: Script Loader Keywords: has-patch
Focuses: performance Cc:

Description

These days wp_localize_script is being used to shovel all kinds of PHP -> JavaScript data, not just localization, and the performance may become bottleneck. It's recommended to write localize scripts during enequeue scripts action but this is still run every time.

It would be useful for performance to be able to put callback parameter which in turn would return the array, e.g. for my imaginary date_i18n script:

wp_localize_script("date_i18n", "DATE_I18N", null, function () { 
	global $wp_locale;
	$monthNames = array_map(array(&$wp_locale, 'get_month'), range(1, 12));
	$monthNamesShort = array_map(array(&$wp_locale, 'get_month_abbrev'), $monthNames);
	$dayNames = array_map(array(&$wp_locale, 'get_weekday'), range(0, 6));
	$dayNamesShort = array_map(array(&$wp_locale, 'get_weekday_abbrev'), $dayNames);

	return array(
		"month_names" => $monthNames,
		"month_names_short" => $monthNamesShort,
		"day_names" => $dayNames,
		"day_names_short" => $dayNamesShort
	);
});

WordPress could then run the callback only if the script e.g. date_18n is enqueued.

For backwards compatibility it is required to introduce fourth parameter the $l10n_callback for the wp_localize_script, this is why in my example third is null.

I must also note, that since wp_localize_script is being used to pass data to javascript (e.g. retrieving posts from db) one could also introduce a new function like wp_data_script, but this would not be backwards compatible.

Thank you for considering.

Attachments (3)

26111.diff (1.6 KB) - added by jtsternberg 10 years ago.
Pretty simple fix that checks if $l10n argument is callable and call it if so.
26111-2.diff (545 bytes) - added by jtsternberg 10 years ago.
Include script handle and object name for context in callback function
26111.2.diff (4.0 KB) - added by DrewAPicture 10 years ago.
4th parameter + docs

Download all attachments as: .zip

Change History (34)

#1 @johnbillion
11 years ago

  • Cc johnbillion added
  • Version changed from trunk to 2.6

#2 follow-up: @SergeyBiryukov
11 years ago

  • Keywords close added

WordPress could then run the callback only if the script e.g. date_18n is enqueued.

You can check if the script is enqueued using wp_script_is():

function date_i18n_callback() {
	...
	return array( ... );
}

function localize_scripts_26111() {
	if ( wp_script_is( 'date_i18n' ) ) {
		wp_localize_script( 'date_i18n', 'DATE_I18N', date_i18n_callback() );
	}
}
add_action( 'wp_enqueue_scripts', 'localize_scripts_26111', 11 );

date_i18n_callback() will only run if 'date_i18n' is enqueued. No changes to wp_localize_script() are needed.

#3 in reply to: ↑ 2 @ciantic
11 years ago

Neat that it is possible, however this is not very developer friendly way.

Adding a callback to wp_localize_script would make this more usable and provide better performance benefit over time if people start to use it.

Replying to SergeyBiryukov:

WordPress could then run the callback only if the script e.g. date_18n is enqueued.

You can check if the script is enqueued using wp_script_is():

function date_i18n_callback() {
	...
	return array( ... );
}

function localize_scripts_26111() {
	if ( wp_script_is( 'date_i18n' ) ) {
		wp_localize_script( 'date_i18n', 'DATE_I18N', date_i18n_callback() );
	}
}
add_action( 'wp_enqueue_scripts', 'localize_scripts_26111', 11 );

date_i18n_callback() will only run if 'date_i18n' is enqueued. No changes to wp_localize_script() are needed.

#4 @nacin
11 years ago

  • Component changed from Performance to Script Loader
  • Focuses performance added
  • Priority changed from normal to low

I've wanted to move this (and post type labels) to a closure for a while. We still support PHP 5.2, though. I wouldn't mind adding it so others can benefit from it in their own environments, though it's not much of a priority as it's more than 50% of all WordPress sites.

I'd be happy to consider a patch. I don't think a new parameter is needed. One would only need to check if the passed parameter is an array or a closure, and than handle that internal to WP_Scripts somehow.

#5 @wonderboymusic
10 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Reopen if anyone is interested and can provide a patch

@jtsternberg
10 years ago

Pretty simple fix that checks if $l10n argument is callable and call it if so.

#6 @jtsternberg
10 years ago

  • Keywords has-patch reporter-feedback added; close removed
  • Resolution maybelater deleted
  • Status changed from closed to reopened

#7 @SergeyBiryukov
10 years ago

  • Keywords needs-testing added; reporter-feedback removed
  • Milestone set to Awaiting Review

#8 @wonderboymusic
10 years ago

  • Keywords needs-testing removed
  • Milestone changed from Awaiting Review to 4.2

#9 @wonderboymusic
10 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from reopened to closed

In 31030:

Allow the 3rd argument to wp_localize_script()/WP_Scripts->localize() to be a callable, allowing data to be lazy-loaded when the script is actually enqueued.

Props jtsternberg.
Fixes #26111.

#10 follow-up: @dd32
10 years ago

Minor suggestion: The function should be called with the script handle, or any other context details about the enqueue.

#11 @wonderboymusic
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#12 in reply to: ↑ 10 @jtsternberg
10 years ago

Replying to dd32:

Minor suggestion: The function should be called with the script handle, or any other context details about the enqueue.

I'm a bit ashamed I didn't think of that. Patch incoming.

@jtsternberg
10 years ago

Include script handle and object name for context in callback function

#13 @jtsternberg
10 years ago

  • Keywords 2nd-opinion added

#14 @wonderboymusic
10 years ago

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

In 31033:

After [31030]: if a callable is passed as the 3rd arg to wp_localize_script()/WP_Scripts->localize(), pass $handle and $object_name to the user func when invoking it.

Props jtsternberg.
Fixes #26111.

#15 follow-up: @jdgrimes
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Unfortunately, this causes issues when someone is localizing a script with a string that just happens to also be a function name.

Real life example, from the Access monitor plugin (1):

wp_localize_script( 'am.functions', 'am_ajax_action', 'am_ajax_query_tenon' );

am_ajax_query_tenon is also the name of the function that responds to the action. This causes the Ajax requests to fail, because they are getting null as the action they should use (because that plugin doesn't return anything).

I'm not sure if this will be a common problem, but it could really break things, even kill a site (we're just lucky that die isn't being called in the middle of every page load in this case).

#16 in reply to: ↑ 15 ; follow-up: @DrewAPicture
10 years ago

  • Keywords dev-feedback added; 2nd-opinion removed
  • Priority changed from low to high
  • Summary changed from wp_localize_script array from callback for performance to K

Replying to jdgrimes:

Unfortunately, this causes issues when someone is localizing a script with a string that just happens to also be a function name.

Looking more closely at WP_Scripts::localize(), I think we kind of painted ourselves into a corner by not properly type-checking $l10n as an array before [31030], instead just casting whatever is passed as an array. Meaning of course, that anybody previously passing a string would have been within bounds.

I'm not sure if this will be a common problem, but it could really break things, even kill a site (we're just lucky that die isn't being called in the middle of every page load in this case).

@wonderboymusic might have a better idea, but the best I can come up with would be a fourth parameter to toggle whether the third parameter should be considered callable. Not exactly elegant but would account for back-compat.

#17 @DrewAPicture
10 years ago

  • Summary changed from K to wp_localize_script array from callback for performance

whoops

#18 in reply to: ↑ 16 @jdgrimes
10 years ago

Replying to DrewAPicture:

@wonderboymusic might have a better idea, but the best I can come up with would be a fourth parameter to toggle whether the third parameter should be considered callable. Not exactly elegant but would account for back-compat.

That is what I had in mind as well.

#19 @DrewAPicture
10 years ago

Added 26111.2.diff to add the fourth parameter + docs. Our other option at this stage would be to revert [31030] and [31033], and fall back and regroup in 4.3.

@DrewAPicture
10 years ago

4th parameter + docs

#20 follow-up: @jdgrimes
10 years ago

Another alternative is to take nacin's original approach, and only support this with closures. Or only closures and arrays (though arrays might not be 100% immune to this).

#21 in reply to: ↑ 20 @jdgrimes
10 years ago

Replying to jdgrimes:

arrays might not be 100% immune to this

I say this because someone might happen to pass an array that like array( 'Some_Class', 'some_static_method' ). Though of course that is far less likely.

But we might also want to consider the case where someone might be doing something like this: wp_localize_script( 'my_script', 'my_js_ob', $_GET['something'] ). This might have been safe before. Now it would mean arbitrary function execution (unless we limit this to only closures).

#22 @SergeyBiryukov
10 years ago

How about making the 4th argument the actual callback, only used when the 3rd argument is empty (as originally proposed in the ticket)?

wp_localize_script( 'date_i18n', 'dateI18N', null, function() {
	...
});
Last edited 10 years ago by SergeyBiryukov (previous) (diff)

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


10 years ago

#24 follow-up: @DrewAPicture
10 years ago

  • Keywords revert added

I think the best course of action right now is to revert [31030] and [31033] and regroup in 4.3.

#25 in reply to: ↑ 24 @azaozz
10 years ago

Replying to DrewAPicture:

Think so too. The changes here made localizing a script quite more complex and unfriendly. I don't see a point is adding them at all.

The only enhancement is that a closure could be used to supply the array of translated strings. Unfortunately we still support PHP 5.2 so closures are out of the question. There might be a minor speed up from not translating 60-70 strings, but it's probably offset by calling functions few times, using is_callable(), etc.

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


10 years ago

#27 @wonderboymusic
10 years ago

  • Keywords dev-feedback revert removed
  • Milestone changed from 4.2 to Future Release

#28 @SergeyBiryukov
10 years ago

[32124] missed the ticket.

#29 @nacin
10 years ago

Keep in mind even an array can be a callable.

I have a feeling the future here will be a new method. Closures are cool but inflexible.

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


8 years ago

#31 @peterwilsoncc
8 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from reopened to closed

between age and the last comment, I'm going to close this out as maybe later.

Note: See TracTickets for help on using tickets.