Multiple containers don't load

One use case for multiple containers (at least as far as config is concerned):

We have a distribution that manages configuration for 1000's of sites, and do not want to override our core distro config just to add an additional container for our clients that have them, because that adds significant service, support and maintenance overhead.

Fair enough use case indeed -- however tests are failing so moving to needs work until that issue can be uncovered.

This patch wont apply to current 2.0.x-dev version. When viewing the MR, the page throws an error and comments and MR not viewable. How do we move forward with this?

Please review Patch

@chaitanyadessai's patch seems to be a reroll of !54, so I applied that locally.

I found that the code did not load multiple gtag script tags correctly. Multiple tags loaded, but each tag repeated the config of the first tag (except for that the tagId). This gave a 404 for those tags.

To resolve this, I took the pattern from google_tag_page_top() and refactored google_tag_page_attachments() accordingly.

gtm.js also needed to account for multiple tag configs, so I also refactored parts of that file.

Since MR !54 was out of date, I took the MR's branch and rebased on 2.0.x., then added my changes and pushed the new branch. I created MR !83. I'm not completely clear if this handles every case, but it should get us closer.

Confirming this is still an issue in 2.0.5. Our use case is similar to trackleft2. We are able to work around this for now as we don't have many instances where we use multiple containers.

Version 2.0.7 with fixes in MR 83 shows multiple <iframe src="https://www.googletagmanager.com/ns.html?id=.... but <script src="https://www.googletagmanager.com/gtm.js?id= disappeared completely.

^^ I rerolled again so the patch will apply. But I noticed, we're failing 3 PHPUnit tests now.

I think we'll also need to update js/gtm.js so that there can be > 1 dataLayer name. Right now, everything is going into dataLayer.

So, more work to be done, but patch works again.

I've updated the merge request !83 to fix an outstanding bug in the patch that has to do with cache tag propagation.
Additionally, I've updated the PHPUnit tests and Issue Summary to account for the API changes present in this MR.

I think that I would suggest releasing this in a major or minor version bump.

I've tested this with 2.0.8 and while it gets multiple containers to load, I'm noticing that if there are three IDs within one container it will only output two of them. Without the patch it will output all three but not the multiple containers.

Hm... Back to Needs work then...

Here's what I was testing in 2.0.8 if someone else can confirm:

1. First container with three IDs G-XXX, G-XXX, GTM-XXX
2. Second container with one ID for G-XXX
3. Looking at the page source I see all of the IDs except for the first G-XXX from the first container

I can confirm the MR solves the issue of having multiple containers in the release 2.0.9

The patch from MR to be used in composer.

Updated patch file to be used by composer.

Thanks for the work with this here, it looks to have indeed fixed the issue with multiple containers from my testing.

However, there is a merge conflict with the latest changes in 2.0.x because of the work in #3512549: gtag.js/gtm.js order in D10.4/D11 vs default consent that fixes an issue where gtag.js is always included even if it's not needed (i.e. all the container ID(s) are GTM not gtag) and you can't apply both changes to 2.0.9 release at the moment.

From a quick glance of the already-merged code and the changes in this MR here, I *think* the lines that start attaching google_tag/gtag library etc (line :153) just need to be wrapped with an if ($default_id) {} after the changes here in the MR actually initialise that variable as NULL now on line :125).

I'll try and update this mr from the latest 2.0.x changes and tweak the logic to fix that.

Ok, upon further investigation this needs some more work.

Half way through merging in and there are still bits that need changing to not reference $config outside of a loop on $configs like the advanced settings calls in both google_tag_page_attachments() and google_tag_page_top().

At the moment with the current code in the MR, it'll just grab the advanced settings from the last returned element from the $configs array which isn't correct.

Also when grabbing the consent mode check it's doing the same thing (outside of a loop of $configs) $config->getConsentMode()

I'll try and sort.

Patch attached with the latest changes in the MR as of 16-02-26 which merges cleanly into 2.0.x.

Also very good spot with adding this check:

    if (allowlist.length || blocklist.length) {
      window[dl].push({
        'gtm.allowlist': allowlist,
        'gtm.blocklist': blocklist,
      });
    }

The patch in 31 due to the lack of checking allowlist / blocklist and unconditionally writing to it can completely cause the dataLayer variables to be completely blatted and should *NOT* be used on a production site if you want your GTM to function :)

Using the path provided in #36 I found a problematic scenario.
I need to use two containers, one for GA4 and one for GTM, when the container having GTM configured is loaded after GA4, then GA4 is not added because getDefaultTagId method always return a string, so it always override any previous valid tag.

I refactored the #36 patch to handle this scenario and I added tech details to the MR.
I am more than happy to push the commit with this change, but I don't want to interfere with someone else MR.

Great spot @marco.pagliarulo.

I've taken just the required bit from that patch and merged into MR 83.

For context, as long as the changes are relevant and not a total rewrite of the approach you should be fine to push to a MR - IMO. It's the responsibility of the developer including a MR patch on their site *not* to just reference the MR patch URL and use a local patch or git diff version.

Whilst I was here, I found a few other issues with the change for multiple containers and the allow / blocklist, and the checking of include_classes and include_environment wasn't done per tag, so i've pushed fixes to the MR for that.

Patch attached with the latest MR fixes.

Thanks @rossb89 for including the fix in the MR and the clear explanation on how to contribute to the existing MR.