Separate the global site tag setup and event tracking #398

martynmjones · 2024-03-13T10:23:10Z

Changes proposed in this Pull Request:

The changes to how the tracker is initiated in version 2.0.0 has caused problems with more sites than anticipated. Primarily those problems come from plugins that minify and combine JS files as many of them disregard listed dependencies while doing so.

As it currently stands there are many optimisation plugins that do work well with Google Analytics for WooCommerce but to make the extension more resilient against unexpected scenarios from different optimisation techniques this PR restructures how things execute.

dataLayer and gtag (or a function using the filtered tracker var name) are no longer included in the JS file. Instead they are added directly to the page <head>
- This is closer to how it was functioning pre-2.0.0
- Does not use the WordPress script queue system to prevent this block of code being moved to anywhere else
- Hooks into wp_head early so that the script is added as soon as possible. This makes it accessible to other plugins and means we are able to rely on it being available on the page regardless of when our scripts load 582e41b
Relies on the DOMContentLoaded event to ensure inline script data is available at the point we begin tracking events
- E2E tests have been added to cover some of the scenarios we've seen with optimisation plugins that has caused problems with this 8e7481d
Restructures the tracker to be more functional 9fede11

Detailed test instructions:

Build extension from update/separate-site-tag-setup-and-event-tracking
Run smoke tests to confirm all events are tracked
Install an optimisation plugin like Autoptimize
- Configure the extension to combine and minify all Javascript
Check the page source to confirm that gtag is still setup in the page <head>
Re-run smoke tests to confirm events are still tracked

Additional details:

An adjustment was made to the selector for the related products add to cart E2E test as it was occasionally failing if a variable product was the first displayed in the related products list 25bd09c

Changelog entry

Update - Separate the site tag from the event tracking file and delay execution until DOMContentLoaded

It's loaded async anyway, so we don't need to hack to have it in place for the tracker definition.

includes/class-wc-google-gtag-js.php

https://github.com/woocommerce/woocommerce-google-analytics-integration/pull/398/files#r1523719734

tomalec · 2024-03-13T19:30:21Z

I have quite a revolutionary proposal, to change the Tracker from singleton to just a function and remove the config file: https://github.com/woocommerce/woocommerce-google-analytics-integration/compare/update/separate-site-tag-setup-and-event-tracking...update/separate-site-tag-setup-and-event-tracking-new-tracker?expand=1#diff-a745b91bc9c87614ca4293b765bb7a9053681512cb2cce7d9158b6e2946048b8R95

quite revolutionary, but TBH, at the stage of 2.0.0, I think separating tracker and postponing blocks & classic tracking after dom content loaded could be disrupting anyway.

WDYT?

tomalec · 2024-03-13T19:38:38Z

includes/class-wc-google-gtag-js.php

+		// phpcs:disable WordPress.WP.EnqueuedResources.NonEnqueuedScript
+		printf(


I wonder if we should be so brutal to break WP rules and print script directly; maybe enqueueing it as an inline with/ no dependencies would be enough? Or does the "optimization" plugins could force scripts enqueued in the head to be loaded later?

Some of those plugins have settings to combine all inline JS and some will move them to a different file. As we know the script needs to be available at this point no matter what I think it makes sense that we avoid that possibility.

Instead of outputting it directly I've wrapped it in wp_print_inline_script_tag so that's we're still doing things the WordPress way 582e41b

and hae a place to document the expected config data structure.

includes/class-wc-google-gtag-js.php

tomalec · 2024-03-13T20:25:22Z

If we aim to support "optimization" plugins, it seems there is another plugin that turns parges into static ones, which undermines the entire idea of setting any data in PHP. See #399

remove config file, pass settings and configs as arguments.

Address #398 (comment)

… from tests we no longer add it

Check that no dataLayer is set.

…t is loaded

tomalec · 2024-03-23T19:24:45Z

Does not use the WordPress script queue system to prevent this block of code from being moved to anywhere else

I wonder if we really need to go that extreme. Pre 2.0.0 implementation used wp_add_inline_script and I assume it was working ok, with other extensions. Or maybe you have found an extension that breaks when we put dependency-less inline script using WP queue system?

I'm concerned that if we were using the WP queue before, then maybe there are extensions in the wild that rely on that. For example, there may be a need to use the WP queue system to hook their scripts before/after to add/overwrite some gtag config with precise timing.

tomalec

Tested locally with and w/o Autooptimizer, and reviewed the code. LGTM
I'd appreciate someone reviewing my code :)

mikkamp

I left some additional comments as I looked through the code.

As @tomalec mentioned, I'm also a bit concerned about the use of wp_print_inline_script_tag compared to the previous use of wp_add_inline_script.

There is no filter for the contents of wp_print_inline_script_tag only for the script attributes. Whereas wp_add_inline_script allows us a lot more flexibility. That becomes evident with the GLA compatibility where it isn't able to set config data right after the snippet in the head.

assets/js/src/index.js

mikkamp · 2024-03-26T14:00:10Z

includes/class-wc-google-gtag-js.php

+				esc_js( self::get( 'ga_id' ) ),
+				esc_js( self::tracker_function_name() ),
+				esc_js( self::DEVELOPER_ID ),
+				json_encode( self::get_consent_modes() ),
+				json_encode( $this->get_site_tag_config() )


Are we tied to legacy function names / visibility here? Just looks confusing to be mixing self and $this.

I had a bigger concern with the self:: functions, especially self::get.
But I didn't want to push it here, not to refactor everything at once.
We use a lot of static functions that are not 100% "static," IMO.

PHP spec

Declaring class properties or methods as static makes them accessible without needing an instantiation of the class.

public static function get_enabled_events uses self::get

public static function load_opt_out uses self::get

public static function get_product_identifier uses self::get

public static function get uses the self::settings
but we set the self::settings in the instance constructor https://github.com/woocommerce/woocommerce-google-analytics-integration/blob/cdb60c08a9cb62f351f5d324472ad62b6eef7105/includes/class-wc-google-gtag-js.php#L47:L47

So we could change self::get( * ) to $this->get() or event $this->settings[ * ] (IMHO, get just blurs the picture here w/o much benefit).

Do you also suggest changing tracker_function_name, DEVELOPER_ID, and get_consent_modes to be instance methods/props?

I'd suggest in this case to access the methods through the current instance, which will make it easier in the future to remove static from those function declarations. We could do that now but then code snippets / other plugins would have a little harder time getting the instance.

Suggested change

esc_js( self::get( 'ga_id' ) ),

esc_js( self::tracker_function_name() ),

esc_js( self::DEVELOPER_ID ),

json_encode( self::get_consent_modes() ),

json_encode( $this->get_site_tag_config() )

esc_js( $this->get( 'ga_id' ) ),

esc_js( $this->tracker_function_name() ),

esc_js( self::DEVELOPER_ID ),

json_encode( $this->get_consent_modes() ),

json_encode( $this->get_site_tag_config() )

Generally the purpose of the get helper function would be to enforce both the parameter type for key and the return value. However it seems the return value for non_scalar values is broken. If you do something like this you'll get a fatal error:

self::$settings['ga_id'] = []; var_dump( $this->get( 'ga_id' ) );

Where we do see it working is for example if we do something like:

self::$settings['ga_id'] = 1; var_dump( $this->get( 'ga_id' ), self::$settings['ga_id'] );

The first value will be a string, but the second will be an int, so we will be less certain what kind of value we will be working with if we don't use the get method.

Thanks :) I changed the reference in non-static methods to use $this-> in 867f696 (plus tweaked self::DEVELOPER_ID to static::DEVELOPER_ID, to suppress the notice).

Let's change the static in methods in a separate PR and release. I'll start a draft to keep this idea on track

Generally the purpose of the get helper function would be to enforce both the parameter type for key and the return value. However it seems the return value for non_scalar values is broken. If you do something like this, you'll get a fatal error:

I wonder if we really need such behavior. I was experimenting with adding array-like settings for consent modes, and this stringification behavior was more of a problem than a help. But that's also something to address in a separate PR

Definitely limiting, and assuming of what settings are like. Agree that it would be better handled in a separate PR.

Added an issue to track that enhancement idea #412

includes/class-wc-google-gtag-js.php

mikkamp · 2024-03-26T14:08:59Z

includes/class-wc-google-gtag-js.php

+	 * @return void
+	 */
+	public function setup_site_tag() {
+		wp_print_inline_script_tag(


Does printing the snippet directly have any consequences for other plugins extending the code?

These solutions are not as compatible anymore: https://github.com/woocommerce/google-listings-and-ads/blob/2.6.4/src/Google/GlobalSiteTag.php#L220-L237

In theory the filter woocommerce-google-analytics-integration still works, but that's added much later and not right after the snippet in the head (the timing when we want to set configuration).

We can't use the filter woocommerce_ga_gtag_config because we don't want to add additional config parameters, we want to add tracking to a second GA_ID. Maybe moving the three gtag lines to an array of strings and then filtering the array before each line is added would be more flexible.

Address #398 (comment)

to more accurately reflect what they are. Address #398 (comment)

tomalec · 2024-03-26T21:49:28Z

Thank you, @mikkamp, for the detailed review!
I addressed some of your comments. AFAIK, @martynmjones is testing whether there are some extreme optimization plugins that would cause problems with a regularly enqueued inline script with gtag definition.
In the meantime, I'd appreciate your answers in the remaining threads.

mikkamp

Thanks for addressing the additional comments, I've added some additional thoughts there.

At this point the only thing in this PR that I'm a little on the fence about is:

There is no filter for the contents of wp_print_inline_script_tag only for the script attributes. Whereas wp_add_inline_script allows us a lot more flexibility. That becomes evident with the GLA compatibility where it isn't able to set config data right after the snippet in the head.

GLA still injects triggers for the events directly within the page body, so we'll want to ensure that the configuration is set early (in the head). One of the following options could resolve that:

Allow filtering of the snippet added in wp_print_inline_script_tag (the whole snippet, or filter for adding extra lines)
Revert back to using wp_add_inline_script so we can enqueue configuration right after the script.

In addition to that I think if we'd pick like 3 or 4 of the most popular optimizer plugins, if we are compatible with more or less the default configurations for optimizing JS then I'd see that as acceptable to release this PR.

Address https://github.com/woocommerce/woocommerce-google-analytics-integration/pull/398/files#r1539297700

to avoid `Constant from class 'WC_Abstract_Google_Analytics_JS' referenced through child. PHP6606` notice

martynmjones · 2024-03-28T13:13:00Z

In addition to that I think if we'd pick like 3 or 4 of the most popular optimizer plugins, if we are compatible with more or less the default configurations for optimizing JS then I'd see that as acceptable to release this PR.

I've run some tests with a few of the popular optimisation plugins using the different configurations on offer. There have been two issues with Autoptimize, Aggregate inline JS as you pointed out earlier, and forcing the JS script to load in the <head>.

If main.js is moved to the <head> then it will continue working and we have a test to cover that scenario. The problem with that setting in Autoptimize is that dependencies are ignored so several things break and not just Google Analytics for WooCommerce. So I don't think we need to make adjustments to try to have the extension work with that setting.

For Aggregate inline JS, the easiest fix is to add ga4w to the list of excluded scripts/ variables in Autoptimize. So an option would be to keep things as they are an add a "Known incompatibilities" section to the docs and explain to merchants who want to have inline JS aggregated how to exclude the script?

Plugin Name	Minify JS	Combine JS Files	Aggregate inline JS	Defer JS	Delay Execution	Force JS in `<head>`
Autoptimize	✅	✅	❌	✅	n/a	❌
WP-Optimize	✅	✅	n/a	✅	n/a	n/a
WP Rocket	✅	✅	n/a	✅	✅	n/a
W3 Total Cache	✅	✅	✅	✅	✅	n/a
LiteSpeed Cache	✅	✅	✅	✅	✅	n/a

tomalec · 2024-03-28T20:20:38Z

Thanks for the nice comparison @martynmjones !

I think we can try to make workarounds to make our code work if the scripts are aggregated, loaded in random order, or extracted to external JS. They question is: do we want to?

Make JS work even if data is loaded from an external defered async script or an external file—but if someone puts that data into an external file, is it still dynamically populated with the data we collect with hooks?

We could assume it is. But if it's not it may silently corrupt the behavior, which will be hard to identify by HEs.

Alternatively, we could make JS fully static, defer, and async by default, and put the data into HTML, like:
<wc-ga4w event-data="{JSON with data}">
This will let all the optimizers optimize the JS, which could be cached, deferred, etc, but we'll keep our data safe in the markup.
And also would me it easy to pick up the timing when the data gets there: customElements.define('wc-ga4w', class extends HTMLElement{ attributeChangedCallback });

Then:

if JS is loaded before the element is stamped to the document, the element will do the job when it's ready
otherwise, the element will do the job once it's upgraded == when the JS gets loaded
our JS could have the load event listener to throw a warning if there is not document.querySelector('wc-ga4w') after the document is complete` => meaning the JS was loaded but expected data is missing.

Plus, we'll have the good old separation of concerns: JS is for behavior, HTML is for the content.

warn if still not available after the document is fully loaded. Make the extension work, regardless of the order and timing of `woocommerce-google-analytics-integration` and `woocommerce-google-analytics-integration-data`scripts. Addresses #398 (comment) Make it work with Autoptimize "Aggregate inline JS" option, address #398 (comment)

tomalec · 2024-03-28T21:11:46Z

@martynmjones @mikkamp I made a change in our logic flow and conditions: dacaabc
Now:

woocommerce-google-analytics-integration-data sets the data global and dispatches an event to document
woocommerce-google-analytics-integration:
- Use the data if it's already there, to process it ASAP
- If it's not there:
  - Add an event listener for our own event to initialize once data is ready,
  - Warn if/once the document is fully loaded, and the data is still missing.

I believe this way, we're more bulletproof towards other extensions messing up with our order and timing.

Pros:

Autoptimize "Aggregate inline JS" works
I'm not sure how about "Force JS in <head>" as for me, it breaks other parts of Woo, but theoretically, it should work as well, as long as both scripts eventually get somewhat loaded.
More lightweight than a custom element implementation.

Cons:

Autoptimize "Aggregate inline JS" works; however, it puts dynamic, cart-specific data into a cacheable, external JS file, which would malform the data silently.
"Pollutes" the document with one event
Will keep showing warnings for folks who load woocommerce-google-analytics-integration in blocking fashion but woocommerce-google-analytics-integration-data - but maybe that's actually good thing
It assumes ga4w:ready means window.ga4w is ours and ready. Will not protect us against somebody triggering ga4w:ready event on their own w/o window.ga4w available. Custom element way would have a bit more integrity in that regard. But I guess we cannot protect against everything.
Puts a bit more JS in PHP string

mikkamp · 2024-03-29T11:44:56Z

Thanks @martynmjones for all the testing, that's a great result, and I think sufficient enough to move forward.

@tomalec I like your last addition and I think that makes it a bit more robust.

it puts dynamic, cart-specific data into a cacheable, external JS file, which would malform the data silently

Having the cart specific data here is not the most ideal solution. Instead we should be fetching it from the cart fragments or the data used to populate the mini cart, but that's getting a little out of scope and goes beyond what we had pre 2.0

As for the other cons you listed, I think it's good to be aware of them, but not necessarily something we want to protect against. Placing some expectations in the documentation would probably be a better way to go about it for now.

So from my perspective this PR is pretty much ready to go. I'm just not sure what the final conclusion to this comment was: #398 (comment)
Are we still looking to make that more flexible here?

…acking

martynmjones · 2024-04-04T18:00:24Z

So from my perspective this PR is pretty much ready to go. I'm just not sure what the final conclusion to this comment was: #398 (comment)
Are we still looking to make that more flexible here?

Thanks for all the input @mikkamp 👍 I updated the PR so it's closer to how we were previously doing it 77067d8

martynmjones added 3 commits March 13, 2024 07:40

Setup global site tag in head

c1cc257

Wait until DOMContentLoaded before tracking

0a3af1e

CS

ac06b6d

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Mar 13, 2024

martynmjones mentioned this pull request Mar 13, 2024

Warn about potential scripts order mismatch. #379

Closed

4 tasks

tomalec added 3 commits March 13, 2024 17:55

Initialize instantly, if the document is already loaded.

d064dfb

Add code doc explaining why we wait to add block actions

f3fe7b4

Enqueue tagmanager in regular way,

9ed093f

It's loaded async anyway, so we don't need to hack to have it in place for the tracker definition.

tomalec reviewed Mar 13, 2024

View reviewed changes

includes/class-wc-google-gtag-js.php Show resolved Hide resolved

Re-add identifier to the settings object

5a9d161

https://github.com/woocommerce/woocommerce-google-analytics-integration/pull/398/files#r1523719734

tomalec force-pushed the update/separate-site-tag-setup-and-event-tracking branch from 0ac886d to 5a9d161 Compare March 13, 2024 18:39

tomalec reviewed Mar 13, 2024

View reviewed changes

tomalec added 2 commits March 13, 2024 20:41

Bring back the woocommerce_ga_gtag_config hook.

92e96fb

Bring back trackClassicPages params to limit the number of changes

c241828

and hae a place to document the expected config data structure.

tomalec force-pushed the update/separate-site-tag-setup-and-event-tracking branch from 7b3ef32 to c241828 Compare March 13, 2024 19:46

tomalec reviewed Mar 13, 2024

View reviewed changes

includes/class-wc-google-gtag-js.php Outdated Show resolved Hide resolved

tomalec mentioned this pull request Mar 14, 2024

Consent Mode not updated with page caching #399

Closed

Rewrite tracker to more functional structure,

9fede11

remove config file, pass settings and configs as arguments.

tomalec added this to the ot sure as I was testing with that branch as well and there was a problem milestone Mar 19, 2024

tomalec and others added 7 commits March 19, 2024 22:14

Separate data from settings in JS object

75e3053

Address #398 (comment)

Remove woocommerce-google-analytics-integration-js-before assertion…

f2afcf3

… from tests we no longer add it

Harden no-trackign for admin tests

ae7562d

Check that no dataLayer is set.

Add E2E tests to confirm tracking works regardless of where the scrip…

8e7481d

…t is loaded

Update related products add to cart selector

25bd09c

Wrap tag setup in wp_print_inline_script_tag

582e41b

CS fixes

d1629c2

martynmjones marked this pull request as ready for review March 22, 2024 14:39

martynmjones requested a review from a team March 22, 2024 14:39

martynmjones assigned tomalec and martynmjones Mar 22, 2024

tomalec approved these changes Mar 23, 2024

View reviewed changes

mikkamp reviewed Mar 26, 2024

View reviewed changes

tomalec added 2 commits March 26, 2024 21:41

Fix the convention for JS block comments

4d6f371

Address #398 (comment)

Rename event handler-related function names

cdb60c0

to more accurately reflect what they are. Address #398 (comment)

mikkamp reviewed Mar 27, 2024

View reviewed changes

tomalec added 2 commits March 27, 2024 19:33

Unify self:: to $this-> reference in instance methods

867f696

Address https://github.com/woocommerce/woocommerce-google-analytics-integration/pull/398/files#r1539297700

Use static:: to reference DEVELOPER_ID in parent class,

89487e6

to avoid `Constant from class 'WC_Abstract_Google_Analytics_JS' referenced through child. PHP6606` notice

This was referenced Mar 28, 2024

Consider using WP Script Modules API #411

Open

Make methods that use settings non-static #412

Open

martynmjones and others added 4 commits April 3, 2024 19:31

Move gtag and dataLayer setup to registered inline script

77067d8

Merge branch 'trunk' into update/separate-site-tag-setup-and-event-tr…

f154988

…acking

PHPCS and add snippet to bypass WooCommerce dependency in e2e tests

757b3e8

Fix WC bypass snippet

5d587a3

martynmjones merged commit 7137073 into trunk Apr 4, 2024
6 checks passed

martynmjones deleted the update/separate-site-tag-setup-and-event-tracking branch April 4, 2024 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate the global site tag setup and event tracking #398

Separate the global site tag setup and event tracking #398

martynmjones commented Mar 13, 2024 •

edited

Loading

tomalec commented Mar 13, 2024

tomalec Mar 13, 2024

martynmjones Mar 22, 2024

tomalec commented Mar 13, 2024

tomalec commented Mar 23, 2024

tomalec left a comment

mikkamp left a comment

mikkamp Mar 26, 2024

tomalec Mar 26, 2024

mikkamp Mar 27, 2024

tomalec Mar 27, 2024

tomalec Mar 27, 2024

mikkamp Mar 28, 2024

tomalec Mar 28, 2024

mikkamp Mar 26, 2024 •

edited

Loading

tomalec commented Mar 26, 2024

mikkamp left a comment •

edited

Loading

martynmjones commented Mar 28, 2024 •

edited by tomalec

Loading

tomalec commented Mar 28, 2024 •

edited

Loading

tomalec commented Mar 28, 2024

mikkamp commented Mar 29, 2024

martynmjones commented Apr 4, 2024

		// phpcs:disable WordPress.WP.EnqueuedResources.NonEnqueuedScript
		printf(

Separate the global site tag setup and event tracking #398

Separate the global site tag setup and event tracking #398

Conversation

martynmjones commented Mar 13, 2024 • edited Loading

Changes proposed in this Pull Request:

Detailed test instructions:

Additional details:

Changelog entry

tomalec commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec commented Mar 13, 2024

tomalec commented Mar 23, 2024

tomalec left a comment

Choose a reason for hiding this comment

mikkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkamp Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

tomalec commented Mar 26, 2024

mikkamp left a comment • edited Loading

Choose a reason for hiding this comment

martynmjones commented Mar 28, 2024 • edited by tomalec Loading

tomalec commented Mar 28, 2024 • edited Loading

tomalec commented Mar 28, 2024

mikkamp commented Mar 29, 2024

martynmjones commented Apr 4, 2024

martynmjones commented Mar 13, 2024 •

edited

Loading

mikkamp Mar 26, 2024 •

edited

Loading

mikkamp left a comment •

edited

Loading

martynmjones commented Mar 28, 2024 •

edited by tomalec

Loading

tomalec commented Mar 28, 2024 •

edited

Loading