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
Ensure a valid timestamp is set when recording a conversion #15587
Conversation
refs matomo-org/matomo-for-wordpress#215 see matomo-org/matomo-for-wordpress#215 (comment) There should be no harm in this logic. Basically the last visit time is the current timestamp anyway. It be actually probably even more accurate to use the current timestamp there in general. Eg this could happen if an exception was triggered in https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L192-L194 where the visitor was not found for some reason when updating, or when no value changed I suppose because it was updated within a few ms. Another possibility that may be better be to set the `visit_last_action_time ` https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L249 at the beginning of that function or so. Not sure if there is maybe a reason it's set at the end. Or maybe it could be at least moved up one line before `updateExistingVisit` https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L247 so if it triggers an exception then we still have surely a value set. Not sure if it would fix that particular error though. Can't really think of a different way though how this would happen so far. I was able to trigger the VisitorNotFoundInDb exception when no value changed
Wondering if we shouldn't change the behavior, so no error is triggered when there actually were no values to update. Maybe we should check if the value actually is different and unset it if not here: Lines 236 to 241 in c16953b
|
@sgiehl technically there were values to update but if a request is sent twice within a short time the values might not actually change causing I wonder if this is maybe not an issue in Matomo core because Will see if I can add that flag to WordPress since there is a I kind of reckon too though we could simply not return an error when |
I've had a look and there is no reliable way for me to set the flag without possibly breaking a WordPress installation. I reckon we could either ignore |
Haven't looked deeper in the code, but I would have tried something like this $valuesToUpdate = $this->getExistingVisitFieldsToUpdate($visitIsConverted);
// update visitorInfo
foreach ($valuesToUpdate as $name => $value) {
if ($this->visitProperties->getProperty($name) === $value) {
unset($valuesToUpdate[$name]);
continue;
}
$this->visitProperties->setProperty($name, $value);
} That might prevent doing an update at all if no values changed. But not sure if that would work. Your suggestion with trying to fetch the visit if update returns false, sounds fine as well. |
Cheers @sgiehl I'll have a think in a bit. I reckon it might not work 100% re checking the values like this technically since maybe the value in the DB has changed again between fetching visitor information and updating it. Although this could be ignored probably. I will check that out. |
@sgiehl couldn't really go with the suggested approach. I could add some of the logic between those lines https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L236-L239 but then in https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L244 there would be more values added so it would have values again in the end. And I couldn't really do the check here https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L247 because the values are always the same because visitProperties are set here: https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L240 I could try creating a copy of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now
refs matomo-org/matomo-for-wordpress#215
see matomo-org/matomo-for-wordpress#215 (comment)
There should be no harm in this logic. Basically the last visit time is the current timestamp anyway. It be actually probably even more accurate to use the current timestamp there in general.
Eg this could happen if an exception was triggered in https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L192-L194 where the visitor was not found for some reason when updating, or when no value changed I suppose because it was updated within a few ms.
Another possibility that may be better be to set the
visit_last_action_time
https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L249 at the beginning of that function or so. Not sure if there is maybe a reason it's set at the end. Or maybe it could be at least moved up one line beforeupdateExistingVisit
https://github.com/matomo-org/matomo/blob/3.13.2/core/Tracker/Visit.php#L247 so if it triggers an exception then we still have surely a value set. Not sure if it would fix that particular error though. Can't really think of a different way though how this would happen so far.I was able to trigger the VisitorNotFoundInDb exception when no value changed
in
To be safe could apply both patches. The one already done in GoalManager and moving up one line setting the last visit time.