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

CarbonPeriod and CarbonImmutable not behaving similarly #1980

Merged
merged 2 commits into from
Jan 10, 2020

Conversation

deleugpn
Copy link
Contributor

@deleugpn deleugpn commented Jan 8, 2020

This Pull Request is in the form of Help Wanted / Possible Bug report.

I recently noticed that when using CarbonPeriod and Carbon, they differ in configuration of when the week start (Monday vs Sunday) causing inconsistency. I'm looking for help on validating whether there's something I can do to fix this issue or if it's actually a bug that would need to be addressed in Carbon itself.

The use case:

Parsing the date 2019-12-01 and asking for the first day of the Week, by default, returns 2019-11-25, which is the Monday of said Week. Using CarbonPeriod to make an array of weeks will give an array of Carbon instances that are differently configured and will consider Sunday as the first day of the week. When asking for the first day of the week from 2019-11-25, it further reduces 1 day to 24th of November (Sunday).

  • Is there a way to instruct CarbonPeriod to be consistent with default Carbon settings?
  • Is this something I can do while looping through each row of Carbon Period?
  • Is this something worth investigating inside Carbon itself?

Thanks!

@kylekatarnls
Copy link
Collaborator

First of all here is how to specify explicitly the start of week and get 25th november for both:

$sunday = CarbonImmutable::parse('2019-12-01');
$period = CarbonPeriod::create($sunday->startOfWeek(CarbonImmutable::MONDAY), '1 week', $sunday->endOfWeek(CarbonImmutable::MONDAY))->toArray();
$formattedSunday = $sunday->startOfWeek(CarbonImmutable::MONDAY)->format('Y-m-d H:i:s');

echo $formattedSunday."\n";
echo $period[0]->toImmutable()->startOfWeek(CarbonImmutable::MONDAY)->format('Y-m-d H:i:s');

And the root cause of this behavior is the locale. That's why setting the period on a locale having Monday as start of week will also work as you expect:

$sunday = CarbonImmutable::parse('2019-12-01');
$period = CarbonPeriod::create($sunday->startOfWeek(), '1 week', $sunday->endOfWeek())->locale('en_GB')->toArray();
$formattedSunday = $sunday->startOfWeek()->format('Y-m-d H:i:s');

echo $formattedSunday."\n";
echo $period[0]->toImmutable()->startOfWeek()->format('Y-m-d H:i:s');

In a next major version, the default start of week might become Sunday (to be aligned on the default locale en = en_US) (see #1967)

Right now, the date use Monday as start of week if it has no explicit locale.

The bug in the particular case of the CarbonPeriod is the period propagate its settings to the dates it creates including its locale:

$sunday = CarbonImmutable::parse('2019-12-01');
$period = CarbonPeriod::create($sunday->startOfWeek(), '1 week', $sunday->endOfWeek(), CarbonPeriod::IMMUTABLE)->toArray();
$formattedSunday = $sunday->startOfWeek()->format('Y-m-d H:i:s');

var_dump($sunday); // no localTranslator
var_dump($period[0]); // has localTranslator

So the fix would be to handle the case where localTranslator is null so it can be propagated to iterated dates.

Side note: use CarbonPeriod::IMMUTABLE option to not have to call ->toImmutable() on each date.

@kylekatarnls kylekatarnls self-assigned this Jan 9, 2020
@deleugpn
Copy link
Contributor Author

I forgot to thank you for your reply. I decided to follow your suggestion on explicitly setting when the Week starts $sunday->startOfWeek(CarbonImmutable::MONDAY) as it will even make my code resilient to a major change when Carbon decides to start on Sunday by default.
It completely resolved my issue and the code is nice.
I also adopted the suggestion for CarbonPeriod with the ::Immutable parameter. Would be nice to have a named constructor for that, though 😄

@kylekatarnls
Copy link
Collaborator

I get your test to pass without breaking other tests (without having to change the default start-of-week day).

We'll have the correct locale setting propagation through CarbonPeriod in the next version 2.29

@kylekatarnls kylekatarnls merged commit d7c026d into briannesbitt:master Jan 10, 2020
@deleugpn deleugpn deleted the carbon-period branch February 8, 2020 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants