Issue with addSeconds and RFC dates after upgrade to 1.22.1

[jakelehner]

We use RFC3339 format in our API, and store dates in unix timestamp in our database.
After upgrading Carbon to 1.22.1, I'm seeing an bug with converting Carbon dates back to timestamp after doing any math on the object:

Carbon 1.21

$startTime = "2017-01-28T12:45:20-07:00";
$startTimeObj = new Carbon($startTime);

$startTimeObj->toRfc3339String();  // 2017-01-28T12:45:20-07:00
$startTimeObj->timestamp;          // 1485632720
$startTimeObj->addSeconds(10);
$startTimeObj->toRfc3339String();  // 2017-01-28T12:45:30-07:00 <-- +10 seconds
$startTimeObj->timestamp;          // 1485632730 <-- +10 seconds

Carbon 1.22.1

$startTime = "2017-01-28T12:45:20-07:00";
$startTimeObj = new Carbon($startTime);

$startTimeObj->toRfc3339String();  // 2017-01-28T12:45:20-07:00
$startTimeObj->timestamp;          // 1485632720
$startTimeObj->addSeconds(10);
$startTimeObj->toRfc3339String();  // 2017-01-28T12:45:30-07:00 <-- +10 seconds
$startTimeObj->timestamp;          // 1485607530 <-- +10 seconds - 7 Hours

It appears as if I'm losing the timezone reference after doing addSeconds(), but only for converting to UNIX timestamp. The RFC conversion still works as expected.

Can anyone else confirm or comment?

[jakelehner]

I think this is related to, or caused by the same core issue as, #863

[jakelehner]

The issue I'm experiencing seems to be coming from the new implementation of the modify() method. In 1.22, methods such as addSeconds() ended called DateTime's modify() method directly. In 1.21.1, The Carbon class contains the following method:

    /**
     * Consider the timezone when modifying the instance.
     *
     * @param string $modify
     *
     * @return static
     */
    public function modify($modify)
    {
        if ($this->local) {
            return parent::modify($modify);
        }

        $timezone = $this->getTimezone();
        $this->setTimezone('UTC');
        $instance = parent::modify($modify);
        $this->setTimezone($timezone);

        return $instance;
    }

I still haven't quite figured out how setting the timezone to UTC before the operation and back after is causing a problem, or why it's necessary, really. Regardless, after this operation, the unix timestamp returned will be for the original TIME in UTC, then adjusted for your time zone.

[lucasmichot]

See #862 (comment)

[jakelehner]

Thanks @lucasmichot. I'm not sure these issues are related, unfortunately. I pulled down your pull request branch and am experiencing the same problem.

We're using the RFC3339 format, rather than the ISO8601. I understand these formats are closely related, but from what I can tell the portion of code affected by your change isn't what's causing my issue.

The updates to the modify() method outlined in my previous comment is the culprit in my case, though I don't know why.

If it's of any value, the "timezone" object that gets pulled and reset in my test case is:

object(DateTimeZone)[2]
  public 'timezone_type' => int 1
  public 'timezone' => string '-07:00' (length=6)

Appears to be valid with the : still in tact.

[jakelehner]

Hi @lucasmichot ... I tried @Ovsyanka's branch from #884 and am experiencing the same results.

I see his change and it looks like it should work. However my code is not going in to his if statement that just calls the parent's modify method. Instead it still falls in to the else block and goes through the UTC adjustment causing this 'bug'.

Let me know if I can provide a test, or the crude code snippet i'm using on my side.

[lucasmichot]

Hey @jakelehner , yes can you provide your failing test?

[Ovsyanka]

I discovered that this bug relates to php DateTime bug. And i have workaround, may be relatively ugly, but it works for me.
I will write here additional info later, but i can say right now, that it is indirectly related to my pull-request #884. I think we should separate this issue from #884.

[Ovsyanka]

Problem is, that when you set timezone using 'HH:MM' notation then something going wrong in the DateTime object. After you modify object it returns wrong time for some formats (i dont know exactly which). But, after you run ->getTimestamp method all become normal;

I suppose testcase can be like that:

$date = Carbon::createFromTimestamp(0, '-07:00');
$date->addSeconds(1);
$this->assertSame('1', $date->format('U'));

[jakelehner]

Here is a crude test I put together. This passes on 1.21 but fails on 1.22.1.

$rfc3339Time = "2017-01-28T12:00:00-07:00";
$rfc3339TimeAssert = "2017-01-28T12:00:10-07:00";
$unixTimestampAssert = 1485630010;
$timeObj = new Carbon($rfc3339Time);
$timeObj->addSeconds(10);
$this->assertEquals($timeObj->toRfc3339String(), $rfc3339TimeAssert, "Invalid RFC3339 string after time addition.");
$this->assertEquals($timeObj->timestamp, $unixTimestampAssert, "Invalid unix timestamp after conversion.");

The first assertion will pass (retrieving the RFC string), but the second assertion (converting to unix timestamp) will fail.

[Ovsyanka]

It seems great to illustrate your initial problem, but i think that there may be enought to have only my test for base problem i desribed.
If you want me to add your test to my pull-request, that fixing this issue, then give me please a full code of the test with documentation block.

[Ovsyanka]

This error doesn't appear in 1.21 because it was introduced in commit 5394301. Before this commit there was not setTimezone methods in modify.

[jakelehner]

Doesn't matter to me which test is used as long as the use case is covered. I was posting per request of @lucasmichot and for completeness.

Noted regarding commit 5394301. I noticed the same but you seem to have more of an understanding as to the reasons behind the change.

[Ovsyanka]

Yeah, it is quite complicated case =) Let see what @lucasmichot will say about tests.

And i just clarify that your test passed in previous versions because there no hidden calls to setTimezone function. After that commit, the setTimezone method calls in ->modify() and this bug showing up.

[Ovsyanka]

You can vote to this bug on php bug tracker, but i dont know if it would make any difference )

[Ovsyanka]

I made the pull-request #900 fixing this issue.

[jakelehner]

@Ovsyanka, I can confirm this pull request addresses the issue stated in this thread.

[Glavic]

This issue was solved in v1.23.