Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add additional Google Analytics cookies to the cookie deny list #1615

Merged
merged 3 commits into from
Apr 2, 2020
Merged

Add additional Google Analytics cookies to the cookie deny list #1615

merged 3 commits into from
Apr 2, 2020

Conversation

Mynyx
Copy link
Contributor

@Mynyx Mynyx commented Apr 2, 2020

leofeyer
leofeyer requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

We do not need two separate sections for Google Analytics. Just keep the original comment as it was and add the missing names. (We might even be able to use a regex such as '__utm[abctvz]' instead of multiple lines).

@leofeyer leofeyer added the bug label Apr 2, 2020
@leofeyer leofeyer added this to the 4.9 milestone Apr 2, 2020
@Toflar
Copy link
Member

Toflar commented Apr 2, 2020

I would maybe just strip __utm*. I can't think of any developer that would prefix their cookies with this string and use it internally :D

@Mynyx
Copy link
Contributor Author

Mynyx commented Apr 2, 2020

We do not need two separate sections for Google Analytics. Just keep the original comment as it was and add the missing names. (We might even be able to use a regex such as '__utm[abctvz]' instead of multiple lines).

Okay, I have additionally added _dc_gtm_<property-id>.

_gat: Used to throttle request rate. If Google Analytics is deployed via Google Tag Manager, this cookie will be named _dc_gtm_<property-id>.

@Mynyx
Copy link
Contributor Author

Mynyx commented Apr 2, 2020

I would maybe just strip __utm*. I can't think of any developer that would prefix their cookies with this string and use it internally :D

That was my first implementation, but I thought I should by more careful. 😄

@Mynyx Mynyx changed the title Add Google Analytics ga.js Cookies to StripCookiesSubscriber Add additional Google Analytics Cookies to StripCookiesSubscriber Apr 2, 2020
@leofeyer leofeyer merged commit 2e78bd1 into contao:4.9 Apr 2, 2020
@leofeyer
Copy link
Member

leofeyer commented Apr 2, 2020

Thank you @Mynyx.

@Mynyx Mynyx deleted the patch-4 branch April 2, 2020 11:02
@leofeyer leofeyer changed the title Add additional Google Analytics Cookies to StripCookiesSubscriber Add additional Google Analytics cookies to the cookie blacklist Apr 2, 2020
@leofeyer leofeyer changed the title Add additional Google Analytics cookies to the cookie blacklist Add additional Google Analytics cookies to the cookie deny list Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofeyer leofeyer leofeyer approved these changes

Assignees

@Mynyx Mynyx

Labels
bug
Projects
None yet
Milestone
4.9
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Mynyx @Toflar @leofeyer