Skip to content
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

Merged
merged 8 commits into from Sep 25, 2015

Conversation

diosmosis
Copy link
Member

This PR includes the following changes:

  • ArchiveInvalidator's dependency on SitesManager has been removed. The code that updates a sites created time has been moved to tracker, since it's reason for existence is to make sure if visits are tracked before a site's creation date, those visits will still be displayed in the UI. This code was moved to a new RequestProcessor located within the SitesManager plugin (so core is no longer dependent on SitesManager for this at least).
  • The date parsing logic was moved from ArchiveInvalidator to callers. The ArchiveInvalidator service now takes well-formed input only. At least for the dates.
  • Some code related to PrivacyManager dependency was grouped closer together in ArchiveInvalidator.php both physically and in terms of code execution.

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Sep 25, 2015
@diosmosis diosmosis added this to the 2.15.0 milestone Sep 25, 2015
…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.
private function getSiteCreatedTime($idSite)
{
$attributes = Cache::getCacheWebsiteAttributes($idSite);
$tsCreated = isset($attributes['ts_created']) ? $attributes['ts_created'] : 0;
Copy link
Member

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?

Copy link
Member

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

Copy link
Member Author

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.

@tsteur
Copy link
Member

tsteur commented Sep 25, 2015

Looks good apart from the possible Date::factory(0). Others are only suggestions.

diosmosis added 2 commits September 25, 2015 01:24
…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).
diosmosis added a commit that referenced this pull request Sep 25, 2015
Refactor ArchiveInvalidator: dependency decoupling, responsibility limiting and code reorganization
@diosmosis diosmosis merged commit d4e86d9 into master Sep 25, 2015
@diosmosis diosmosis deleted the invalidator_refactor branch September 25, 2015 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants