Skip to content
This repository has been archived by the owner on May 16, 2018. It is now read-only.

Fix DateObject->subMonth() bug related to non-UTC timezones #693

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix DateObject->subMonth() bug related to non-UTC timezones #693

wants to merge 1 commit into from

Conversation

kylehendricks
Copy link

$parts = $this->getDateParts($this->mktime($hour, $minute, $second, $date, $day, $year, false));

This purpose of this line is to help see if the day of month (ex: 30th, 31st) exists in the month we are adding/subtracting to. The problem with it is that mktime call is taking into account the timezone but the getDateParts is not. This will cause problems with times that are "ahead a month" of UTC. (Ex: a +4 timezone at 1 am on the first of a month). We first convert from local timezone to UTC with the mktime, getDateParts is then called which returns a UTC time. We need the resulting "overflow" time to be in the local timezone.

@froschdesign
Copy link
Member

@kylehendricks
Thanks for the PR!

Please check the failure on Travis: https://travis-ci.org/zendframework/zf1/jobs/129276932#L498

@froschdesign
Copy link
Member

@kylehendricks
I can not found a signed CLA.

@kylehendricks
Copy link
Author

@froschdesign Can the CLA be e-signed?

@froschdesign
Copy link
Member

@kylehendricks

Can the CLA be e-signed?

No. Print, fill, scan and email it.

@froschdesign
Copy link
Member

@kylehendricks
Have you check the problems on Travis?

@kylehendricks
Copy link
Author

Just sent the CLA, looking into the test failure now.

@kylehendricks
Copy link
Author

@froschdesign I didn't notice all the tests in tests/Zend/DateTest.php so I moved my test into there.

I noticed that all the other Zend_Date add/sub month tests had a hidden dependency on the PHP default_timezone being set to the correct timezone that you are working with. My change should effectively eliminate that dependency.

@froschdesign
Copy link
Member

@kylehendricks

so I moved my test into there.

Please create a new test method. Thanks!

@froschdesign
Copy link
Member

@weierophinney
Can you look after the CLA? Thanks!

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

Successfully merging this pull request may close these issues.

2 participants