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

Changes to support custom periods #11837

Merged
merged 4 commits into from Sep 11, 2017

Conversation

Projects
None yet
3 participants
@diosmosis
Copy link
Member

commented Jul 3, 2017

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.

@diosmosis diosmosis requested review from tsteur, mattab and sgiehl Jul 3, 2017

*/
public function getDateStartUTC()
public function startOfDay()

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 4, 2017

Member

should we prefix with get?

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 4, 2017

Author Member

Sounds good, keeps it consistent

@@ -55,6 +56,17 @@ public static function build($period, $date, $timezone = 'UTC')
return new Year($date);
break;
}
$customPeriodFactories = StaticContainer::get('period.custom.factories');

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 4, 2017

Member

Maybe we could name it like just periods or so and then move all hard coded ones out of here into the config?

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 4, 2017

Author Member

I thought about that, but I think would be broken if a plugin accidentally removed the ability to create one of those periods, so I thought it would be better to hard code those, to make sure they are always available. (For example would it make sense to ever remove the day period?) What do you think?

This comment has been minimized.

Copy link
@tsteur

tsteur Jul 4, 2017

Member

I don't think it would make sense to ever remove a day period. Would move it to eg CoreHome plugin so it cannot be disabled. The advantage is that we would notice if something breaks in the code for "custom periods" as the "core" is using it itself and be good to have them as examples in a plugin.

This comment has been minimized.

Copy link
@diosmosis

diosmosis Jul 4, 2017

Author Member

It could still be removed if a plugin sets it to an empty array or uses DI\decorate to remove it. That said it would be hard for a plugin developer to not notice if they removed it, so perhaps it's not really an issue. I'll put them in CoreHome, seems like the better path.

@tsteur

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

Left 2 comments, otherwise looks good 👍

@diosmosis diosmosis force-pushed the diosmosis:archive-query-factory branch 3 times, most recently from c4f3859 to 57195a1 Jul 8, 2017

@diosmosis

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2017

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

@diosmosis

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2017

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

This comment has been minimized.

Copy link
Member

commented Jul 9, 2017

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 diosmosis force-pushed the diosmosis:archive-query-factory branch from 57195a1 to cb4329d Jul 9, 2017

@diosmosis

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2017

@tsteur ready for another review.

@mattab mattab added this to the 3.1.0 milestone Aug 3, 2017

@tsteur

This comment has been minimized.

Copy link
Member

commented Aug 13, 2017

Sorry for late review @diosmosis
looks all good to me

@diosmosis

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2017

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

@mattab mattab merged commit f0ddb70 into matomo-org:3.x-dev Sep 11, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@diosmosis diosmosis deleted the diosmosis:archive-query-factory branch Sep 11, 2017

@mattab mattab modified the milestones: 3.2.0, 3.1.1 Sep 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.