Additional test fixes

To be clear, "additional_test_fixes-2464339-test-changes-4.patch" is the patch to commit. We included the other patch to show that this is fixing most of the remaining tests once that part works again.

GoogleAnalyticsSearchTest is the only remaining test that is failing, AFAIK that's actually broken and there's an issue somewhere, so we didn't look into that closely.

+++ b/google_analytics.module
@@ -624,7 +624,7 @@ function _google_analytics_visibility_pages() {
         // Compare the lowercase path alias (if any) and internal path.
-        $path = Url::fromRoute('<current>')->toString();
+        $path = ltrim(\Drupal::service('path.current')->getPath(), '/');

The difference of those two call is the following:

toString() returns an absolute path that includes and subdirectories, so on testbot, $path is not '/admin/something' but '/checkout/admin/something'. You could use getSystemPath(), but that's kind of deprecated.

path.current returns /admin/something, always, and because that is consistent with other patch matches, we also remove the leading /.

1. +++ b/google_analytics.module
   @@ -318,18 +318,16 @@ function google_analytics_page_attachments(array &$page) {
   +      $script = $googleanalytics_adsense_script . $script;
        }
    
   -    $page['#attached']['js'][] = array(
   -      'data' => $script,
   -      'scope' => 'header',
   -      'type' => 'inline',
   -    );
   +    $page['#attached']['html_head'][] = [
   +      [
   +        '#tag' => 'script',
   +        '#value' => $script,
   +      ],
   +      'ga_scripts'
   +    ];
   

   This will not committed. Remove it from the patch, please.

   Core need to add inline JS support. See #2391029: Attaching inline JS no longer possible. Help get this back into core!

2. +++ b/google_analytics.module
   @@ -420,31 +418,6 @@ function google_analytics_cron() {
   - * Implements hook_preprocess_item_list__search_results().
   - *
   - * Collects and adds the number of search results to the head.
   - */
   -//function google_analytics_item_list__search_results(&$variables) {
   -function google_analytics_preprocess_item_list(&$variables) {
   -  $config = \Drupal::config('google_analytics.settings');
   -
   -  // Only run on search results list.
   -  if ($config->get('track.site_search') && $variables['theme_hook_original'] == 'item_list__search_results') {
   -    // There is no search result $variable available that hold the number of items
   -    // found. The variable $variables['items'] has the current page items only.
   -    // But the pager item mumber can tell the number of search results.
   -    global $pager_total_items;
   -
   -    $page['#attached']['js'][] = array(
   -      'data' => 'window.google_analytics_search_results = ' . intval($pager_total_items[0]) . ';',
   -      'type' => 'inline',
   -      'group' => JS_LIBRARY-1,
   -    );
   -    drupal_render($page);
   -  }
   -}
   -
   -/**
   

   Inline JS required and may be the reason why the GoogleAnalyticsSearchTest are failing. This is not fixed by #2256633: Search results counter is missing in search result pages

In GoogleAnalyticsBasicTest changed "allowAnchor":true to "allowAnchor":"1" because it is requested output.

What does this mean? It should be "true" per Google. See https://developers.google.com/analytics/devguides/collection/analyticsjs...

- In google_analytics.settings I changed admin\nadmin/* to /admin\n/admin/* as with previous path it doesn't exclude Google Analytics code from admin pages.

Than the D6 and D7 upgrade need to be fixed, too.

Changed deprecated DRUPAL_AUTHENTICATED_RID to AUTHENTICATED_ROLE.
Reverted back changed paths and modified $path in google_analytics.module to be without forward slash, too.

Is this the way how block visibility works in core?

You looked at the wrong patch, we provided one without the inline js changes, for exactly this reason. It's not possible to test it without that change.

I don't know what causes the string changes.

There is no upgrade path change needed, because that was reverted again in the next patch. And yes, that is how block visibility works too.

+++ b/config/schema/google_analytics.schema.yml
@@ -93,14 +93,31 @@ google_analytics.settings:
+              value_expected:
+                type: integer
+                label: 'Expected value'

That looks strange... we need this values only in the test not in the schema.

Then we need to save the value somewhere else, in state or so.

I committed all the stuff that is safe from my review without testing, but left out the following for clarification

1. value_expected as it should not in the schema. We need to find a better solution than cluttering the schema.
2. The changes to the variables cookieExpires, allowAnchor, sampleRate are not correct. With proper data type this should not happen. Today I can only guess why this has been broken, but I think it happens because of the schema sequence "string" setting. In past core has saved the array with proper data types like written here:

$codesnippet_create = array(
      'cookieDomain' => 'foo.example.com', // string
      'cookieName' => 'myNewName', // string
      'cookieExpires' => 20000, // integer
      'allowAnchor' => TRUE, // boolean
      'sampleRate' => 4.3, // float
    );

3. Url::fromRoute('<current>')->toString(); as I'm not sure if this change is correct
4. Additionally 'Custom metrics' mapping looks wrong. Values can be float and integer and it looks likre core suxxx now if it comes to optional schema settings

3. Yes, that change is correct. I explained the difference in #7. It is required to make the tests work on testbot. You won't see a difference locally unless you install drupal in a subfolder.

4. If it supports float and integer, then the schema needs to define it as float.

#3. I cannot find anything about 'path.current' in block module. I tried to find out how block visibility works, but failed until now. No idea how this works. I guess we need to migrate the functions to the block visibility logic. If someone knows how please share some details.

#4. But these settings are optional and not only this - I don't know what settings Google may add in future. If they are not configured they cannot saved. In past all was saved properly.

I rolled the metric float schema change back as I guess this fail if I use a token. Not tested.

I have no clue how we can have different data type of settings saved in same schema. Sometimes a value can be boolean/integer/float/string. Only the key is string for sure.

Then just set the type to string. If you enter a string through a form, then it is treated as a string anyway, I don't think that was really different in the past.

There's also a type: any or so, that won't do any casting at all. But it will also not ensure that a float stays a float, for example.

If you want to make sure that a value is casted correctly, then the safest option would be to add an explicit integer/float/string setting anyway so that the user can choose. Then you can make sure that the output of a token is always treated as a float, for example.

Ah, cool. I try out "any" than.

I used some tricks in past in form validation function. The validation callbacks casts the string values to correct types and than these no longer need to be casted later on every output.

Cannot find any information about the data types in https://www.drupal.org/node/1905070#types

@mbovan: Do you know how we can solve the datatype issues I noted before?

You can't upload core changes as a patch here. Just include the google analytics changes, preferably a version without the JS changes as well.

1. +++ b/google_analytics.module
   @@ -624,7 +599,7 @@ function _google_analytics_visibility_pages() {
          if ($visibility < 2) {
            // Compare the lowercase path alias (if any) and internal path.
   -        $path = Url::fromRoute('<current>')->toString();
   +        $path = ltrim(\Drupal::service('path.current')->getPath(), '/');
            $path_alias = Unicode::strtolower(\Drupal::service('path.alias_manager')->getAliasByPath($path));
   

   Trying to explain this again:

   The current code returns the absolute path, including leading / and subfolders:

   /some/patch (local)
   /checkout/some/path (on testbot, for example)

   Obviously, this breaks, as the alias manager expects an internal path.

   The new method always returns /some/path. Additionally, we need to remove the leading /, because getAliasByPath() expects just "some/path".

2. +++ b/google_analytics.module
   @@ -654,7 +629,9 @@ function _google_analytics_visibility_pages() {
    
   -  if (($account->id() || \Drupal::config('system.performance')->get('cache.page.use_internal') == 0) && $config->get('privacy.donottrack') && !empty($_SERVER['HTTP_DNT'])) {
   +  // @todo: This is not reliable method to check if page cache is enabled. Look
   +  //   for different solution.
   +  if (($account->id() || !\Drupal::moduleHandler()->moduleExists('page_cache')) && $config->get('privacy.donottrack') && !empty($_SERVER['HTTP_DNT'])) {
   

   I'm not sure about this.

   Basically, D8 doesn't have a reliable method to detect if a patch will be cached. It might be the page_cache module, or it could be an external reverse proxy like varnish. Also, with solutions like smartcache, it is even possible that it will be cached for authenticated users. In that case, cache context might work, though.

   Also, internal page cache is now enabled by default, so this won't work for anonymous users in a default installation.

   I know you're against it and I really don't want to repeat those discussions here, but that's the kind of stuff that you can do reliably in client side JS.

We can

+++ b/google_analytics.module
@@ -56,6 +57,7 @@ function google_analytics_page_attachments(array &$page) {
   $request = \Drupal::request();
+  $page['#cache']['tags'] = Cache::mergeTags(isset($page['#cache']['tags']) ? $page['#cache']['tags'] : [], $config->getCacheTags());
 
   // Get page http status code for visibility filtering.

This is the change that requires the core issue to work, we want to add cache metadata to the page so that when the settings are changed, all cached pages are automatically invalidated.

We can try to extract those changes in separate issues, or separate patches if you prefer that.

1. +++ b/google_analytics.module
   @@ -56,6 +57,7 @@ function google_analytics_page_attachments(array &$page) {
   +  $page['#cache']['tags'] = Cache::mergeTags(isset($page['#cache']['tags']) ? $page['#cache']['tags'] : [], $config->getCacheTags());
   

   What is this line doing?

2. +++ b/google_analytics.module
   @@ -654,7 +656,9 @@ function _google_analytics_visibility_pages() {
   -  if (($account->id() || \Drupal::config('system.performance')->get('cache.page.use_internal') == 0) && $config->get('privacy.donottrack') && !empty($_SERVER['HTTP_DNT'])) {
   +  // @todo: This is not reliable method to check if page cache is enabled. Look
   +  //   for different solution.
   +  if (($account->id() || !\Drupal::moduleHandler()->moduleExists('page_cache')) && $config->get('privacy.donottrack') && !empty($_SERVER['HTTP_DNT'])) {
   

   Before this goes in we need to find a reliable fix.

3. +++ b/src/Tests/GoogleAnalyticsBasicTest.php
   @@ -282,7 +275,7 @@ class GoogleAnalyticsBasicTest extends WebTestBase {
   -    $this->assertRaw('ga("create", "' . $ua_code . '", {"cookieDomain":"foo.example.com","cookieName":"myNewName","cookieExpires":20000,"allowAnchor":true,"sampleRate":4.3});', '[testGoogleAnalyticsTrackingCode]: Create only fields have been found.');
   +    $this->assertRaw('ga("create", "' . $ua_code . '", {"cookieDomain":"foo.example.com","cookieName":"myNewName","cookieExpires":"20000","allowAnchor":"1","sampleRate":"4.3"});', '[testGoogleAnalyticsTrackingCode]: Create only fields have been found.');
   

   Again, this is an incorrect change! The data types are incorrect. We need to solve the underlying issue that this was an settings array before with correct data types and now with the change to sequence shit we have a data type string what is NOT correct.

1. Explanation is in the last chapter in #29.

2. See #29.2. There is no reliable way. This is enough to make the tests work again and we can commit it like this, but long term, the only thing we can do is remove the setting and either check this in JS or tell users to not enable this when page cache is enabled. But actually, that's *not* really an option, everyone wants to have that enabled.

@Bedir:

The current code returns the absolute path, including leading / and subfolders:

/some/patch (local)
/checkout/some/path (on testbot, for example)

Obviously, this breaks, as the alias manager expects an internal path.

Sure I know. Is there any case that removes this inconsistency soon? I'd like to wait until the inconsistency may be fixed. I remember there have been some cases around for this at least for ckeditor is one that is open and maybe others I'm not aware of.

+ if (($account->id() || !\Drupal::moduleHandler()->moduleExists('page_cache')) && $config->get('privacy.donottrack') && !empty($_SERVER['HTTP_DNT'])) {

Well, this is bad code in D7, too. If there is a way how I get the 'HTTP_DNT' on cached pages and I guess we can do this in D8 better than in D7 as not everything is cached. I'm happy to get rid of the limitation that 'HTTP_DNT' only works if caching is disabled. I hope this code do not need to exist in D8 any longer. We need to be able to remove the tracking code if the header 'HTTP_DNT' is send from the client. This is not possible under D7.

FabianX told me in #2474353: Cannot replace placeholders with #post_render_cache about '#post_render_cache' and placeholders. I hoped it could also solve the issue, but it does not allow me to replace a placeholder in header region. Maybe you can join there and help, too.

We cannot directly see the 'HTTP_DNT' HTTP header in JS... this a server level header only. If this need to be done on JS level we also need a server level helper.

Sure I know. Is there any case that removes this inconsistency soon? I'd like to wait until the inconsistency may be fixed. I remember there have been some cases around for this at least for ckeditor is one that is open and maybe others I'm not aware of.

I don't think this will go away. I think it we should have made it so that it returns it without the leading /, but the problem is that 50% of the subsystems expect a leading / (like the routing system) and the other half doesn't (like the path alias system). That's mostly for historic reasons, in 9.x, we might change it so that everything uses leading /.

DNT:
The issue you referenced doesn't help, that's just for render caching. The page cache happens earlier, and very explicitly, we do not support any kind of post render or context handling there. Because pretty much every microsecond there counts for performance. And it also wouldn't work if you're caching using varnish, for example, although you could configure varnish to cache differently based on the existence of that header.

You *can* get code to run before the page cache in a relatively fast way, by adding a middleware. And then for example adding something to the response.

But are you sure that you can't read the value in JS? Have a look at http://stackoverflow.com/questions/16947459/is-it-possible-to-check-the-... for example, most browser seem to have an API for that...

@bedir: I hoped to get rid of _google_analytics_visibility_pages() and _google_analytics_visibility_roles() functions and just be able to reuse the block plugins that handles visibility. I guess they are now pluggable and can be reused better than in past. Thereby I may get the burden to maintain this logic away from the module. I just have no idea where it is and how to use it.

You could use condition plugins, but note that you still have to manage and store the configuration for them yourself. The main advantage is that you could support all available condition plugins in a generic way. But you'd only save a handful of lines and first need to write some code on handling conditions, which is likely considerably more than you have right now.

You could look at how page_manager uses them for the visibility conditions.

Also, not really related to this issue, we're not touching them here...

That sounds not good. I hoped to call a function, provide the paths and it tells me true/false. Developing every condition plugin agsin and again may introduce bugs and is like redevelop the wheel. That suxxx.

Thanks. Adding https://www.drupal.org/developing/api/8/cache/contexts as it explains the context names. Funny... I guess I have user.roles and user.permissions and headers context may be the one for DNT? And not to forget the many dependencies.

That cache stuff will introduce a lot of new bugs in D8 I'm sure.

With https://www.drupal.org/node/1884800 I do not understand the cache "tag" change at all you proposed here.

That page is not relevant, that is the actual cache tags API, that does not explain when to use them.

The cacheability documentation contains this

"What causes the representation of the thing I'm rendering become outdated?"
I.e. which things does it depend upon, so that when those things change, so should my representation? Those are the cache tags.

Changing the settings results in those things to become outdated, so that's why we need to add a cache tag for them, just like I said in #29. Each config object automatically has a cache tag, and we add that, because by changing those settings, we might output different data.

Yes, it probably needs additional cache tags and context as well, but those do not currently have test coverage, so I think it's better to open a separate issue to complete that. (user.roles makes sense, user.permissions AFAIK not so much, because there are no permissions that control if google analytics is shown or not?, also cache tags/context based on e.g. the node also makes sense). Note that unlike the problems outlined here, that information is currently not actually cached, so it will not be possible to write failing tests. Only the addition of the smart cache will result in that.

Each config object automatically has a cache tag, and we add that, because by changing those settings, we might output different data.

Correct, but we also have role dependencies for users. And if a user may become a member of a excluded role it also need to be refreshed for this user.

(user.roles makes sense, user.permissions AFAIK not so much, because there are no permissions that control if google analytics is shown or not?

Sure we have that... visibility by role and we have these user setting to unsubscribe from tracking. That means it requires a cache update if a user setting has changed.

Yes, that's correct, but this hook is currently only cached as part of the page cache and not (yet) for authenticated users.

So while that should be added (with tests), nothing is actually broken right now, so I don't see a reason to hold up these fixes. Then we can open a follow-up issue.

1. +++ b/src/Tests/GoogleAnalyticsBasicTest.php
   @@ -121,19 +121,12 @@ class GoogleAnalyticsBasicTest extends WebTestBase {
        // DNT works only with system internal page cache disabled.
   -    $this->config('system.performance')
   -      ->set('cache.page.use_internal', 0)
   -      ->set('cache.page.max_age', 0)
   -      ->save();
   +    \Drupal::service('module_installer')->uninstall(['page_cache']);
   

   We should better fix DNT to work correctly than fixing any test failures of code that that has no future.

2. +++ b/src/Tests/GoogleAnalyticsBasicTest.php
   @@ -282,7 +275,7 @@ class GoogleAnalyticsBasicTest extends WebTestBase {
   -    $this->assertRaw('ga("create", "' . $ua_code . '", {"cookieDomain":"foo.example.com","cookieName":"myNewName","cookieExpires":20000,"allowAnchor":true,"sampleRate":4.3});', '[testGoogleAnalyticsTrackingCode]: Create only fields have been found.');
   +    $this->assertRaw('ga("create", "' . $ua_code . '", {"cookieDomain":"foo.example.com","cookieName":"myNewName","cookieExpires":"20000","allowAnchor":"1","sampleRate":"4.3"});', '[testGoogleAnalyticsTrackingCode]: Create only fields have been found.');
   

   As noted several times, this is an invalid change. The bug is in the save function and sequence definition of the variables. Remove this change from the test, please!

I figured out a way to tell Core to ignore the data types by defining class: '\Drupal\Core\Config\Schema\Ignore' in Create only fields schema. It seems working. Not sure what '\Drupal\Core\Config\Schema\Undefined' was made for.

This fixes the data type conversion bugs from #51/2.

I do not understand the caching code you guys have written in last patches, but below works for me and seems to be the way to go. I guess I need to add more dependencies like user and header for DNT.

With this change custom dimension tests complete without any failure.

    $page['#attached']['html_head'][] = [
      [
        '#tag' => 'script',
        '#value' => $script,
      ],
      'ga_scripts'
    ];
    // If module configuration changes the cache need to be refreshed.
    // @todo: Core documentation is highly confusing and unclear.
    \Drupal::service('renderer')->addCacheableDependency($page, $config);

Why not use "type: ignore" ?

Your code does essentially the same thing internally. It adds a cache tag for the config object. So feel free to use that instead of setting it directly.

However, it's important to add it *outside* of conditionals. You also need to be able to invalidate pages that previously didn't add the GA script when you change settings. If this fixes the tests, then this just means that you have no test scenarios for going to a page that did not show anything and then change configuration to do show something.

And for #51.1: As discussed above in detail, fixing DNT properly for page cache means we need to completely refactor how this works, the only solution that I see is still that we need to do it in JS. This change is just adapting the move from page cache being configuration to it being a module, it already has the same problems right now.

Our goal for this issue was to resolve all problems that are not directly tied to #2391029: Attaching inline JS no longer possible. Help get this back into core!, so we can get back to that.

Why not use "type: ignore" ?

Oh - if this works... The documentation at https://www.drupal.org/node/1905070 may confused me.

However, it's important to add it *outside* of conditionals. You also need to be able to invalidate pages that previously didn't add the GA script when you change settings.

Makes sense. This caching logics suxxx so much.

If this fixes the tests, then this just means that you have no test scenarios for going to a page that did not show anything and then change configuration to do show something.

Maybe.

And for #51.1: As discussed above in detail, fixing DNT properly for page cache means we need to completely refactor how this works, the only solution that I see is still that we need to do it in JS. This change is just adapting the move from page cache being configuration to it being a module, it already has the same problems right now.

Why can we not use context header DNT? Maybe core need to allow this.

I tend to say caching GA code makes no sense in 99% of all cases. So it may be useful to switch caching completely off as it may changes on every page. Otherwise we require many many conditions and until now I do not understand the caching and what tags are and maxage is used for and how to make the conditionals working. If we set maxage = 0 on the render array, will this disable all the suxxx caching issues?

Not sure if we can add our own caching handlers in modules e.g. look for the DNT header and kill the cache for this users than. I have no idea how this may collide on anonymous user as one anonymous may has DNT and the other not. DNT is just an example - is is much more complex as we required per page visibility, per path visibility and so on and all this require cache conditions if I understood it properly. And maybe core does not provide the ability to implement this.

As it is not documented - the caching is defined per render array - is this correct? So if the GA render array has caching disabled, is this really possible or will such a disabled cache - render the full page non-cached?

I really really do not understand the not well documented cache stuff and if someone may write better documentation to explain with complex dependencies it would be extreme helpful I think. We also need documentation that explains what is not possible (typical use cases like here). Maybe we could have a talk by phone otherwise. I already wasted a week on caching and still have no idea how this works.

This is *not* about caching google analytics on it's own.

I tend to say caching GA code makes no sense in 99% of all cases. So it may be useful to switch caching completely off as it may changes on every page. Otherwise we require many many conditions and until now I do not understand the caching and what tags are and maxage is used for and how to make the conditionals working. If we set maxage = 0 on the render array, will this disable all the suxxx caching issues?

The documentation is very clear about this IMHo.

Read "The thought process" again on https://www.drupal.org/developing/api/8/render/arrays/cacheability. To quote the relevant parts:

Is this something that's expensive to render, and therefore is worth caching?
If the answer is "yes", then what identifies this particular representation of the thing I'm rendering? Those are the cache keys.

Cache contexts, tags and max-age must always be set, because they affect the cacheability of the entire response. Therefore they "bubble": parents automatically receive them.

Cache keys must only be set if the render array should be cached.

Yes, it doesn't make sense to cache the google analytics information separately. We do not set cache keys, so it does not get cached.

However, it can still get cached elsewhere, mostly about the page cache. And we need to add cache tags/context so that the relevant pages in the *page cache* are invalidated when settings change. The page cache is already per URL, obviously.

Disabling page caching for the DNT header is *not* an option. And it's not just an example, everything else works fine, because it is cached per URL. You need to generate the javascript so that it checks the DNT configuration, per my link in #35. Then you can almost completely forget about it on the PHP side, just need to make sure to generate the JS if the setting is enabled.

If you don't understand the documentation, which is clear to me, then maybe the slides of my presentation help: http://slides.com/saschagrossenbacher/render-caching

We do not set cache keys, so it does not get cached.

I learned that this may not true. In reCAPTCHA module my form was cached, but I have not set this to be cached. That is why caching confuses me so much. It looks like Drupal 8 is caching in an unpredictable way any random stuff and I need to tell the code - NO do not cache - or to refresh the cache on configuration changes. Same for GA. It is cached, but I have not set any cache tags. Why the hell is something cached than if you say it will not if I do not say to do so.

However, it can still get cached elsewhere, mostly about the page cache. And we need to add cache tags/context so that the relevant pages in the *page cache* are invalidated when settings change. The page cache is already per URL, obviously.

Oh. This may explain the point before.

everything else works fine, because it is cached per URL.

We cannot cache per URL... in case we cache only per URL the content is always the same for all users. This is not true for GA. Every user has different tracking code. Just think about userID feature. In this case the tracking code is always different. If we have metrics and dimensions and someone has a token configured the JS tracking code will change on every page. As the JS code is part of the page cache we may be lost?

How can we implement this if the page is cached? This is not something we can handle with JS like it may be possible with DNT.

The page cache is only for anonymous users. You can't identify them and you can't have different metrics/dimensions for them.

This is not a D8 problem. The basic page cache mechanics are exactly the same as in 7.x. What breaks the tests is just the fact that it is now enabled *by default*. Among other reasons, to force modules to think about how the page cache affects them, and that they need to invalidate when they change the output.

Smartcache would be different, but that doesn't exist yet and we don't have to worry about it.

Looks like we are blocked a bit by #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

No, we are not.

This hook is not cached anywhere *except* in the anonymous page cache in HEAD. It's exactly the same as in 7.x, except we have better tools now deal with invalidating caches.

This issue will try to cache it as well, correct, but that isn't committed yet. When it will be, then this will get more complicated, but for now, it's not blocking anything.

Should we waste our time now and rewrite again in a few weeks just because smart cache got in? We could also wait and change code than!?