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
Refactor ArchiveInvalidator: dependency decoupling, responsibility limiting and code reorganization #8851
Conversation
…rvice by moving logic to new request processor. ArchiveInvalidator used to update site creation time. The reasoning was, if we tracked data in the past that was before the site's creation time, the site creation time should be updated, so it will be displayed in the UI. The tracking should trigger archive invalidation which should eventually trigger an update to the site creation time. This is incorrectly mixing concerns. Now the new request processor will make this change at the moment old data is tracked. New test included.
99742f5
to
ed2991d
Compare
…php so the service class only accepts well formed input.
…with logic to detect maximum date to purge from.
ed2991d
to
41b5b38
Compare
private function getSiteCreatedTime($idSite) | ||
{ | ||
$attributes = Cache::getCacheWebsiteAttributes($idSite); | ||
$tsCreated = isset($attributes['ts_created']) ? $attributes['ts_created'] : 0; |
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.
Returning 0
could result in the current day as Date::factory(0) == now or today
. So we might update the sites day accidentally just because it was not in the cache. Do you know what I mean? If request time is earlier than now or today, the site will be updated. In reality the sites creation time could be like a couple of years back but because it's not in cache it would be suddenly set to the request timestamp?
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.
Also not using ? :
would sometimes improve readability (especially when looking at a diff side by side where there's not much space) but I guess that's just personal opinion so feel free to ignore :) I know many prefer the other way around
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.
Ok, will handle this case specifically. And I don't have an issue w/ not using ? :
here.
Looks good apart from the possible |
…d in tracker cache more explicitly. Make sure the site created time is not modified in this case.
…ever be set. If a tracker request is for an existing visit in the past, then the site creation time will always be earlier, since new action will be later than the original visit (so the original visit would have changed the creation time).
Refactor ArchiveInvalidator: dependency decoupling, responsibility limiting and code reorganization
This PR includes the following changes: