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

Bug #68161 - DateTime::setTimestamp doesn't clear relative data #870

Closed
wants to merge 1 commit into from

Conversation

lt
Copy link
Contributor

@lt lt commented Oct 8, 2014

Fixes PHP bug #68161 - Explicitly setting a timestamp does not clear previously set relative data.

@lt lt changed the title Clear relative ts info when setting a timestamp Bug #68161 - DateTime::setTimestamp doesn't clear relative data Oct 8, 2014
@lt
Copy link
Contributor Author

lt commented Oct 8, 2014

I would like a second opinion on the expected behaviour here, with the following two methods:

DateTime::setTimestamp -- date_timestamp_set — Sets the date and time based on an Unix timestamp

DateTime::modify -- date_modify — Alters the timestamp

The bug report identifies that if modify is used before setTimestamp, then the modifications are applied to this timestamp too. Is this expected behaviour? (/cc @derickr)

I can see how this can both be viewed as unexpected, and also useful, as in the following:

$timestamps = [ ... list of timestamps ... ];
$firstDOM = new DateTime();
$firstDOM->modify('first day of this month');

foreach ($timestamps as $ts) {
    $firstDOM->setTimeStamp($ts);
    // do something that requires the first day of the month for many timestamps
} 

If it's decided the current behaviour is the expected behaviour, I will modify the test to ensure this behaviour remains unaltered.

@derickr
Copy link
Member

derickr commented Oct 8, 2014

The modifications should happen only once, and not stick around.

Your patch is not correct though, the unsetting of relative needs to happen after modify() has run the modification, and not in the setTimestamp() method as that only addresses the symptom, and not the problem.

@derickr
Copy link
Member

derickr commented Oct 8, 2014

I would also target PHP 5.4 with the patch.

@datibbaw
Copy link
Contributor

datibbaw commented Oct 8, 2014

@derickr Problem with targeting 5.4 is that Travis doesn't run the test suite for that branch =(

@lt
Copy link
Contributor Author

lt commented Oct 8, 2014

Thanks @derickr, I will address this tonight.

@lt
Copy link
Contributor Author

lt commented Oct 8, 2014

And, 5.4 is security fixes only.

@smalyshev
Copy link
Contributor

I'm not sure I understand this patch - why save and restore relative time? Shouldn't relative part be reset if we set explicit timestamp?

@lt
Copy link
Contributor Author

lt commented Dec 1, 2014

I think I've failed to push a commit. I made a change based on Derick's feedback. I'll see if I can track it down.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 91d4185 on lt:bug-68161 into * on php:PHP-5.5*.

@krakjoe
Copy link
Member

krakjoe commented Jan 3, 2017

The original bug is now marked closed on the systems bug tracker, so assume this is fixed in supported branches.

@krakjoe krakjoe closed this Jan 3, 2017
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.

6 participants