Take archive TTL time into consideration for today when browser archiving is enabled #18241
Conversation
tsteur
changed the title
|
From a logically point of view that change makes sense to me. maybe @diosmosis can quickly confirm that |
|
@sgiehl yes, makes sense to me too 👍 |
|
I was actually trying to reproduce that issue but couldn't. The As you can see the first parameter is Meaning when the archive is older than the min time, it will actually never go into that code. None of the previous It was added in https://github.com/matomo-org/matomo/pull/17379/files for ranges. Also tried to reproduce it for ranges where it includes today, but failed to reproduce to get it into that method. Also tested eg with various I suppose we could just merge it and it'll be safe but not entirely sure why it is there. |
|
Guess having it as fallback might not hurt. We could maybe add a comment there, so someone looking at that code might directly notice it might be unneeded. |
|
Good idea @sgiehl added it. Will check in the evening if anything came up. |
|
No log so far, will check again tomorrow |
|
@sgiehl ran this for almost 24 hours and it was not triggered once. I'll check again tomorrow |
|
@sgiehl last 24 hours it was again not logged. I can check again next Monday. On Cloud this would go in there only for ranges. Possible they aren't being invalidated that often or so. |
|
AKA it might be more reproducible that the code is needed on a Matomo where browser archiving is enabled. Which it isn't on Cloud |
|
Still nothing in the logs @sgiehl but then we also don't have browser archiving enabled. we could either remove the as it the check was added I'm assuming there was a reason for it and we'd need to change to return |
|
I'll simply merge it. |
Description:
While we looked into the goal range issue this looked potentially like a bug to me.
There is a
return false;but at the end of the method also areturn false;so I was wondering why the check was there in the first place as it would return false anyway.Then looked at the check
$minDatetimeArchiveProcessedUTC && Date::factory($tsArchived)->isEarlier($minDatetimeArchiveProcessedUTC)and if I read this right, if the archive was created before the minimum date, then it doesn't force archive invalidation but probably it should.Could maybe someone have a look that this PR looks correct? Could think about adding a test but this might be tricky. I haven't even tried to reproduce the issue due to lack of time. I would think that
[General]time_before_today_archive_considered_outdatedsettingThen I would expect that maybe Matomo falsely isn't archiving again meaning effectively Matomo will only archive again if the report is being requested the next day again. But only max once per day maybe.
Review