@peterhashair opened this Pull Request on July 8th 2022 Contributor

Description:

Review

@sgiehl commented on July 8th 2022 Member

@peterhashair Could you please explain when exactly this is an issue? (The issue number L3-264 seems invalid, so can't look up anything)

@peterhashair commented on July 10th 2022 Contributor

@sgiehl This happens when you tracking a site has login function, before login and after login are record as 2 different visits under visits log, and have those config set up in the tracking code. Then I revert this PR e0417845f8fd7031e00e97c3576415c998abb2ce it start to work.

 _paq.push(['trackPageView']);
          _paq.push(['enableLinkTracking']);
          _paq.push(['setSecureCookie', true]);
          _paq.push(['setUserId',null]);
          _paq.push(["setCookieDomain", "*.test"]);
          _paq.push(["setDomains", "*.test"]);
          _paq.push(["requireCookieConsent"]);
@sgiehl commented on July 11th 2022 Member

@peterhashair What exactly happens in that case? You now described a certain tracking use case and which problem your changes might aim to fix, but not which problem in the PHP code it tries to solve.

Based on the changes in the PR, there must have been some sort of warning / error / notice in PHP (like an undefined index).
In order to say if your fix actually solves any issue it would be good to know the exact PHP error and the stack trace, so it's possible to check if maybe reading the config in the first place might be done incorrectly.

@github-actions[bot] commented on July 26th 2022 Contributor

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

@sgiehl commented on July 26th 2022 Member

@peterhashair could you answer my questions from the previous comment?

@peterhashair commented on July 26th 2022 Contributor

@sgiehl sorry, I believe this PR is not valid anymore, but the problem was I can remember the code below keeps returning true.

        if (TrackerConfig::getConfigValue('enable_userid_overwrites_visitorid', $this->getIdSiteIfExists())) {
@sgiehl commented on July 27th 2022 Member

@sgiehl sorry, I believe this PR is not valid anymore, but the problem was I can remember the code below keeps returning true.

        if (TrackerConfig::getConfigValue('enable_userid_overwrites_visitorid', $this->getIdSiteIfExists())) {

The default value is true, so this is expected, unless someone changes the value in his config. 🤷

@sgiehl commented on August 4th 2022 Member

I'll merge this one now. Even though it does not have anything to do with the referenced issue, it could under certain circumstances avoid possible notices. e.g. when accessing a tracker config value that doesn't exist.

This Pull Request was closed on August 4th 2022
Powered by GitHub Issue Mirror