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.

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

fix: Cross-subdomain tracking is not working for long TLDs #787

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

MahdiGhiasi
Copy link
Contributor

@MahdiGhiasi MahdiGhiasi commented Sep 2, 2023

Changes

Fixes #786.

Changed DOMAIN_MATCH_REGEX variable from [a-z0-9][a-z0-9-]+\.[a-z.]{2,6}$ to [a-z0-9][a-z0-9-]+\.[a-z]{2,}$.

It accepts any domain with a TLD length of at least 2 characters, and extracts the domain name only.

(I'm not sure why there were a . in the final section of previous regex, as it also fails on cases such as me.you.co as well, where whole string matches with the previous pattern. The new pattern should work fine and extract the domain name.)

...

Checklist

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

I think this looks good. Sorry for the delay on the review

@benjackwhite benjackwhite added the bump patch Bump patch version when this PR gets merged label Sep 12, 2023
@MahdiGhiasi MahdiGhiasi changed the title Fix: Cross-subdomain tracking is not working for long TLDs fix: Cross-subdomain tracking is not working for long TLDs Sep 14, 2023
@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@MahdiGhiasi
Copy link
Contributor Author

@benjackwhite @mariusandra When will this be merged? We're using PostHog and need this to track our users.

@benjackwhite benjackwhite self-requested a review October 2, 2023 09:45
@jiito
Copy link

jiito commented Oct 10, 2023

  • 1 for this change. We have a .systems TLD and the multi-host tracking does not work because of this. Thanks for the fix @MahdiGhiasi

@mariusandra mariusandra reopened this Oct 11, 2023
@mariusandra mariusandra merged commit ab2ac71 into PostHog:master Oct 11, 2023
13 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cross-subdomain tracking is not working for long TLDs
5 participants