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

Why don't factories convert dates to the correct timezone when formatting #2416

Closed
grantholle opened this issue Aug 12, 2021 · 5 comments · Fixed by #2418
Closed

Why don't factories convert dates to the correct timezone when formatting #2416

grantholle opened this issue Aug 12, 2021 · 5 comments · Fixed by #2418
Assignees
Labels
Milestone

Comments

@grantholle
Copy link

Hello,

I tried searching to see if this question has come up before, and for some reason it hasn't.

I'm having trouble understanding why factories don't automatically convert the date and time to the factory's timezone when parsing/making from the factory.

$factory = new \Carbon\Factory([
    'locale' => 'zh_CN',
    'timezone' => 'Asia/Shanghai',
]);

$baseDate = \Carbon\Carbon::parse('2021-08-01 08:00:00', 'UTC');

echo $baseDate->format('c') . "\n";
// 2021-08-01T08:00:00+00:00

$date = $factory->make($baseDate);
echo $date->format('c');
// 2021-08-01T08:00:00+08:00

Carbon version: 2.51.1

PHP version: 8.0.8

I expected to get:

$date = $factory->make($baseDate);
echo $date->format('c');
// 2021-08-01T16:00:00+08:00

But I actually get:

$date = $factory->make($baseDate);
echo $date->format('c');
// 2021-08-01T08:00:00+08:00

I don't understand the reasoning of giving the factory a timezone if it just displays the same date and time, but a different timezone. When I give a factory a date in UTC and give it to a factory, I expect it to convert that date to its configured timezone.

Take the documentation example for instance.

An event happened in time, Carbon::parse('2018-06-15 12:34:00', 'UTC'), and we want to display that event for different locales, which factories definitely solve. But it tells them (in the their own locale 👍 ) that the event happened at the exact same time (2018-06-15 12:34) in their own timezone, which isn't true at all. Martin sees the move happening at 12:34 in Paris, and John also sees it happening at 12:34 in Chicago, which doesn't make sense to me. Wouldn't it make sense to show the date in their locale and relative to their timezone?

Thanks!

@grantholle grantholle changed the title Why don't factories convert dates Why don't factories convert dates to the correct timezone when formatting Aug 12, 2021
@kylekatarnls
Copy link
Collaborator

Hello 👋

Meanwhile I'm checking if this behavior is a regression or backward-compatibility for something, note that you can get the result you expect with:

$factory = new \Carbon\Factory([
    'locale' => 'zh_CN',
    'timezone' => 'Asia/Shanghai',
]);

$baseDate = \Carbon\Carbon::parse('2021-08-01 08:00:00', 'UTC');

$date = $factory->createFromTimestamp($baseDate->getTimestamp());
echo $date->format('c');
// 2021-08-01T16:00:00+08:00

@kylekatarnls
Copy link
Collaborator

Hum I see where it broke. It's quite old:
99976db#diff-4cb6bb84421fc609ad881f42ea5f250ad8f9e03c1c5b654f50929d8739c0d763R301

And we can't easily fix it because you can also use make() like this:

$date = $factory->make('2021-08-01 16:00:00');
echo $date->format('c');

So it has to behave differently depending on the input data had a timezone information or not.

@grantholle
Copy link
Author

Thanks for workaround @kylekatarnls.

Since I couldn't find anyone who has brought this up, I wanted to validate that my expected behavior is what others expect as well. What a wonderful world timezones are! 🤪

@kylekatarnls
Copy link
Collaborator

It will be fixed for the next version, you can see various factory usages in tests and the output it will produce once 2.52.2 will be released:

https://github.com/briannesbitt/Carbon/pull/2418/files#diff-2adca31832a76f38aef5997d5d141c612c5f5261967210e8b57bfb37cf4fcb3d

@grantholle
Copy link
Author

@kylekatarnls you are the bomb 💣 ! Thank you so much

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 a pull request may close this issue.

2 participants