Cache noscript content and avoid using file_get_contents for better performance

Updated the patch to
* use cache for both script and noscript
* set proper cache tags

removed extra function as this doesn't allow patching older versions.

Thanks for revisiting this topic. Others have suggested using a cache backend for the inline snippets. For the record, Drupal recommends use of a file source for a script snippet. (Google examples always show their snippets as inline but possibly for historical reasons as noted in another issue.)

In any case, I created a separate issue and noted therein the difference in approach from what I see in your patch files. For example, I see no reason to use other than a standard cache bin. The 'reason for creating separate bin' in the issue summary would seem to apply to a standard bin. If mistaken, please clarify.

If possible, please test the latest dev release and comment whether that satisfies this topic.

@Jim

Thanks for considering the approach and pushing a solution for it.

Queries I have:
1. Why create a new ticket when we already have a ticket for it? For a community created problem I would really wait for feedback from community before pushing it.
2. To answer your question about using PCB, cache.data (one used in your commit) is generic and we really don't want the data in it to be stored permanently, we want that to be cleared any-time we run "drush cr". Since this data is not really Drupal Cache and we are using Cache just to allow it to be processed faster we should use a separate cache bin so each site can use their own ways to optimise it, by default it will still be stored in database or default cache mechansim but it is extendible. Now with cache.data either I've to move everything or I can't at all.

@nikunjkotecha In reply:

we want that to be cleared any-time we run "drush cr".

The data in cache_bin is cleared on 'drush cr'. Also, have you noted what else is stored in this bin? Things like aggregated css and js. Are the tag module snippets not just like the other js items in this cache? Is cache_data restricted from being housed in something besides the standard database?

Also, my tests indicate your patch does not create the custom bin, and fails on use for lack of same.

Now with cache.data either I've to move everything or I can't at all.

Is it not true the items in this bin should be refreshed on each code or site deployment? If so, then does this eliminate your concern with the approach?

@Jim

The data in cache_bin is cleared on 'drush cr'. Also, have you noted what else is stored in this bin? Things like aggregated css and js. Are the tag module snippets not just like the other js items in this cache? Is cache_data restricted from being housed in something besides the standard database?

What was done before this ticket was to set the data in filesystem whenever the configuration was updated. With the new approach it is faster to load and write but it is re-calculated when you do "drush cr", this was not there before, correct? With separate cache bin and use of permanent_cache_bin we are trying to achieve the same, re-calculate only when configuration is updated and not when "drush cr" is done.

I suggest you to check the module documentation once to understand more on what we are trying to say here.

Also, my tests indicate your patch does not create the custom bin, and fails on use for lack of same.

Cache missing on the latest patches seems to be a bug, will update once we conclude on the approach

No, not correct. The snippet files have been refreshed on each cache rebuild long before this. So they do not 'persist' in the sense you seem to be looking for. The refresh was added precisely because others, in their deployment procedures, did NOT move or copy the snippet files and wanted an easy way for them to be recreated. The consensus was that all the caches should be rebuilt on each deployment and so doing a refresh of these files made sense. While your PCB may have its place, it is not needed here; there is no harm in refreshing these files or cache items on rebuild. As noted, the aggregated CSS-JS files plus views unpack items are handled this way.

Also, you are introducing a dependency on PCB which is not indicated in your patch, nor justified.

Hi @Jim,

I see the code and thanks for clarifying that out.

My apologies for mixing topics here but it seems there is bug in what was added in https://www.drupal.org/project/google_tag/issues/3114963

I understand we are not using the latest version and we have set the rebuild_snippets configuration to false so for us it is not re-created on "drush cr". Since this is a configuration I think what is introduced in https://git.drupalcode.org/project/google_tag/-/commit/bc23d262f79bf8516... is not respecting the configuration and deleting even if not required.

While your PCB may have its place

It seems you are offended in some way, I apologise if that's the case but it's not about pushing something that I've built. It is more about doing something what feels right. As I said before if the configuration was to not rebuild on hook_rebuild files would have stayed as is and with new code to have same cache bin it would be rebuilt every-time.

As next step IMO we first need to fix the bug introduced in https://www.drupal.org/project/google_tag/issues/3114963 and re-discuss about all the scenarios. In-case you are still convinced to continue with just cache.data I would recommend to remove the configuration and code in hook_rebuild to do it only once.

Yes you getting side-tracked. Open an issue to discuss the changes made with 3114963. Also note your comments do not indicate a bug but rather a feature request to change the behavior. The behavior was developed with community input.

On the topic at hand, as noted in various other issues, most people disagree with you on keeping the snippet items on deployment (your 'permanent' cache approach). Their reasons include deploying from a dev environment which does not have the production container ID and so would deploy an incorrect tag snippet if retained.

On the topic at hand, as noted in various other issues, most people disagree with you on keeping the snippet items on deployment (your 'permanent' cache approach). Their reasons include deploying from a dev environment which does not have the production container ID and so would deploy an incorrect tag snippet if retained.

I'm really not sure what you mean by deployment here? How is the non-prod container deployed on production? Also what Purushotam has suggested is to only use a separate bin so that it can be easily extended, I really don't get what's the issue in that?

What does "extended" refer to in "use a separate bin so that it can be easily extended"? A PHP class that can be extended? Other?

Nope not PHP Class but it's backend, check https://www.drupal.org/node/2754947

I can easily set the backend of my choice for the specific bin.
$settings['cache']['bins']['google_tag'] = 'cache.backend.memcache' or 'cache.backend.permanent_memcache';

New patch provided which works with the current module version.

Some changes were needed in the previous patch to store snippets in filesystem always and to properly fetch data from cache.

+1 RTBC

Have you tested the changes made with issue 3153033? [issue link above in 'Referenced by' block]
The commits are not the same as proposed here but agree with the topic to cache the snippet content.

In light of aforementioned issue and commits, this seems to be a duplicate issue at this point. If I am mistaken, then please provide reasons to continue this thread.