Add notification about DateTime and microseconds support #2186

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@SjonHortensius
Contributor
SjonHortensius commented Nov 5, 2016 edited

The addition of microseconds to DateTime are great; but looking at the 3v4l.org bughunt output I see hundreds of scripts unexpectedly 'failing' because two variables are no longer equal due to non-obvious microsecond differences (eg. https://3v4l.org/7rdeS ).

Hopefully this notice will prevent some bogus bugreports

@SjonHortensius SjonHortensius Add notification about DateTime and microseconds support
The addition of microseconds to DateTime are great; but looking at https://3v4l.org/bughunt/ I see hundreds of scripts unexpectedly 'failing' because two variables are no longer equal due to microsecond differences (eg. https://3v4l.org/7rdeS ).

Hopefully this notice will prevent some bogus bugreports
55b9e62
@Majkl578
Contributor
Majkl578 commented Nov 5, 2016 edited

I commented on this issue below the commit here, noone seems to care unfortunately. I'm going to properly report it as a bug (BC break) in RC.

@SjonHortensius
Contributor

I agree, see my related comment @ https://bugs.php.net/bug.php?id=73301. This change should have been added to 7.1.0 from the start; or moved to 7.2.

It seems some PHP developers handle a different definition then what is generally accepted:

A release candidate (RC) is a beta version with potential to be a final product, which is ready to release unless significant bugs emerge. In this stage of product stabilization, all product features have been designed, coded and tested through one or more beta cycles with no known showstopper-class bugs. A release is called code complete when the development team agrees that no entirely new source code will be added to this release.

I find it intriguing this specific change was added by @derickr who is normally very vocal against any change that creates BC behaviour (see various RFC votes)

@Majkl578
Contributor
Majkl578 commented Nov 5, 2016 edited

Hmm, nice catch with setTime BC break, it definitely breaks subclasses (and is not even documented). 😟
https://3v4l.org/Kmogl
Whole microseconds patch should be reverted and moved to 7.2 imho.

ccing RMs again: @dshafik @krakjoe

@derickr
Contributor
derickr commented Nov 5, 2016

I asked @dshafik before merging this large amount if bug fixes.
There is no BC break in any stable released versions.
Asking for this to be reverted means bug fixes don't make it out for another year.
Bugs can always be fixed regardless of whether we're in RC or not, even in patch releases.

@@ -280,6 +280,9 @@ PHP 7.1 UPGRADE NOTES
. Timezone initialization failure from serialized data will now throw an
instance of Error from __wakeup() or __set_state() instead of resulting in
a fatal error.
+ . DateTime instances now support microseconds. Please note this can have impact
+ when comparing; eg. this would previously return TRUE, but now returns FALSE:
+ (new DateTime('first day of next month') == new DateTime('first day of next month'));
@derickr
derickr Nov 5, 2016 Contributor

This is not a new change. This could result in false before if the first and second instantiation spanned a change in current second.

@SjonHortensius
SjonHortensius Nov 5, 2016 edited Contributor

True, but if generating a DateTime takes roughly 4 μs (which it does for me), this happens ~ 250.000 times more often

@derickr
derickr Nov 7, 2016 Contributor

True, but if generating a DateTime takes roughly 4 μs (which it does for me), this happens ~ 250.000 times more often

That doesn't make that the developer's original assumption was correct. There was still a bug in their code.

People not understanding an issue should not prevent fixing bugs. It's in the same league as not knowing about DST and assuming that each day is 86400 seconds long.

I will add something to UPGRADING, but with a better wording, so will be closing this PR.

@Majkl578
Contributor
Majkl578 commented Nov 5, 2016 edited

There is no BC break in any stable released versions.

Isn't there really? How about these?
https://3v4l.org/Kmogl
https://3v4l.org/4Z0et

  • another multiple issues with ==, >=, <= etc.

Don't they look like a BC break? They do, at least to me. The behavior obviously changed.

Asking for this to be reverted means bug fixes don't make it out for another year.

That is perfectly ok if they break BC, some of them actually look rather like feature requests...

Bugs can always be fixed regardless of whether we're in RC or not, even in patch releases.

Not if they break BC with stable versions. This makes any semver promises/expectations void.

@smalyshev smalyshev added the Quickfix label Nov 6, 2016
@arjenschol
Contributor
  1. There is no easy way to set microseconds to 0, you have to call setTime see https://3v4l.org/YUhFF A setMicroseconds method would be handy and/or support to relative strings to set microseconds to 0 (just like midnight does for H:i:s). Or am I missing something?
  2. Microsecond support is useful, by not by default I think. Why not introduce a 3rd parameter $with_microseconds/$set_microseconds which defaults to false? Or A second class DateTimeWithMicroseconds? Maybe not so fancy naming, but it's very clear what will happen.
  3. The 4th parameter to setTime() should not cause BC breakage.
@dshafik
Contributor
dshafik commented Nov 7, 2016

I concur with @arjenschol here, particularly on the BC breaks.

  1. I think default microseconds to 0 when unspecific for relative strings is correct behavior
  2. Preserving BC is critical, however introducing a new class also means a new immutable variant too. I'd be more in favor of an argument, but that's up to @derickr
  3. Agreed. Add a new method, maybe setMTime()? Having said this, examples of this type of usage are minimal on GitHub.
@jerrygrey
jerrygrey commented Nov 7, 2016 edited

Defaulting microseconds to zero, like seconds, should avoid this BC break and avoid the need for new classes and methods. But keep in mind, the final release of 7.1 is due in a few weeks and if this can't be fixed quickly, it should be reverted so that it doesn't be delayed 7.1

@@ -280,6 +280,9 @@ PHP 7.1 UPGRADE NOTES
. Timezone initialization failure from serialized data will now throw an
instance of Error from __wakeup() or __set_state() instead of resulting in
a fatal error.
+ . DateTime instances now support microseconds. Please note this can have impact
+ when comparing; eg. this would previously return TRUE, but now returns FALSE:
+ (new DateTime('first day of next month') == new DateTime('first day of next month'));
@derickr
derickr Nov 7, 2016 Contributor

True, but if generating a DateTime takes roughly 4 μs (which it does for me), this happens ~ 250.000 times more often

That doesn't make that the developer's original assumption was correct. There was still a bug in their code.

People not understanding an issue should not prevent fixing bugs. It's in the same league as not knowing about DST and assuming that each day is 86400 seconds long.

I will add something to UPGRADING, but with a better wording, so will be closing this PR.

@derickr derickr closed this Nov 7, 2016
@dshafik
Contributor
dshafik commented Nov 7, 2016

@derickr

  1. the change to the signature of setTime() is a valid concern that needs to be addressed.
  2. We should document that the correct way to get the relative date, should be something like: new DateTime('midnight first day of next month') (because adding midnight makes it not depend on current time)
  3. I support a third argument to toggle microseconds (off by default)

7.1.0RC6 is being built tomorrow or the next day, as the last RC, I'd like to see this resolved before then or we will have to revert. True bug fixes can be done in 7.1.1 and beyond.

@derickr
Contributor
derickr commented Nov 7, 2016

This pull request is unrelated to that dicussion.

@SjonHortensius
Contributor

FWIW; 69a3659

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment