jamesmills / laravel-timezone

Enable user Timezones in your application.
MIT License
675 stars 89 forks source link

Laravel 10 future support. #89

Open jackkitley opened 1 year ago

jackkitley commented 1 year ago

Hi James

Right now if a new data is declared with protected $dates it gets converted with base timezone helper.

The new docs have changed this:

`Model "Dates" Property Likelihood Of Impact: Medium

The Eloquent model's deprecated $dates property has been removed. Your application should now use the $casts property:

protected $casts = [ 'deployed_at' => 'datetime', ];`

Will it still work as intended?

Thanks

jamesmills commented 1 year ago

In all honesty I’ve no idea.

If you’re looking at this can you test it and let me know?

I suspect the ideal solution would be to add a custom cast to the package.

jackkitley commented 1 year ago

Hi.

We have tested and it doesnt work anymore. $dates property used to work fine but has since been deprecated.

Thanks!

protected $casts = [
    'trade_ins'           => DealTradeInsCast::class,
    'accessories'         => SummedListCast::class,
    'fi_plans'            => SummedListCast::class,
    'fees'                => SummedListCast::class,
    'summary'             => DealSummaryCast::class,
    'vehicle_of_interest' => DealVehicleOfInterestCast::class,
    'valid_until'         => 'datetime:Y-m-d H:i:s',
    'deal_type'           => DealType::class,
    'financed_by'         => FinancedBy::class,
    'type'                => Type::class
];

we are testing 'valid_until' => 'datetime:Y-m-d H:i:s',

jackkitley commented 1 year ago

This works:

protected function validUntil(): Attribute { return Attribute::make( get: fn (string $value) => (new Timezone())->convertToLocal(Carbon::createFromDate($value)), ); }

This used to work when i declared a date with $dates property:

class BaseModel extends Model { protected function serializeDate(DateTimeInterface $date): string { return (new Timezone())->convertToLocal(Carbon::instance($date)); } }

boryn commented 1 year ago

IMHO this does not break anything.

Just make sure that the first parameter passed to convertToLocal() is indeed a Carbon instance.

I have something like this:

protected $casts = [
    'created_at' => 'datetime:Y-m-d H:i:s',
];

and echo get_class($item->created_at); shows: Illuminate\Support\Carbon which is expected (and no mutators or accessors are used in the model).

jackkitley commented 1 year ago

@boryn try a custom datetime column from mysql and try to cast it the same way.

It doesn't convert

boryn commented 1 year ago

I tried with:

protected $casts = [
        'start_date' => 'datetime:Y-m-d H:i:s',
        'stop_date'  => 'datetime:Y-m-d H:i:s',
    ];

where start date is defined as simple date in MySQL: $table->date('start_date')->nullable()

and I get:

Illuminate\Support\Carbon Object
(
    [endOfTime:protected] => 
    [startOfTime:protected] => 
    [constructedObjectId:protected] => 0000000000000ab40000000000000000
    [localMonthsOverflow:protected] => 
    [localYearsOverflow:protected] => 
    [localStrictModeEnabled:protected] => 
    [localHumanDiffOptions:protected] => 
    [localToStringFormat:protected] => 
    [localSerializer:protected] => 
    [localMacros:protected] => 
    [localGenericMacros:protected] => 
    [localFormatFunction:protected] => 
    [localTranslator:protected] => 
    [dumpProperties:protected] => Array
        (
            [0] => date
            [1] => timezone_type
            [2] => timezone
        )

    [dumpLocale:protected] => 
    [dumpDateProperties:protected] => 
    [date] => 2023-03-05 00:00:00.000000
    [timezone_type] => 3
    [timezone] => America/New_York
)

so it seems to be working. Maybe you have somewhere some additional accessor that modifies the value, maybe getValidUntilAttribute()? Try to var_dump() this value at different places in the code.

Actually what error do you get after running directly: (new Timezone())->convertToLocal($this->valid_until); without any accessors?

Have you tried running statically Timezone::convertToLocal($this->valid_until); (remember of using different use statement then)?

jackkitley commented 1 year ago

@boryn serializeDate doesn't pick up custom dates is the issue.

https://laravel.com/docs/10.x/eloquent-serialization#customizing-the-default-date-format

boryn commented 1 year ago

What do you mean by "doesn't pick up custom dates"? The serialization date format like 'Y-m-d H:i:s' is only applied when you use ->toArray() or ->toJson() on your model.

jackkitley commented 1 year ago

@boryn yes. It does now but that's not how views are rendered. This plug-in used to work with laravel 9 just fine declaring custom dates with dates property.

I have the serializeDate in a baseModel and used to convert the readable dates to the useds timezone on the fly.

Now I need to exclude the formatting of the date to have it work.