Changes to support custom periods by diosmosis · Pull Request #11837 · matomo-org/matomo

diosmosis · 2017-07-03T21:57:57Z

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 · 2017-07-04T08:15:02Z

core/Date.php

-     * @return string
+     * @return Date
     */
-    public function getDateStartUTC()


should we prefix with get?

Sounds good, keeps it consistent

tsteur · 2017-07-04T08:43:00Z

core/Period/Factory.php

                break;
        }
+
+        $customPeriodFactories = StaticContainer::get('period.custom.factories');


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

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?

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.

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 · 2017-07-04T08:43:24Z

Left 2 comments, otherwise looks good 👍

diosmosis · 2017-07-08T23:54:29Z

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

diosmosis · 2017-07-09T03:14:04Z

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):

Keep the periods in CoreHome & change the unit tests to use the loaded plugins (via the deprecated UnitTestCase or making them integration tests).
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.
Modify Piwik so only the code that executes controller/API methods needs to call it (everything else uses a period).

tsteur · 2017-07-09T08:20:32Z

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.

…t LogAggregator.

diosmosis · 2017-07-09T21:09:26Z

@tsteur ready for another review.

tsteur · 2017-08-13T23:28:49Z

Sorry for late review @diosmosis
looks all good to me

diosmosis · 2017-08-13T23:56:06Z

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

Separate Archive query creation responsibility from Archive class.

70860c7

diosmosis requested review from mattab, sgiehl and tsteur July 3, 2017 21:57

tsteur reviewed Jul 4, 2017

View reviewed changes

diosmosis added 3 commits July 9, 2017 13:28

Add ability for plugins to define custom period types.

be89be2

Make period responsible for determining start/end time of periods, no…

516102e

…t LogAggregator.

Allow specifying custom archive writer in PluginsArchiver.

cb4329d

sgiehl mentioned this pull request Jul 19, 2017

Adding support for a fiscal year #11805

Closed

mattab added this to the 3.1.0 milestone Aug 3, 2017

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

diosmosis deleted the archive-query-factory branch September 11, 2017 17:42

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

tsteur mentioned this pull request Mar 7, 2023

As a user I want to be able to select a date period "Quarter" #20418

Open

Uh oh!

Conversation

diosmosis commented Jul 3, 2017

Changes

Notes

Uh oh!

tsteur Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

diosmosis Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

tsteur Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

diosmosis Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

tsteur Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

diosmosis Jul 4, 2017

Choose a reason for hiding this comment

Uh oh!

tsteur commented Jul 4, 2017

Uh oh!

diosmosis commented Jul 8, 2017

Uh oh!

diosmosis commented Jul 9, 2017

Uh oh!

tsteur commented Jul 9, 2017

Uh oh!

diosmosis commented Jul 9, 2017

Uh oh!

tsteur commented Aug 13, 2017

Uh oh!

diosmosis commented Aug 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants