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

Carbon support for time-zone-aware doctrine types #2835

Open
rela589n opened this issue Aug 22, 2023 · 6 comments · May be fixed by CarbonPHP/carbon-doctrine-types#10
Open

Carbon support for time-zone-aware doctrine types #2835

rela589n opened this issue Aug 22, 2023 · 6 comments · May be fixed by CarbonPHP/carbon-doctrine-types#10
Assignees
Labels
enhancement good first issue If you wish to contribute to Carbon you could start with this issue

Comments

@rela589n
Copy link

Hello,

Doctrine has two types, which handle time-zones: DateTimeTzImmutableType and DateTimeTzType.
For carbon specifically, community has already written couple of types, which implement this:

https://github.com/beberlei/DoctrineExtensions/blob/master/src/Types/CarbonImmutableDateTimeTzType.php
https://github.com/beberlei/DoctrineExtensions/blob/master/src/Types/CarbonDateTimeTzType.php

I would like to propose introduction of two doctrine types to handle time zones:

  • carbontz
  • carbontz_immutable
@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Aug 23, 2023

As a matter of completeness and consistency, we could add that just like we have for CarbonImmutableType and CarbonType

Pull-request would be welcome.

  • If someone submit one while taking inspiration from a another implementation out there from the community please add some link to the original author repository.
  • Please add some unit-test to cover it.
  • CarbonTypeConverter trait should provide the needed transformation stuff already and ensure to make the new types consistent with existing one, then only have very few line of definition to write on the actual types.

Now as a recommendation:

Don't use any kind of entity type mixing both datetime and timezone.

You should try to have table columns (and entity property) to represent each piece of the data as atomically as possible. And while having timezone attached to a DateTime/Carbon object is fine and convenient as a high level input received by human, or output shown to them, it's not good to store them like that.

They are actually 2 different (and actually unrelated) things: a moment and a place. When receiving from user the result of a datepicker as 2023-08-23 05:35 America/New_York user actually gives you a moment: 2023-08-23 09:35 UTC and where they is (or some explicit setting to consider as the timezone choice of the user). It should go in another column of your entity or even in another entity (if talking about a meeting, might better be attached to a UserSettings entity than on the MeetingEvent entity, because if user setting latter change in such example it will affect the user display for all event, but you don't change when happen the meeting.) If we talk about something such as a flight, you should not actually store any timezone, just UTC time for take off and landing, then your entity would have in this example location of the take off and landing so you already has all you need to compute timezone of those places and show local times.

Then for 99% of the other cases, for instance showing when a message was sent in a messaging application, no timezone at all should be stored, you really must have UTC datetimes here so you can sort/compare consistently those messages and you just need to use the device timezone to show those dates and times.

I explained this recommendation more in details in this article Always Use UTC Dates And Times

@rela589n
Copy link
Author

Thanks for your response.

I agree that dates should always be stored in a way that allows us to sort/compare consistently. It can either be UTC date or date time with time zone. As a general practice, if we do not save time zone in any way, date must be saved in UTC.

Though, it is not always convenient to operate with UTC dates on the code level.
Say, on backend we accept date in two fields: datetime, timezone. Then it is converted to universal date time and all operations (incuding storage to database) happen on UTC date. Though, a lot of business logic depend on date time in user timezone, and it is necessary to convert from UTC back to user time zone just for this logic to work correctly. For instance, condition that datetime is after midnight, that date is current month, condition that next form submission is allowed after 7 days (when day 7 comes, user is allowed to submit the form again starting from 12:01am), etc.

Therefore, not at all is such swaying time zones back and forth convenient. For aforementioned cases it would be much more easier for us to operate on dates just in the user time zone. And when it comes to saving into DB, we'd save as carbontz_immutable.

@kylekatarnls
Copy link
Collaborator

it is necessary to convert from UTC back to user time zone just for this logic to work correctly
Yes, this conversion is super-cheap. It's not an issue to do it on the fly (at SQL level with CONVERT_TZ or once retrieve with Carbon with $date->tz()) then to do whatever calculation in whatever timezone.

The great advantage of doing this is that the same date can be "worked" on different timezones so to be served to different users having different timezones, or to APIs that would likely be UTC too.

So being able to know if a time is <, >, or = 12:01am if user timezone is not an valid reason to store date with user timezone, on the contrary, you keep flexibility and allow user timezone to later change without having to update all the dates everywhere if you just store those dates as UTC.

I would enjoin you to triple-challenge such seemingly convenient decisions, it's the one kind that might have clear visible benefit and many unnoticeable burden cost to long live with.

Saving few calls to CONVERT_TZ and ->tz() simply not worth the constraints it set that later become super annoying when you need to extend the design to work with a potentially more variable timezone context.

That's one of the reason why I will not implement this myself, that too much sounds like a tool that would hurt many users and likely never solve real problems the right way.

But this is open to pull-requests.

@kylekatarnls kylekatarnls added enhancement good first issue If you wish to contribute to Carbon you could start with this issue labels Aug 25, 2023
@varshneydevansh
Copy link

I have completed the setup and trying to do this :)

@varshneydevansh
Copy link

Do we also need common conversion logic to share between the CarbonTzType and CarbonTzImmutableType classes ?

@kylekatarnls
Copy link
Collaborator

Not necessarily, new types should land here: https://github.com/CarbonPHP/carbon-doctrine-types/tree/main/src/Carbon/Doctrine (doctrine types have been externalized in a separate package since this suggestion was open).

It contains a trait https://github.com/CarbonPHP/carbon-doctrine-types/blob/main/src/Carbon/Doctrine/CarbonTypeConverter.php that help not having duplicate code across type classes.

For instance things such as the maximum precision would be the same for a type with timezone. So we should have write new stuff for that and just re-use the existing trait for everything that works the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue If you wish to contribute to Carbon you could start with this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants