Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strange behavior with parse() when chaining addHours() and addMinutes() #1075

Closed
toddstoker opened this issue Jan 23, 2018 · 8 comments
Closed

Comments

@toddstoker
Copy link

toddstoker commented Jan 23, 2018

I'm experiencing some strange behavior. When using Carbon::parse() on an atom formatted string (e.g. '2018-01-23T03:30:45-5:00'), if addHours() and addMinutes() are chained then you will receive an erroneous result.

>>> $d = Carbon\Carbon::parse('2018-01-23T03:30:45-5:00')
=> Carbon\Carbon @1516696245 {#1042
     date: 2018-01-23 03:30:45.0 -05:00,
   }
>>> $d->addHours(1)->addMinutes(1)
=> Carbon\Carbon @1516681905 {#1042
     date: 2018-01-22 23:31:45.0 -05:00,
   }

Edit: Carbon 1.22.1, PHP 7.1.2

@toddstoker toddstoker changed the title Strange with parse() when chaining addHours() and addMinutes() Strange behavior with parse() when chaining addHours() and addMinutes() Jan 23, 2018
@Ovsyanka
Copy link
Contributor

I can't reproduce it (I have php 7.1.11 on the windows though).

Could you write the unit test with date_default_timezone_set() with your current timezone like this? Does it fail?

<?php

namespace Tests\Carbon;

use Carbon\Carbon;
use Tests\AbstractTestCase;

class issue1705Test extends AbstractTestCase
{
    public function testIssue() {
        date_default_timezone_set('America/New_York');
        $d = Carbon::parse('2018-01-23T03:30:45-5:00');
        $d->addHours(1)->addMinutes(1);
        $this->assertEquals('2018-01-23T04:31:45-05:00', $d->format('c'));
    }
}

@toddstoker
Copy link
Author

My default TZ is UTC, but if I change it to America/New_York it functions as expected.

Also I just tried this in another environment, where PHP is at 7.1.7, and I couldn't reproduce the issue.

@Ovsyanka
Copy link
Contributor

Ovsyanka commented Jan 24, 2018

So, this test failed in your environment, didn't it?

<?php

namespace Tests\Carbon;

use Carbon\Carbon;
use Tests\AbstractTestCase;

class issue1705Test extends AbstractTestCase
{
    public function testIssue() {
        date_default_timezone_set('UTC');
        $d = Carbon::parse('2018-01-23T03:30:45-5:00');
        $d->addHours(1)->addMinutes(1);
        $this->assertEquals('2018-01-23T04:31:45-05:00', $d->format('c'));
    }
}

Glavic added a commit to Glavic/Carbon that referenced this issue Feb 5, 2018
@Glavic
Copy link
Collaborator

Glavic commented Feb 5, 2018

Default TZ should have no saying here, since you already have TZ info in the string.

See note on $timezone parameter (http://php.net/manual/en/datetime.construct.php):

The $timezone parameter and the current timezone are ignored when the $time parameter either is a UNIX timestamp (e.g. @946684800) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00).

@toddstoker: this is a very strange bug. Are you sure you are not doing anything else with $d object in-between?

@Glavic Glavic closed this as completed Feb 12, 2018
@Ovsyanka
Copy link
Contributor

Ovsyanka commented Feb 13, 2018

It might be because changed modify method (see #883). So I ask this just for sure, that this is not the case. But newermind, seems like @toddstoker not interested in this issue anymore.

@Glavic
Copy link
Collaborator

Glavic commented Feb 13, 2018

We will fix that modify method ASAP.

@Ovsyanka
Copy link
Contributor

Btw, I already made pull-request for that a quite time ago: #1070

@Glavic
Copy link
Collaborator

Glavic commented Feb 13, 2018

True, but we must first fix travis and styleci, and then you should rebase, and if all goes well, we shall merge. Bear with us :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants