Problem/Motivation

After upgrading to Google Tag 2.0.3 today, I see the following warnings either on the Front Page or under admin/config/services/google-tag:

Warning: Trying to access array offset on value of type null in Drupal\google_tag\Entity\TagContainer->getConsentMode() (line 311 of /srv/www/web/modules/contrib/google_tag/src/Entity/TagContainer.php)

Warning: Undefined array key "gtm" in Drupal\google_tag\Form\TagContainerForm->validateGtmFormValues() (line 120 of modules/contrib/google_tag/src/Form/GoogleTagManagerSettingsTrait.php).
Warning: Undefined array key "gtm" in Drupal\google_tag\Form\TagContainerForm->validateGtmFormValues() (line 124 of modules/contrib/google_tag/src/Form/GoogleTagManagerSettingsTrait.php).
Warning: foreach() argument must be of type array|object, null given in Drupal\google_tag\Form\TagContainerForm->validateGtmFormValues() (line 124 of modules/contrib/google_tag/src/Form/GoogleTagManagerSettingsTrait.php).

We are running Drupal 10 with PHP 8.2.

Issue fork google_tag-3423718

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

ishore created an issue. See original summary.

chetan 11’s picture

Assigned: Unassigned » chetan 11

chetan 11’s picture

Assigned: chetan 11 » Unassigned
Status: Active » Needs review

Please check the above MR.

berdir’s picture

Status: Needs review » Needs work

Seeing this too.

This doesn't seem to adress the warning in TagContainer->getConsentMode(), which may very well go away after resaving the settings, but there should either be an update function or runtime logic to handle the setting not being there.

berdir’s picture

Status: Needs work » Needs review

Ok, TagContainer is an operator precedence issue, ternary operations are weird like this: https://3v4l.org/b9Rt5

Fixed that, but I think this is still a bit of an issue, because existing sites will now implicitly default to TRUE as there is no upgrade path to keep the existing behavior. Maybe there should be a post update like google_tag_post_update_move_advanced_settings() that initializes it to FALSE.

Reading the change in validateGtmFormValues() again, I think it's valid, still think it might not be necessary to be this elaborate. $form_state->getValue() has second argument that could be set to an array and then it could use foreach ($advanced_values['gtm'] ?? [] and drop the condition there entirely, but I don't know about the changes to the loop, pretty sure those are _not_ necessary, but leaving for now.

mortona2k’s picture

MR is working for me.

gobind singh’s picture

I can also confirm this works for me.

japerry’s picture

The reason why its set to TRUE by default is because Google asked it to be the default for all tags. Originally, we weren't going to make it even configurable, but after consulting with the Google engineering team, they said there were some cases where it'd be good to disable it.

Thats also why there isn't an upgrade hook, because if its null (existing site), we default it to true.

berdir’s picture

There's a difference between being default for new installations (which I fully agree with this makes sense) and changing the behavior for existing sites (in a patch release). I'm not even sure what it does exactly, but I suspect I'll have to look into it with some of our clients that have their own teams that manage their tag manager and configure things in combination with third party cookie banners.

Either way, the behavior here isn't changed, this is just fixing the undefined array key warning.

japerry’s picture

Status: Needs review » Fixed

Yah, the MR looks right. Committed!

Regarding True vs False: The problem with existing sites is that if its not set to true (in most cases), their sites will stop (or dramatically reduce) reporting on March 6th. So having an upgrade path that sets the setting to false for existing sites would put their configuration into a non-desirable state, and they may not know which setting they have to explicitly set.

ishore’s picture

No warning messages for me now after latest update (2.0.4), thank you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.