When using a default timezone other than UTC and calling transitionTo, datetime values are shifted by the timezone modifier

[tmas]

Q	A
Bug?	yes
New Feature?	no
Framework	Laravel
Framework version	8.83.7
Package version	13.0.2
PHP version	7.4.16

Actual Behaviour

I have the default timezone for my project set to "America/New_York" in the app.timezone config option. Prior to Laravel changing the way date serialization works in 7.x, the package was functioning normally.

Now if I transition to an audit and a date value on the model is loaded from the audit, the date value is shifted forward in time by 5 hours (the inverse of the timezone offset).

I originally mentioned this issue in the Discord and explained it in much greater detail - you can find that detailed explanation in this pastebin.

The quick version of the explanation is that the getModified function on the Audit class treats any value that implements DateTimeInterface differently from other values. Specifically if a value implements DateTimeInterface it will be passed through the auditable's serializeDate function instead of simply being returned like other values. With the new Laravel behavior as of 7.x, serializeDate converts the timezone to UTC and returns an ISO 8601 datetime string which ends in Z to indicate that it's UTC. When this value is eventually passed to the setAttribute function the timezone is ignored and the default timezone is applied, because that function expects any dates to be passed in using the datetime format for the current database.

Expected Behaviour

Date values should be transitioned to the value stored on the audit and maintain their original value, and ideally their original timezone.

Steps to Reproduce

I posted a repository to Github that reproduces the issue, including a test showing exactly what happens:

1. git clone [email protected]:tmas/laravel-audit-timezone-bug
2. cd laravel-audit-timezone-bug
3. composer install
4. Set up a database connection
5. php artisan migrate
6. php artisan test

I have changed 2 of the configuration options in that project. First, app.timezone is set to "America/New_York". This is required to reproduce the issue. Second, audit.console is set to true. This is not required to reproduce the issue, but makes it much easy to poke around using php artisan tinker.

I'll also link the pastebin I mentioned in a previous section again since it seems like it fits better here: https://pastebin.com/BUSGeGS3

Possible Solutions

I propose removing this section from the getModified function:

if ($value instanceof DateTimeInterface) {
    $modified[$attribute][$state] = !is_null($this->auditable)
        ? $this->auditable->serializeDate($value)
        : $this->serializeDate($value);
}

In my testing, everything works properly if that code is removed. I'm not sure what made it necessary in the first place, but passing a value that implements DateTimeInterface to the setAttribute function works fine and this code block converts the DateTimeInterface to a string which isn't handled as expected when passed to setAttribute. As far as I can tell the getModified function is only ever called in one place in the transitionTo function on the Auditable trait, so I'm fairly certain removing this code block won't have any side effects.

[MortenDHansen]

Thank you for the thorough report! - and i did see the post in discord!

It is on top of my to-do. I need to make sure we do it right and don't break backwards. When serialization changed in laravel 7, the package wasn't correctly updated so a few fixes has been applied, but i feel like we need to make sure it works consistently both with the presenting logic, various databases and that the behaviour is documented if need be.

I do need some hours of focus for it though. Those hours a few and far apart sometimes!

[erikn69]

#775 is an idea, @tmas It seems you are up to date with this problem. could you test it? ping @MortenDHansen

[MortenDHansen]

@erikn69 - right! I'll give it a run

[tmas]

@erikn69 Thanks for putting together a PR for this. I just pulled up the repository I created to reproduce the bug, and it looks like my test is passing now!

[tmas]

Another thought: it may be worth pulling the test from my reproduction repo into this project. I'm not super familiar with PHPUnit so I'm not sure how you'd handle setting up the timezone config without also affecting other tests, but I imagine there's probably a way to do it.

This is the test in question, if anyone wants to give it a shot: https://github.com/tmas/laravel-audit-timezone-bug/blob/master/tests/Feature/TimesRestoredCorrectlyTest.php

[erikn69]

@tmas i did already copy your test on mi PR, is that what you meant?

[tmas]

@erikn69 Whoops, I completely overlooked that! It looks like you have the timezone set up and everything, my only concern is that I'm not sure if the updated config gets reset to its original values after the test runs. Given that I'm not familiar with PHPUnit, that may be a non-issue.

[erikn69]

I'm not sure if the updated config gets reset to its original values after the test runs

I asked myself the same thing, so I did some tests and it seems that everything is reset in each test(UTC as default again)