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

Fixing #871, workaround of php bug [72338](https://bugs.php.net/bug.php?id=72338) #900

Merged
merged 1 commit into from
Feb 16, 2018

Conversation

Ovsyanka
Copy link
Contributor

No description provided.

@lucasmichot
Copy link
Collaborator

lucasmichot commented Feb 13, 2017

<3
Let me some time to review this @Ovsyanka

Copy link
Collaborator

@lucasmichot lucasmichot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good @Ovsyanka, just some comments

@@ -943,7 +943,11 @@ public function tz($value)
*/
public function setTimezone($value)
{
return parent::setTimezone(static::safeCreateDateTimeZone($value));
$date = parent::setTimezone(static::safeCreateDateTimeZone($value));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use $instance instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ofc, we can. Or even better, we can use $this. I will try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, i made it and updated the branch

*
* @internal I use days changing in tests because using seconds|minute|hours may run setTimezone within.
*/
class PhpBug72338Test extends AbstractTestCase
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would put this specific tests in the dedicated existing test classes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that you will do it by yourself? I dont know how modifying pull-request works, so if you need anything from me - just ask.

$date = Carbon::createFromTimestamp(0);
$date->setTimezone('+02:00');
$date->modify('+1 day');
$this->assertSame('86400', $date->format('U'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a stricter assertion would be fine too? as $date->format('U') always returns an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont get your point. What do you mean? $date->format('U') always returns string as i know.

Copy link
Contributor Author

@Ovsyanka Ovsyanka Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want we can discuss all this things on Discord (it has web-interface, so you don't need to install anything): https://discord.gg/FDbay

….net/bug.php?id=72338)

The problem is, that $date->setTimezone($tz) with $tz in 'HH:MM' notation (["timezone_type"]=>int(1)) put DateTime object on inconsistent state. It looks like internal timestamp becomes changed and it affects to such functions:
* $date->modify() uses changed timestamp and result is wrong
* $date->setTimezone($tz) settle this changed timestamp, even in case if $tz is not in 'HH:MM' format
* $date->format('U') returns changed timestamp

I founded the workaround: if you run DateTime::getTimestamp() method after setting such timezone, internal structure of DateTime seems fixed - it behavior looks right in such cases described above. So i added call to getTimestamp() in setTimezone().
@kylekatarnls kylekatarnls merged commit a918399 into briannesbitt:master Feb 16, 2018
@Ovsyanka Ovsyanka deleted the issue-871 branch February 16, 2018 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants