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
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
Comment #2
chetan 11 commentedComment #4
chetan 11 commentedPlease check the above MR.
Comment #5
berdirSeeing 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.
Comment #6
berdirOk, 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.Comment #7
mortona2k commentedMR is working for me.
Comment #8
gobind singh commentedI can also confirm this works for me.
Comment #9
japerryThe 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.
Comment #10
berdirThere'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.
Comment #12
japerryYah, 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.
Comment #13
ishore commentedNo warning messages for me now after latest update (2.0.4), thank you!