how to ignore condition plugins that have no context to snippet insertion

Created on 24 April 2024, 2 months ago
Updated 11 May 2024, about 2 months ago

Hello,

It looks like the newly-added hostname container parameter from #3241123 โœจ Support Server-Side GTM RTBC isn't always being correctly set to www.googletagmanager.com for some existing containers, in google_tag_update_8104().

This is the deployment log output showing that google_tag_update_8104() has run:

    Running Drupal updates (if any available)...
     ------------------- ----------- --------------- ----------------------------
      Module              Update ID   Type            Description
     ------------------- ----------- --------------- ----------------------------
      google_tag          8104        hook_update_n   8104 - Add hostname to the
                                                      default container settings
                                                      and all containers.
     ------------------- ----------- --------------- ----------------------------

     // Do you wish to run the specified pending updates?: yes.

    W: >  [notice] Update started: google_tag_update_8104
    W: >  [notice] Add www.googletagmanager.com as the default hostname and the hostname for each container.
    W: >  [notice] Update completed: google_tag_update_8104

However, the hostname is empty:

This is causing the tag to silently stop loading; it looks like we have lost a month of business-critical analytics data due to this bug :(

A workaround seems to be to reset the schema version and re-run updb:

drush ev "\Drupal::service('update.update_hook_registry')->setInstalledVersion('google_tag', 8103);"
drush updb -y

This produced the same output as above but this time the hostname is filled in.

Edit to add: despite all appearing correct now, the Google Tag is still not loading. It seems the access check $this->access('view') at line 420 of container.php is returning false - even for UID 1 - and the snippet isn't inserted.

Stepping in to the access check, I can see that the access denied result has the following reason message:

One of the container insertion conditions ('rules_data_comparison', 'rules_data_is_empty', 'rules_entity_is_new', 'rules_node_is_promoted', 'rules_node_is_published', 'rules_node_is_sticky', 'rules_user_is_blocked') denied access.

... originating from web/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php line 81 (Drupal 9):

    if (($return = $this->getCache($cid, $operation, $langcode, $account)) !== NULL) {
      // Cache hit, no work necessary.
      return $return_as_object ? $return : $return->isAllowed();
    }

These screenshots of the debugger might provide a little more information:

https://www.drupal.org/files/issues/2024-04-25/Screenshot%202024-04-25%20at%2011.57.14.png โ†’

https://www.drupal.org/files/issues/2024-04-25/Screenshot%202024-04-25%20at%2011.57.32.png โ†’

Alex :-(

๐Ÿ’ฌ Support request
Status

Fixed

Version

1.0

Component

Code

Created by

๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexharries

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @alexharries
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexharries
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexharries
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexharries
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexharries
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States solotandem

    I cannot reproduce what you claim. After running update 8104 on a very plain drupal site all is well. My guess is the undesirable outcome you describe has something to do with your site configuration or deployment procedures. The former may include other modules whose code interferes in some way.

    While you may have a concern, the issue summary is lacking and fails to comply with the community bug reporting guidelines. You have indicated an undesirable response but not uncovered (or disclosed) the actual cause (in particular you have not shown the response to be the result of the code in this project). If you can please update the summary. Until then let's change the issue type to support request.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexharries

    Thatโ€™s ok; weโ€™re just going to stop using your module on all our sites (-:

    All the best,

    Alex

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States solotandem

    yes, you did provide a detailed description of what you observed (along with harsh and false accusations) and, by deduction, missed. from that i replied with items intended to help you get the desired outcome.

    no, the tone of my response was not dismissive, but simply factual. in the first paragraph, i refute your claim. in the second, i justify the issue status change (probably what you find as dismissive). in the third paragraph, i offer a possible reason for the undesirable outcome (hinting at a solution).

    taking a step backward, this is really two issues: update hook and snippet insertion, with the second one being the persistent problem that you need support for. the second issue is clear from the direction change you took after managing to get the update hook to stick.

    from what you wrote the update hook did run and was successful. but then something in your deployment workflow (e.g. config override) reverted the changes from the update hook. (the latter you did not notice as you have not documented it and would have omitted it from the issue if you had.) this is evident from your description of steps to revert the module update value to 8103 and rerun the hook, all using drush. as the update hook succeeded here so it did on the initial run of the update hook (as i attest to in first paragraph of my previous comment).

    then we get to the problem of container visibility and snippet insertion. in its ContainerAccessControlHandler class, the tag module overrides the core processing of condition plugins. if it did not, then most likely no one would have snippets on a page. it changes the incorrect core behavior of returning false if any of the plugins does not have context. given that block visibility is the only use case of condition plugins in core, this logic, while incorrect, is benign because the default is to deny the block visibility and so not display it. with block placement you need to affirmatively set a condition to display a block, so the default visibility condition being false is acceptable.

    with the tag module the default visibility should be true unless you declare a reason to hide the snippets. the condition plugins provided by the tag nodule (that override their core counterparts) do just that; their default out-of-the-box value returns true (for non-admin paths). condition plugins that have no context should not be present to be evaluated and certainly should not render the final verdict.

    apparently this is not the case with the rules plugins, some of which will have context on the pages you are wanting to gather analytics.

    after noticing that the update hook was overridden (that is not what you noticed but that is what happened), why is it you did not immediately notice the snippets not being added to the page? how is it you dare to blame me for this? rather are you not negligent in this regard?

    if you would read the code in this project, you might notice that it implements core hook_plugin_filter_TYPE_alter, to remove some of the condition plugins defined by core and a contributed module (as they do not behave well). this hook is available to you to filter the condition plugins defined by rules. until core properly implements condition plugins, everyone probably needs to implement this hook.

    lastly, is it 'particularly constructive or in the spirit of the Drupal community' for you to lash out at me with ad hominem remarks:

    • loudly (with an issue status of critical) accusing me of something you can not prove (in disregard of community guidelines)? as i have written elsewhere, if you are not able to provide the necessary evidence, then do not tag an issue as a bug (and certainly not critical).
    • and, without justification, blaming me for you having 'lost a month of business-critical analytics data'? rather, are you not negligent for not noticing this much sooner?
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom alexharries

    Solotandem, in the kindest-possible way, I think you need to take a step back and stop taking this as a personal attack.

    Youโ€™ve proven yourself very good at writing posts containing wildly-excessive accusations.

    If this was the first time in this issue queue that youโ€™d been rude, Iโ€™d be prepared to believe that Iโ€™m the problem here.

    But itโ€™s not the first time, and - respectfully - Iโ€™m not the problem here.

    :)

  • Status changed to Fixed 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States solotandem

    More ad hominem.

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

Production build 0.69.0 2024