Laravel-activitylog: timestamps are converted to utc on logging

[ctf0]

Describe the bug
for example i have a from & to date columns with values of
|from|to|
|-----|---|
|2021-02-23 00:00:00|2021-02-27 00:00:00|

but when logged they become
|from|to|
|-----|---|
|2021-02-22T22:00:00.000000Z | 2021-02-27T22:00:00.000000Z|

note the -2 hrs diff.

To Reproduce

• set config.app.timezone to something else ex.EET
• create a record
• check the saved log

Expected behavior
data should be logged as it is without any changes.

Versions (please complete the following information)

• PHP: PHP 8.0.2 (cli)
• Database: 8.0.23
• Laravel: 8.28.1
• Package: 3.16.1

[Gummibeer]

The value is still the same as the zulu offset is added. So if you say carbon to display & format it for a given timezone everything is fine.

I can recommend you to use https://github.com/jamesmills/laravel-timezone by @jamesmills to display your Datetimes.

[ctf0]

but why the package changes the value in the first place ?, shouldnt it be a copy / paste of the original ?

[Gummibeer]

We use the serializeDate() method on the model to ensure a string which we can save and the database understands.
https://github.com/spatie/laravel-activitylog/blob/ade270f291f4cb5883b3653919304a0b4e1cc284/src/Traits/DetectsChanges.php#L158-L162

Under the hood this method calls Carbon::toJSON() which again calls \Carbon\Traits\Converter::toISOString() and this methods drops timezone offset by default.

But you should always enforce a timezone when you need it instead of trusting Laravel or any package to properly keep your timezone. It makes it also easier to see that a given timezone is required there.

[ctf0]

is there a way i could hook into the activity creating event and set/cast the dates ?

[jamesmills]

I've used this package in a project before and I've used the https://github.com/jamesmills/laravel-timezone to output the logged event date/time to the user in their timezone. I'm a strong believer in setting and storing everything in UTC.

[ctf0]

@jamesmills
this would be valid if u r saving the data in one way everywhere and displaying it in a unified manner according to user prefs,
but when the data is saved in one way & logged in another, this creates wrong logs with changes not recognized from the original action.

@Gummibeer
enforcing such a strategy on the user without warning is a bad decision, if i set a timezone globally for the whole app, it means i expect that every single date in the app will follow that, otherwise its considered a bug.

[jamesmills]

Ah, I get you.

Mon, 22 Feb 2021 22:00:00 +0000 Coordinated Universal Time

It is saving it as UTC as the ISO standard but you are asking why it's not saving it in the config.app.timezone Timezone. It looks like the laravel-activity package saves all data as UTC be default. I would say that's a safe way of doing things. I guess when it pulls it out you could argue it _could_ look at the config.app.timezone value and output it as that. But you could just use the jamesmills/laravel-timezone package to output the date to the user. It will try use the users timezone and if that fails it will fallback to use the config.app.timezone (I think we added that recently).

Sorry if I'm not understanding or helping.

[Gummibeer]

It would be a bug if we drop the timezone/zulu offset which would make it impossible to format the value in a wanted timezone. But as the timezone is appended it's only a different representation of the same datetime.

The data display representation is up to the user of this package and part of this is the timezone you want to display the date in.

The only solution I see I would be open to merge is to use toISOString(true) instead of serializeDate(). This should solve your issue.

[ctf0]

for now here are a couple of solutions

1. save the date as expected

public static function logChanges(Model $model): array
{
  $changes = static::origLogChanges($model);

  foreach ($changes as $key => $val) {
      if ($model->isDateAttribute($key)) {
          $changes[$key] = (new Carbon($val))->setTimezone(config('app.timezone'))->toISOString(true);
      }
  }

  return $changes;
}

1. display the date as expected

protected function convertDate($item)
{
    if (strtotime($item)) {
        return (new Carbon($val))->setTimezone(config('app.timezone'))->format(...);
    }

    return $item;
}

[Gummibeer]

Both aren't solutions I would accept in the package as it will only enforce another timezone instead of keeping the one that the carbon instance has.
That's why I would use the toISOString() which can keep the offset of the current instance.

[ctf0]

the solutions would work as intended only if the all the dates in the app are saved/displayed according to app.timezone otherwise, you r correct using toISOString(true) would be the right one.

[nagi1]

Please would you PR a failing test descripting this issue. I wasn't able to reproduce this.

[tyler36]

Because this is a Laravel package, I think it should respect the Laravel timezone setting.

Perhaps it's "best practise" to save as UTC but I couldn't see anything on the website that suggests it disregards the config setting. And I think this can cause confusion for new-comers.

[Gummibeer]

After digging a bit into I reject this feature request in favor of display formatting (packages). The persisted value is 100% valid and equals the assigned one. Please note "equals" not the "same". 😉

$now = now();

$utc = $now->clone()->timezone('UTC');
$cet = $now->clone()->timezone('CET');

$utc->diff($cet);

// DateInterval {
//     interval: 0s,
// }

This formatting is even better than the default DB datetime/timestamp column as they totally lose track of the timezone or require a second column.
I will accept a PR adding a note to the docs. But this change would be breaking (not so bad as we prepare v4) but also super hard to get consistent results as Laravel already does some timezone formatting (to the app timezone). In case we would implement such a feature I would want to do it in a more general way and persist the timezone of the assigned carbon instance independent of the app timezone.
As the user is able to switch the ISO string to whatever timezone wanted without having to guess the persisted timezone I will close this issue for the moment.

Regarding the last argument by @tyler36 it's another argument for not persisting the timezone of carbon object, even if it wasn't his intent. Every time you cast a carbon instance to JSON in Laravel it uses the ISO format without offset as that's the carbon default one. As we don't have any read/get casting logic in place the user is anyway responsible to get the wanted value/object instance - so the user also knows the best which timezone is wanted right now as that can change per app, user, view and so on.