Changes to support custom periods #11837
Conversation
core/Date.php
Outdated
| */ | ||
| public function getDateStartUTC() | ||
| public function startOfDay() |
There was a problem hiding this comment.
Sounds good, keeps it consistent
core/Period/Factory.php
Outdated
| @@ -55,6 +56,17 @@ public static function build($period, $date, $timezone = 'UTC') | |||
| return new Year($date); | |||
| break; | |||
| } | |||
|
|
|||
| $customPeriodFactories = StaticContainer::get('period.custom.factories'); | |||
There was a problem hiding this comment.
Maybe we could name it like just periods or so and then move all hard coded ones out of here into the config?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Left 2 comments, otherwise looks good 👍 |
|
@tsteur made some changes including using |
|
Looking at the unit test failures, I'm not sure if it's a good idea to make 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):
|
|
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. |
|
@tsteur ready for another review. |
|
Sorry for late review @diosmosis |
|
No worries @tsteur, will probably be a bit before it gets merged anyway :) |
This PR contains the following changes which allow defining & handling custom period types.
Changes
Notes