filamentphp / filament

A collection of beautiful full-stack components for Laravel. The perfect starting point for your next app. Using Livewire, Alpine.js and Tailwind CSS.
https://filamentphp.com
MIT License
18.07k stars 2.83k forks source link

DateTimePicker timezone jumps back 1 day #5601

Closed madsem closed 1 year ago

madsem commented 1 year ago

Package

filament/tables

Package Version

v2.16.67

Laravel Version

v9

Livewire Version

v2

PHP Version

php 8.1

Problem description

Referencing the discussion here: https://github.com/filamentphp/filament/discussions/5600

I created a repo for reproduction of this issue.

Expected behavior

That the first date picker shows Feb 1, the second Feb 28

Steps to reproduce

Clone it, run npm run build and php artisan serve and go to http://127.0.0.1:8000/pickme

At first the correct dates are presented, but when you click reset filters and refresh you will see that the first date time picker shows Jan 31 and the second Feb 28.

Screen Shot 2023-02-02 at 2 48 29 PM

Reproduction repository

https://github.com/madsem/filamentphp-datetimepicker

Relevant log output

No response

modrictin commented 1 year ago

I can confirm, I also experience this issue. After each request, the date subtracts one day.

danharrin commented 1 year ago

Hey, I've pulled down your repo and tested this. I've discovered that timezone() on the date picker is only appropriate if your Carbon instance doesn't already have one. So you should be able to just remove that line and keep the Carbon::now()->timezone($timezone). Let me know if this isn't the correct solution.

madsem commented 1 year ago

Hey, I've pulled down your repo and tested this. I've discovered that timezone() on the date picker is only appropriate if your Carbon instance doesn't already have one. So you should be able to just remove that line and keep the Carbon::now()->timezone($timezone). Let me know if this isn't the correct solution.

Hey @danharrin thanks for looking into this, but your solution doesn't seem to work.

Have you tried removing that line, and use a timezone other than UTC?

Works as you describe:

public function getTimezoneConfiguredDatePickerCombo(
        string $range = 'day',
        string $timezone = 'UTC',
    ): array {

        //DateTimePicker::configureUsing(fn(DateTimePicker $component) => $component->timezone($timezone));

        $now = Carbon::now()->timezone($timezone);
        $startDate = $now->toDateString();
        $endDate = $now->toDateString();

Does not work as you describe:

public function getTimezoneConfiguredDatePickerCombo(
        string $range = 'day',
        string $timezone = 'America/New_York',
    ): array {

        //DateTimePicker::configureUsing(fn(DateTimePicker $component) => $component->timezone($timezone));

        $now = Carbon::now()->timezone($timezone);
        $startDate = $now->toDateString();
        $endDate = $now->toDateString();

The problem only appears when switching time zones.

In the app where I ran into this issue, some Datepickers are using UTC, others are using EST.

danharrin commented 1 year ago

Yes I tested it with your exact example. Ensure that your apps config locale is still UTC.

madsem commented 1 year ago

It's UTC :)

But even the example repo doesn't behave the way you described above, when changing the timezone as above... When changing the function argument to EST America/New_York the end date jumps an entire day.

danharrin commented 1 year ago

I will try again soon, but if I can't replicate it again this issue will be closed and you'll have to debug it and submit a PR yourself. Is that ok?

madsem commented 1 year ago

sure, thanks :)

madsem commented 1 year ago

Just try to set it up like this:

public function getTimezoneConfiguredDatePickerCombo(
        string $range = 'month',
        string $timezone = 'America/New_York',
    ): array {

        //DateTimePicker::configureUsing(fn(DateTimePicker $component) => $component->timezone($timezone));

        $now = Carbon::now()->timezone($timezone);
        $startDate = $now->toDateString();
        $endDate = $now->toDateString();

It seems the issue becomes apparent when using a timezone different from the app timezone and month start + month end in datepicker.

At the current time, date picker 1 shows March 1, second shows April 1

danharrin commented 1 year ago

Hey, I've had another look. Was able to solve using $now = Carbon::now()->toImmutable();

https://user-images.githubusercontent.com/41773797/224559318-939fa441-fb58-4889-acad-fb4d07e46f04.mov

Let me know if this doesn't fix!

madsem commented 1 year ago

@danharrin sorry, that doesn't work. I think this really is a bug.

When you look at your solution, yes the datepicker behaves correct. But it's not anymore configured to use the timezone adjustment.

As soon as you introduce the timezone into the datepicker components, it starts jumping again:

/**
         * Must be immutable:
         * @link https://github.com/filamentphp/filament/issues/5601
         */
        $now = Carbon::now()->toImmutable()->timezone($timezone);
        $startDate = $now->toDateString();
        $endDate = $now->toDateString();

        if ($range == 'month') {
            $startDate = $now->startOfMonth();
            $endDate = $now->copy()->endOfMonth();
        }

        return [
            Grid::make()
                ->schema([
                    DateTimePicker::make('date_from')
                        ->timezone($now->timezone)
                        ->default($startDate)
                        ->format('yyyy-dd-mm')
                        ->withoutTime()
                        ->weekStartsOnMonday()
                        ->closeOnDateSelection(),
                    DateTimePicker::make('date_until')
                        ->timezone($now->timezone)
                        ->default($endDate)
                        ->format('yyyy-dd-mm')
                        ->withoutTime()
                        ->weekStartsOnMonday()
                        ->closeOnDateSelection(),
                ]),
        ];

PS:

The goal here, at least to my own logic, is to add the offset to the queries to get the results in the configured timezone. But it seems that it also uses the offset to display the dates...?

danharrin commented 1 year ago

The intended behaviour of Filament is to retrieve an app.timezone date object from your model, and return an app.timezone date object for saving. timezone() method on the date picker will convert the app.timezone to the custom timezone when the field loads, and then it will convert it back when the form is saved.

It sounds like you ate expecting different behaviour, if you are confused about why it is using the offset to display dates? I don't know what you mean by offsetting queries

madsem commented 1 year ago

I see, yes it is quite possible that I simply had a different expectation :)

I expected that if you configure the timezone, that db queries would be scoped to the timezone offset.

Ie: your app timezone is for example UTC, the queries made by a DateTimePicker configured to use a specific timezone would then contain the required time offset to query your dataset converted from UTC, to configured timezone on the picker instance.

In short, be able to display records in the configured timezone, not the app timezone.

danharrin commented 1 year ago

The DateTimePicker doesn't make database queries though, I don't know what you mean?

In short, be able to display records in the configured timezone, not the app timezone.

The current implementation of timezone() does this. In a form, it will display in the correct timezone. You just pass in a date in the app timezone.

madsem commented 1 year ago

Well, the DateTimePicker is used in the table builder, as a filter. So the selected dates are added as query parameters?

So I really thought that this is for displaying, vs saving data. I understand the intended usage now.

But I think displaying data in different time zones is a valid use case as well? So maybe a feature request

danharrin commented 1 year ago

Ah right, a filter. But the form component doesn't know it's being used in a filter. So the current state of the date picker in a filter will be in the configured timezone, not the app timezone.

So in your query(), you should just convert the date into the app timezone instead, and it should work fine.

madsem commented 1 year ago

Ah right, a filter. But the form component doesn't know it's being used in a filter. So the current state of the date picker in a filter will be in the configured timezone, not the app timezone.

So in your query(), you should just convert the date into the app timezone instead, and it should work fine.

Ahh, ok let me try that tomorrow morning, maybe that actually works.

The issue is that in some parts of the app I need to query the app timezone, in other parts I need to configure a different timezone for queries because the data in the app comes from outside sources (via apis)

Thank you @danharrin :) Super nice you're helping me like you do

danharrin commented 1 year ago

Heres the rules you need to follow:

modrictin commented 1 year ago

Hey Guys, I have an issue where I have the default timezone set to CET in my config file. This is causing this behavior where on each request the day goes to -1. I don't use any ->timezone() methods on my inputs.

modrictin commented 1 year ago

Hey @danharrin, can you please check my comment? I'm experiencing this when I have my default timezone different than UTC.

image

This is my date picker, it's pretty simple, but on each request it subtracts 1 day

Am I doing something wrong? Do I need to set the timezone to UTC for each Datepicker I create?

Arachnos commented 1 year ago

Hey everyone.

I have found a solution by casting the dates in the model like this :

'purchase_date' => 'date:Y',
'last_maintenance' => 'date:Y-m-d',
'available_on' => 'date:Y-m-d',

And now I don't have the datepicker substracting 1 day from the original date.

I have tested on 2.17.51

Hope this can help you !

You can also follow these comments from @marcreichel and @aliloubm :