Problem/Motivation

This issue has been closed as duplicate. The discussion is moved to #1813014: Dynamically switch visibility setting.

Some module might not want to add the google_analytics JS code, like for example in the AMP pages.
There is already an existing issue and this is a reference issue for #2731279: Add google_analytics hook to skip default GA integration for AMP pages.

Proposed resolution

add \Drupal::moduleHandler()->alter('google_analytics_visibility_pages', $page_match); in _google_analytics_visibility_pages()

Remaining tasks

User interface changes

API changes

Data model changes

Comments

tduong created an issue. See original summary.

tduong’s picture

Assigned: Unassigned » tduong
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new376 bytes

Status: Needs review » Needs work

The last submitted patch, 2: add_alter_hook_to-2733291-2.patch, failed testing.

tduong’s picture

Status: Needs work » Needs review
StatusFileSize
new1.36 KB
new1.52 KB
berdir’s picture

Status: Needs review » Needs work
  1. +++ b/google_analytics.module
    @@ -642,6 +642,7 @@
    +    // Allow modules to define if the google_analytics JS code should be added.
         \Drupal::moduleHandler()->alter('google_analytics_visibility_pages', $page_match);
    

    to define if => to alter whether

  2. +++ b/google_analytics.api.php
    @@ -0,0 +1,36 @@
    + * A module may not want to add the GoogleAnalytics JS code on the current page
    + * and set $page_match to FALSE.
    + * This hook is called to allow modules to modify the $page_match boolean by
    + * reference, so the interaction of multiple visibility check page modules can
    + * be altered.
    + *
    + * The resulting grants are then checked against the active route to determine
    + * if the operation may be completed.
    + *
    

    This is a bit strange.

    The part about multiple grants/modules can be removed, that's from the example you copied and doesn't apply here.

    Not sure about GoogleAnalytics, is that the same elswewhere? I would expect "Google Analytics module"

  3. +++ b/google_analytics.api.php
    @@ -0,0 +1,36 @@
    +  if (!\Drupal::service('path.current')->getPath()) {
    +    $page_match = FALSE;
    

    This is a strange example, getPath() returns a string, not a boolean, and it always returns something. Do == '/my/special/page' instead or something like that.

tduong’s picture

Status: Needs work » Needs review
StatusFileSize
new1.82 KB
new1.25 KB

Done.

hass’s picture

I'm wondering why we do not integrate Google Analytics for AMP into the module and add another condition for themes.

berdir’s picture

We could also do a theme condition, want us to create an issue for that?

Adding more hardcoded conditions is also not so great, though.

We'll close this anyway and update #1813014: Dynamically switch visibility setting as that one is older. I still think there are valid use cases for this.

hass’s picture

I'm wondering how you could know better than the ga code if a code should be shown. It is quite complex as there are so many conditions and with this alter you have no idea why ga module decided to disable it and there is no priority. Maybe the PHP code has disabled it and the alter enables it than. You only get a TRUE/FALSE and still have no context about the "why". Not sure if this is really important, but it could be.

I have seen there is an url param amp. That may be easier than the theme to integrate into the path field.

I still have no idea why this alter is required for any other things. Have you also tried using the library remove feature? I named all unique. That is like an alter hook, but does not require extra code in ga.

I'm open to discuss what is the best solution.

berdir’s picture

Knowing/conflicts is a problem of the alter hook.

In case of AMP, for example, you *never* want to show it. AMP has its own google analytics integration (amp.module has basic integration for that. An google recommends to user a different property for it, so it actually makes sense to "duplicate" that).

Path matching doesn't support query arguments, you'd have to write your own implementation.

It's not about getting rid of the library but the inline JS, I don't think libray remove would help there? You could alter it out but then that's pointless processing that GA has to do. Better to prevent it from running.

tduong’s picture

Issue summary: View changes
Status: Needs review » Closed (duplicate)
Related issues: +#1813014: Dynamically switch visibility setting

Closing this issue as duplicated. Discussion moved to #1813014: Dynamically switch visibility setting.