8000 Google Tag Manager by tijsverkoyen · Pull Request #3047 · forkcms/forkcms · GitHub
[go: up one dir, main page]

Skip to content

Google Tag Manager#3047

Merged
carakas merged 14 commits intomasterfrom
google-tagmanager
Aug 12, 2020
Merged

Google Tag Manager#3047
carakas merged 14 commits intomasterfrom
google-tagmanager

Conversation

@tijsverkoyen
Copy link
Member

Type

  • Enhancement
  • Feature

Pull request description

New Google Analytics options

We removed automatically adding Google Analytics code if you link the Analytics
module. From now on you have two options to let Fork add Google tracking:

Interface for setting the Google ID's

You can select them both, but that is not something I would recommend. In this case both the Google Analytics and Google Tag Manager code will be added.

Google Analytics

If you don't want to use Google Tag Manager you can just set the Tracking ID for Google Analytics. With this we will add the Google Analytics code directly.

Google Tag Manager

Google Tag Manager is a very powerful product from Google where you can manage
everything yourself, or allow external users access to manager the scripts
for you. With Google Tag Manager you don't need a Fork CMS developer to add
scripts for you.

If you want to add Google Tag Manager to your Fork CMS website you need to fill
in the Google Tag Manager Container Id in the Settings screen under Google Tag
Manager Container Id. You can find this Id by entering Google Tag Manager, in the
overview it is listed under Container ID.

anonymize IP

If you are targeting users in the EU, you have probably heard of GDPR. GDPR is all
about protecting the personal data of people.

Google has made it possible to anonimize the IP's that are send, so by default we
will anonimize them.1 If the user allows it we will not anonymize them anymore. At the moment this happens as they agree on the current Cookie Bar implementation.

To make this possible we need to configure a variable in Google Tag Manager:

  • Open Variables
  • Click "New" next to "User-Defined Variables"
  • Name it "DataLayer - anonymizeIp"
  • Choose "Data Layer Variable" for the type
  • Use "anonymizeIp" as the value for "Data Layer Variable Name"
  • Enable "Set Default Value", and set the Default Value to "true"

Configuration of the anonymizeIp variable

1: In Belgium this is not enough, the Belgian DPA has said that anonimyzeIp is not
enough. So if you are a Belgian company you should ask the visitor permision before loading
Google Analytics.

Google Analytics

The next step is to add Google Analytics thru Google Tag Manager.

Configure a variable which contains the Tracking ID, this is optional but advised

  • Open Variables
  • Click "New" next to "User-Defined Variables"
  • Name it "GA - Tracking ID"
  • Choose "Google Analytics setting" as the type
  • Enter the Tracking ID into the correct field, it should look like UA-xxxxxxx-y

Add the Universal Analytics

  • Open Tags
  • Click "New"
  • Choose "Google Analytics: Universal Analytics" as the type
  • Choose the "GA - Tracking ID" variable for Google Analytics settings
  • Enable "Enable overriding settings in this tag"
  • Leave "Tracking ID" empty
  • Click "More settings" --> "Field to Set"
  • Add the field "anonymizeIp", and set the value to the variable DataLayer - anonymizeIp.
  • Choose "All pages" for the trigger

Don't forget to publish the changes.

The module setting will fallback to `site_html_header` as that was the name before.
While at it I fixed the labels as they used hardcoded text or displayed faulty output.
The module setting will fallback to `site_html_footer` as that was the name before.
While at it I fixed the labels as they used hardcoded text or displayed faulty output.
It seems like this settings was already available but not displayed in the backend :s
Sorry, I copy/pasted it wrong.
I think I wrote it more readable:

* If Google Tag Manager is added thru the backend we expect Google Analytics to be loaded from within Google Tag Manager
* If the web property is empty we can't build a correct code therefore it should not be added
* If the web property is present in any of the site wide HTML than we should not add it.
gtag.js is
8000
 the current way to add Google Analytics to a site.

See https://support.google.com/analytics/answer/7538414
The Analytics module should not be responsible for adding the Google Analytics
Tracking Code.
As we now have a separate setting we use that one to decide if the code should be added.
It should be as easy as:

	$container->get(ForkCMS\Google\TagManager\DataLayer::class)->add('key', $value);

We will add the anonimizeIp variable by default
@tijsverkoyen tijsverkoyen requested a review from a team as a code owner February 24, 2020 15:41
@carakas
Copy link
Member
carakas commented Feb 24, 2020

@tijsverkoyen Would it make sense to deprecate the analytics module in Fork after this PR is merged since the whole setup has moved somewhere else? it shows some graphs but google analytics shows them better and you can do more with it.

@tijsverkoyen
Copy link
Member Author

I think the integrated analytics module has it value for a lot of people who are not that experienced in Google Analytics. On the other hand it would mean that there is a lot of code not to be maintained anymore.
Maybe ask the opinion on this in the (Slack) community?

8000

@bart-webleads
Copy link
Contributor

We (webleads) never load Google Analytics directly.
We always use the Google Tag Manager.

So I would say, remove the GA code from ForkCMS.

@jacob-v-dam
Copy link
Contributor

I think removing GA could be ok. But for the time being add both to Fork until version 6.0. If you remove GA a good manual should be available on how to add GA in combination with GTM.

Somewhere else I noticed a discussion about the graphs in Fork, if these should stay or be removed. At first I thought remove them from Fork. A week ago the api or something had an error this resulted in a 500 error, and users where unable to login.

But on the other hand users look at these graphs since GA is sometimes to complicated. If users get more used to GA they prefer this above the Fork stats.

@tijsverkoyen
Copy link
Member Author

I think there are several questions to be answered:

1. Should we remove the Analytics module.
As with this PR the Google Analytics code won't be added automatically anymore. The Analytics module uses the Google Analytics API and that API is prune to change, it means maintaining code.
Jelmers argument is that Google Analytics has a better interface, more features, ... The current Analytics module is a very simple interface which only allows the admin to see very little and basic information.

2. Should we provide the option to choose between Google Analytics and Google Tag Manager
This PR provides the user with 3 options:

  1. Use the scripts-field to add code yourself
  2. Use Google Analytics
  3. Use Google Tag manager

3. Some question about the removal of graphs
See, the comment from Jacob: #3047 (comment)

My personal opinion:

1: I think the current Analytics has value for non-professional users or users that only need basic stats. At this moment I would vote against removing the module.
2: The user should be able to choose, with the 3 options you can anything you want. Google Tag Manager is complicated if you don't are experienced with it.
3: Not relevant for this PR, so if you want to discuss this: create a PR/Issue.

@carakas carakas modified the milestones: 5.8.0, 5.9.0 Mar 3, 2020
@carakas
Copy link
Member
carakas commented Jul 31, 2020

I would keep all three options as well. I find the arguments of @tijsverkoyen convincing.
Is there still a lot of work that needs to be done for this PR @tijsverkoyen? Any idea on a timeline?
I think it might be a good idea to release 5.9.0 ones this one and #3048 are finished if it is in the near future

@tijsverkoyen tijsverkoyen changed the title [WIP] Google Tag Manager Google Tag Manager Aug 5, 2020
@tijsverkoyen
Copy link
Member Author

@carakas For this PR there is no work left. For #3048 I need to review the comments again and check to finish up the code, I will try to finish it this month.

Let me know if you merge these PR's, in that case I will create a blogpost based on the PR descriptions, so we can use it as a reference/docs for these features.

}

// assign site wide html
$this->template->assignGlobal('siteHTMLFooter', $siteHTMLFooter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now I would assign it to the both of them for BC reasons. There are people that upgrade to the latest version of fork and this will break the theme that they use

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in dabf936

$this->get('fork.cookie')
);
$siteHTMLHeader .= "\n" . $this->jsData;
$this->template->assignGlobal('siteHTMLHeader', trim($siteHTMLHeader));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now I would assign it to the both of them for BC reasons. There are people that upgrade to the latest version of fork and this will break the theme that they use

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in dabf936

);
$this->get('fork.settings')->set(
'Core',
'site_start_of_body_scripts',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write to the old one as well with a deprecation notice for BC reasons. There are people that upgrade to the latest version of fork and this might create issues if they use that setting somewhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in dabf936

);
$this->get('fork.settings')->set(
'Core',
'site_html_footer',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write to the old one as well with a deprecation notice for BC reasons. There are people that upgrade to the latest version of fork and this might create issues if they use that setting somewhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in dabf936

@tijsverkoyen tijsverkoyen requested review from a team, Katrienvh and carakas August 6, 2020 15:00
@carakas carakas merged commit 8653c14 into master Aug 12, 2020
@carakas carakas deleted the google-tagmanager branch August 12, 2020 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

0