@tsteur opened this Pull Request on February 18th 2020 Member

refs https://github.com/matomo-org/wp-matomo/issues/215

see https://github.com/matomo-org/wp-matomo/issues/215#issuecomment-587160358

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
image

in
image

To be safe could apply both patches. The one already done in GoalManager and moving up one line setting the last visit time.

@sgiehl commented on February 18th 2020 Member

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:

https://github.com/matomo-org/matomo/blob/c16953bd8a8c03efd0a516f571516ea3638e5799/core/Tracker/Visit.php#L236-L241

@tsteur commented on February 18th 2020 Member

@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 0 to be returned.

I wonder if this is maybe not an issue in Matomo core because MYSQLI_CLIENT_FOUND_ROWS is set at least for the Mysqli tracker. PDO also seems to set PDO::MYSQL_ATTR_FOUND_ROWS. So this seems like a WordPress only issue since I can't find that flag there and I can't really add that flag.

Will see if I can add that flag to WordPress since there is a MYSQL_CLIENT_FLAGS constant config but this might not always work since a user might define custom flags, and I would need to figure out if WordPress is using mysqli or mysql to set correct constant etc.

I kind of reckon too though we could simply not return an error when updateExistingVisit is not changing any value in model::updateVisit since we can kind of assume it should find that visit. Otherwise it would think it is a new visit anyway. Not sure why it's there.

@tsteur commented on February 18th 2020 Member

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 wasInserted and always return true or if $wasInserted = false (which should be rarely the case) we could do a select from where idsite=? and idvisit =? and if the row exists only then return true. Then we could safely assume there was no change and technically the update was successful. This query should be pretty much never executed in On-Premise so it wouldn't be an issue to have it there. Any thoughts?

@sgiehl commented on February 19th 2020 Member

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.

@tsteur commented on February 19th 2020 Member

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.

@tsteur commented on February 20th 2020 Member

@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 visitProperties or compare some values before and after but it's not really reliable to assume what the value in the DB is. Therefore added the hasVisit check and also leaving the goal manager check in there just to be safe.

This Pull Request was closed on February 20th 2020
Powered by GitHub Issue Mirror