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

Avoid creating any archive tables for future dates #13504

Merged
merged 2 commits into from Oct 2, 2018

Conversation

Projects
None yet
3 participants
@sgiehl
Member

sgiehl commented Sep 29, 2018

Under certain circumstances Archiving might create empty archive_numeric tables for future dates:

  • enable_browser_archiving_triggering is set to 0
  • requesting a date range in the future. e.g. something like &period=range&date=2018-01-01,2019-09-29

@sgiehl sgiehl added the Needs Review label Sep 29, 2018

@sgiehl sgiehl added this to the 3.6.1 milestone Sep 29, 2018

@@ -166,6 +166,9 @@ public static function getArchiveIds($siteIds, $periods, $segment, $plugins)
$monthToPeriods = array();
foreach ($periods as $period) {
/** @var Period $period */
if ($period->getDateStart()->isLater(Date::now()->addDay(2))) {
continue; // avoid creating any archive tables in the future
}

This comment has been minimized.

@diosmosis

diosmosis Sep 30, 2018

Member

Should we add this code to ArchiveTableCreator? I can't think of a reason for code to create a table in the future (except possibly tests, though probably not). If some other code uses ArchiveTableCreator directly, this safeguard would be applied there too.

This comment has been minimized.

@sgiehl

sgiehl Oct 1, 2018

Member

Not sure if it might make sense for some custom plugins to create archives in the future. e.g. for some kind of prediction reports of whatever. Nevertheless the ArchiveSelector shouldn't trigger the creation

This comment has been minimized.

@diosmosis

diosmosis Oct 1, 2018

Member

Makes sense.

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 1, 2018

Is it possible to add a test for this? Otherwise looks good to me.

@sgiehl

This comment has been minimized.

Member

sgiehl commented Oct 1, 2018

@diosmosis what shall the test check? That no archive table is created when ArchiveSelector::getNumeicTable is called with a future date?

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 1, 2018

That could work. Or a system/integration test that uses a date in the future w/ VisitsSummary.get and has a check that the table wasn't created.

@mattab

This comment has been minimized.

Member

mattab commented Oct 2, 2018

Test sounds good 👍 then good to merge

@sgiehl sgiehl force-pushed the nofuturearchive branch from 495c326 to fcc2234 Oct 2, 2018

@sgiehl

This comment has been minimized.

Member

sgiehl commented Oct 2, 2018

I've added two simple tests. (Note: The first of the tests would currently fail on 3.x-dev branch, as an archive table is created)

@diosmosis diosmosis merged commit 8940384 into 3.x-dev Oct 2, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@diosmosis diosmosis deleted the nofuturearchive branch Oct 2, 2018

InfinityVoid added a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018

Avoid creating any archive tables for future dates (matomo-org#13504)
* Avoid creating any archive tables for future dates

* Adds simple tests to prove no future archive tables are created
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment