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

Prevent possible notice when using tracker config #19495

Merged
merged 1 commit into from Aug 4, 2022
Merged

Prevent possible notice when using tracker config #19495

merged 1 commit into from Aug 4, 2022

Conversation

peterhashair
Copy link
Contributor

@peterhashair peterhashair commented Jul 8, 2022

Description:

Review

update tracking config
@sgiehl
Copy link
Member

sgiehl commented Jul 8, 2022

@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
Copy link
Contributor Author

peterhashair commented Jul 10, 2022

@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
Copy link
Member

sgiehl commented Jul 11, 2022

@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
Copy link
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'.

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jul 26, 2022
@sgiehl
Copy link
Member

sgiehl commented Jul 26, 2022

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

@peterhashair
Copy link
Contributor Author

peterhashair commented Jul 26, 2022

@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())) {

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Jul 27, 2022
@sgiehl
Copy link
Member

sgiehl commented Jul 27, 2022

@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 sgiehl changed the title L3-264 tracking Config not set return null Prevent possible notice when using tracker config Aug 4, 2022
@sgiehl
Copy link
Member

sgiehl commented Aug 4, 2022

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.

@sgiehl sgiehl merged commit e7d7ece into 4.x-dev Aug 4, 2022
@sgiehl sgiehl deleted the L3-265 branch August 4, 2022 13:39
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Aug 4, 2022
@sgiehl sgiehl added this to the 4.12.0 milestone Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants