Not possible to create multiple Piwik tracker instances having different API urls #42
Conversation
…ent API urls fix #14
| if (empty($this->trackerUrl) && !empty(self::$URL)) { | ||
| // for BC | ||
| $this->trackerUrl = self::$URL; | ||
| } |
There was a problem hiding this comment.
Think $URL should override $this->trackerUrl? I'm thinking of the following situation:
$tracker = new PiwikTracker('http://mymatomo1.com/piwik.php'); // trackerUrl property set to mymatomo1.com
...
PiwikTracker::$URL = 'http://myothermatomo2.com/piwik.php';
$tracker->doTrackPageView(...); // trackerUrl overrides $URL so mymatomo1 is used instead of myothermatomo2.com
Actually I'm not too sure how to fix this... Maybe there needs to be two properties, $originalTrackerUrl set in the constructor and $trackerUrl set by the method. Where $trackerUrl overrides everything, but if not set, self::$URL is used? This isn't as simple as I thought it was...
There was a problem hiding this comment.
Yeah I'm not sure either. That's why I mentioned BC and being difficult. The problem is the next tracker creation new Tracker() would also call ::$URL = '...' and overwrite that... was thinking to remove self::$URL = ... in constructor but then we regress there. Not sure what is used more than the other... maybe we need to wait for Matomo 4 with this.
There was a problem hiding this comment.
I suppose we could add an options parameter like this:
$tracker = new PiwikTracker($idSite, 'myurl', [ 'doNotUseGlobalUrl' => true ]);
And maybe issue a warning or notice when doNotUseGlobalUrl isn't set to true?
Then in Matomo 4 we get rid of the option. At least this way, the bug doesn't cause problems in the meantime.
There was a problem hiding this comment.
Wouldn't we need to set it to false by default re BC? Users wouldn't be aware they need to change this parameter when they update (even when we mention it in changelog it would go mainly unnoticed). But at least with the option there be a way to restore old behaviour. Let's see what @mattab thinks
There was a problem hiding this comment.
Yes, would need to be false by default. I think this solution would mostly be for us not running into it, though if we could issue a warning of some kind w/o causing any issues, users might notice.
There was a problem hiding this comment.
Sorry but don't know what is the best solution here. But sounds good to throw a warning to help people notice their code might be mis-behaving.
There was a problem hiding this comment.
@tsteur any new thoughts on this conversation?
There was a problem hiding this comment.
I wouldn't do anything before Matomo 4 I reckon
There was a problem hiding this comment.
Ok, I'll remove this from the current sprint.
|
I'll close this one. If someone continues working on #14 this could be reused and the same changes could be applied to |
fix #14
cc @mattab @diosmosis