Stats: Update method for inserting JavaScript

[kraftbj]

We use a global var and a combo of wp_footer and shutdown to insert the stats js, initially as part of a workaround for sites that aren't using wp_footer. It's been long enough now that we can likely move to something more modern.

h/t fabacab/wp-sri#2 for the report

[rickcurran]

Hi, I'm trying to tackle this as a PHP-related "Good first bug". However, I'm not sure I fully understand the issues with the current method that is used to load the stats js file.

I understand that the stats JS is first attempted to be loaded using a call to wp_footer, and this is then followed by the workaround which is using the shutdown action to load it in using wp_head action, the latter only happening if the $rendered_stats_footer global variable is false.

What I'm trying to get my head around is what would be a 'more modern' solution to the issue. Is it safe to just presume that a theme will always usewp_footer? If so then a fairly simple update is to remove the workaround / fallback add_action( 'wp_head', 'stats_add_shutdown_action' ) and then remove the use of the $rendered_stats_footer global variable.
(If it's not safe to presume that a theme will always usewp_footer then I'm not sure what would be a more modern alternative way of doing it?)

I also wondered whether another modernisation would be to switch from the current method of printing the JavaScript block which sets all the _stq calls etc inline, instead using wp_enqueue_script to load in an external JavaScript file making use of wp_localize_script to set variables within this new external JS file. However, I wasn't sure if that is making it more complex than needed as well as adding yet another external JS file and corresponding loading overhead, printing inline is perhaps the most efficient method in this case?

An additional issue with using wp_enqueue_script is that there's currently no way to add the async or defer attributes so some further code would be required to apply these.

The code that renders the footer also outputs an AMP-specific footer with an 'amp-pixel' so this would have to be accommodated in any code change, I think this would still need to be output via wp_footer even if wp_enqueue_script was used for the stats JS.

I'd greatly appreciate any thoughts / suggestions anyone has!

[jeherve]

Is it safe to just presume that a theme will always use wp_footer?

I think that by now, it may be a safe assumption to make. WordPress themes have changed a lot since this was originally implemented, and it has now become quite rare to see a theme that does not include the most basic WordPress hooks that are wp_head and wp_footer.

An additional issue with using wp_enqueue_script is that there's currently no way to add the async or defer attributes so some further code would be required to apply these.

Yes, I believe that's a blocker for using wp_enqueue_script right now. Until async is supported directly into wp_enqueue_script, I don't think we'll be able to use it to load stats.
The Stats script is loaded on enough sites and we don't really want to impact those sites' performance.

Related Trac ticket:
https://core.trac.wordpress.org/ticket/12009

[rickcurran]

Thanks @jeherve, so would an improvement for now be to get rid of the workaround / fallback add_action( 'wp_head', 'stats_add_shutdown_action' ), drop the use of the $rendered_stats_footer global variable but just keep it adding the stats JS via the existingwp_footer action?

[rickcurran]

I also just noticed that the Jetpack Amp support class (class.jetpack-amp-support.php) has a function add_stats_pixel which does a has_action check for the wp_footer action that is adding the stats JS, so this would need updating too if there was a switch to use wp_enqueue_script .

[jeherve]

would an improvement for now be to get rid of the workaround / fallback add_action( 'wp_head', 'stats_add_shutdown_action' ), drop the use of the $rendered_stats_footer global variable but just keep it adding the stats JS via the existingwp_footer action?

Yes, that'd be nice, all the while making sure we don't break compatibility with AMP as you mentioned. 👍

[rickcurran]

Thanks, I've added a pull request #13182 for this now.

[mdawaffe]

#13182 is a nice code simplification, but let's leave this issue open for now since we may still need to address the SRI issue.

[stale]

This issue has been marked as stale. This happened because:

• It has been inactive in the past 6 months.
• It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

[jeherve]

This was eventually implemented in #29780.