@diosmosis opened this Pull Request on July 3rd 2017 Member

This PR contains the following changes which allow defining & handling custom period types.

Changes

  • Add the ArchiveQueryFactory class & ArchiveQuery interface so plugins can intercept Archive instance creation.
  • Add the CustomPeriodFactory interface to handle creating new period instances.
  • Make period responsible for determining start/end time of periods so aggregation is not limited to days w/o times.
  • Allow specifying a custom archive writer in PluginsArchiver.

Notes

  • This PR should be reviewed for any potential BC breaks. Don't think there are any, but I may have missed something.
  • This PR can be reviewed commit by commit.
@tsteur commented on July 4th 2017 Owner

Left 2 comments, otherwise looks good 👍

@diosmosis commented on July 8th 2017 Member

@tsteur made some changes including using Plugin\Manager::findComponents instead of DI for the period factory. I think it deserves another review.

@diosmosis commented on July 9th 2017 Member

Looking at the unit test failures, I'm not sure if it's a good idea to make Period\Factory::build dependent on plugins for every period type. For example, the AddSegmentByLabelInUTC filter uses it, and will thus become dependent on plugins being loaded to work (which breaks unit tests that use it).

I think ideally, the build function shouldn't be called as much as it is. Since it only creates a period instance from query parameters, I think it should ideally only be called at the beginning of a request. This would be a very large change to make, though.

I see three options to solve this (please suggest others if I'm missing something):

  1. Keep the periods in CoreHome & change the unit tests to use the loaded plugins (via the deprecated UnitTestCase or making them integration tests).
  2. Go back to creating core periods in Period\Factory::build w/ a test to ensure core knows if the custom period factory code breaks. This way plugins are only required to be loaded if a custom period is requested.
  3. Modify Piwik so only the code that executes controller/API methods needs to call it (everything else uses a period).
@tsteur commented on July 9th 2017 Owner

Good points. I'd probably go back to 2) like you had it before (core periods in core and not in plugin).

Didn't think of tests. If we decided to keep it in CoreHome, would need to make them integration tests.

@diosmosis commented on July 9th 2017 Member

@tsteur ready for another review.

@tsteur commented on August 13th 2017 Owner

Sorry for late review @diosmosis
looks all good to me

@diosmosis commented on August 13th 2017 Member

No worries @tsteur, will probably be a bit before it gets merged anyway :)

This Pull Request was closed on September 11th 2017
Powered by GitHub Issue Mirror