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

feat: add datetime constructor with timezone handling #509

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

romm
Copy link
Member

@romm romm commented Mar 27, 2024

This is a draft PR, I'm not sure I want to merge it. Any feedback is really appreciated.

TODO:

  • Add doc

It is now possible to use the following signature to map to a datetime: array{datetime: non-empty-string|int, timezone: \DateTimeZone}.

(new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(\DateTimeInterface::class, [
        'datetime' => '2024-03-28T21:12:27+00:00',
        'timezone' => 'America/New_York',
    ]);

It is now possible to use the following signature to map to a datetime:
`array{datetime: non-empty-string|int, timezone: \DateTimeZone}`.

```php
(new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(\DateTimeInterface::class, [
        'datetime' => '2024-03-28T21:12:27+00:00',
        'timezone' => 'America/New_York',
    ]);
```
@romm romm force-pushed the feat/handle-datetimezone-in-datetime-constructor branch from 028bac8 to c087e75 Compare April 12, 2024 15:38
@boesing
Copy link
Contributor

boesing commented Apr 22, 2024

My 2 cents, I'd rather have the default timezone configured via the mapper builder.
Could fallback to date_default_timezone_get() which always provides the system defaults timezone (which is also the default which is used when instantiating date objects without a specific timezone).

The whole timezone thing is not a big deal for our project since we either have Y-m-d with prefixed ! (to always end up with 00:00:00 in those objects) or Y-m-d\TH:i:sP which contains timezone information. Due to the usage of DateTImeImmutable::createFromFormat within valinor, luckily we can dodge the whole timezone problematic. But when working with external APIs which do not follow this kind of formatting, a default timezone might not be what we need as well.
Maybe a DateTimeZone attribute to the date property could help to extend the default timezone for JSON specific timezones?

I am thinking about 3rd party APIs need to be mapped which state "we deliver times always in timezone America/Los_Angeles" while the client (i.e. we who use valinor to map those responses) is in Europe/Berlin. That would not help when having default timezone configured (I personally would not need that feature) but an attribute could actually provide per-property configuration (maybe even property + class level? not sure if that works well with valinor tho).


The example above is also kinda missleading, since timezone argument actually has no effect if timezone is provided within the datetime format:
https://3v4l.org/Kkkap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants