Re: [PHP-DEV] DateTime microseconds discussion
On 8 November 2016 at 09:32, Arjen Schol wrote: > Hi, > > Support for microseconds was added late in the 7.1 RC cycle, however is has > some issues: My understanding is that if this affects your code, then your code already has a bad assumption in it. The code is mistakenly assuming that two DateTime's generated will have the same the same time for both of them.* I don't fully understand what problem you are want to solve through this discussion. No-one is forcing you to update to PHP 7.1 immediately; you should have plenty of time to fix the code that has bad assumptions about the time in a generated DateTime before you upgrade to PHP 7.1. SjonHortensius wrote: > if generating a DateTime takes roughly 4 μs (which it does for me), this > happens ~ 250.000 times > more often Having bugs happen more frequently is a good thing. It stops you from ignoring edge cases and programming by coincidence.** I realise that having a release make it more obvious that broken code is broken is an annoying thing to have to fixbut that code is already wrong. Delaying useful features, just to avoid having to fix flaky code is not a good way for a project to proceed, imo. dshafik wrote: > I think default microseconds to 0 when unspecific for relative strings is > correct behavior I don't think that sounds like a good plan at all. When creating a DateTime through `new DateTime('first day of next month');` only the date values are set from the input string, the rest of the time is set to now(). Wouldn't having microseconds behave differently to the rest of the time values would be instantly an extra piece of inconsistency for people to regret? cheers Dan * Example code that doesn't work correctly now. If the sleep period is reduced, it becomes less obvious that the code is broken, but it is still broken. for ($i=0; $i<10; $i++) { $dt1 = new DateTime('first day of next month'); usleep(rand(0, 50)); $dt2 = new DateTime('first day of next month'); if ($dt1 == $dt2) { echo "Same\n"; } else { echo "Different\n"; } } ** Programming by Coincidence - https://pragprog.com/the-pragmatic-programmer/extracts/coincidence -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] DateTime microseconds discussion
Hi Dan, I think you make some bad assumptions here. The examples provided by Sjon are scripts submitted to 3v4l.org They may have bad assumptions, but are real life examples of DateTime usage. And they will break. We have two issues in our codebase. We ARE testing RC release, I think feedback should be taken seriously. 1. We overloaded DateTime::setTime, this needed an update because of the extra parameter (point 3). BC break. 2. We test the following: A. $date = new DateTime; B. Store in database. C. Retrieve datetime. D. $fromDatabase = new DateTime(storedDateTime); E. $fromDatabase == $date (which fails now, because $date has microsecond precision while $fromDatabase doesn't). Where does this have a bad assumption? We tested an assumption (DateTime has seconds precision), and now it fails. Is that a bad assumption? I think it's just backward breaking with no good way to fix the code. We could set microseconds to 0, which is cumbersome because of a missing setMicroseconds method. One needs to call setTime with retrieved hours, minutes, seconds or setTimestamp($dt->format('U'))... "Rely only on reliable things."... Arjen On 11/08/2016 12:03 PM, Dan Ackroyd wrote: On 8 November 2016 at 09:32, Arjen Schol wrote: Hi, Support for microseconds was added late in the 7.1 RC cycle, however is has some issues: My understanding is that if this affects your code, then your code already has a bad assumption in it. The code is mistakenly assuming that two DateTime's generated will have the same the same time for both of them.* I don't fully understand what problem you are want to solve through this discussion. No-one is forcing you to update to PHP 7.1 immediately; you should have plenty of time to fix the code that has bad assumptions about the time in a generated DateTime before you upgrade to PHP 7.1. SjonHortensius wrote: if generating a DateTime takes roughly 4 μs (which it does for me), this happens ~ 250.000 times more often Having bugs happen more frequently is a good thing. It stops you from ignoring edge cases and programming by coincidence.** I realise that having a release make it more obvious that broken code is broken is an annoying thing to have to fixbut that code is already wrong. Delaying useful features, just to avoid having to fix flaky code is not a good way for a project to proceed, imo. dshafik wrote: I think default microseconds to 0 when unspecific for relative strings is correct behavior I don't think that sounds like a good plan at all. When creating a DateTime through `new DateTime('first day of next month');` only the date values are set from the input string, the rest of the time is set to now(). Wouldn't having microseconds behave differently to the rest of the time values would be instantly an extra piece of inconsistency for people to regret? cheers Dan * Example code that doesn't work correctly now. If the sleep period is reduced, it becomes less obvious that the code is broken, but it is still broken. for ($i=0; $i<10; $i++) { $dt1 = new DateTime('first day of next month'); usleep(rand(0, 50)); $dt2 = new DateTime('first day of next month'); if ($dt1 == $dt2) { echo "Same\n"; } else { echo "Different\n"; } } ** Programming by Coincidence - https://pragprog.com/the-pragmatic-programmer/extracts/coincidence -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] DateTime microseconds discussion
Hey Arjen, On 8 November 2016 at 09:32, Arjen Schol wrote: There is no easy way to set microseconds to 0, you have to call setTime There actually is an easy way, you can pass microseconds absolute value in the constructor as well, it would like: `new DateTime("5 minutes ago, 0 microseconds")` As Dan said it is probably a mistkae in the code, really, and it is a very popular one, to think that `new DateTime("5 minutes ago") == new DateTime("5 minutes ago")`. If you sample it enough times even on pre-7 versions of PHP you will get inequality as well: https://3v4l.org/JV59e (sorry for overloading 3v4l a bit here). It is especially an undesirable mistake because it causes failures randomly, no one likes their test suite failing /sometimes/, it makes debugging a very unpleasant experience. When doing date manipulations, let's say the task is to generate a report for the previous month, you need to always have a base date as a point of reference: ``` $startDate = new DateTimeImmutable("first day of previous month, 00:00:00.0"); $endDate = $startDate->add(new DateInterval("P1M")); ``` -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] DateTime microseconds discussion
Hi Nikita, The https://3v4l.org/YUhFF example was to demonstrate it is NOT easy to set microseconds to zero. NOT to demonstrate new DateTime("5 minutes ago") == new DateTime("5 minutes ago"). new DateTime("5 minutes ago, 0 microseconds") indeed sets the microseconds to zero (nice to know) but this does not work for 'now'. See https://3v4l.org/8VE7W for new DateTime('now, 0 microseconds'); And this code fails < 7.1RC4. Arjen On 11/08/2016 01:24 PM, Nikita Nefedov wrote: Hey Arjen, On 8 November 2016 at 09:32, Arjen Schol wrote: There is no easy way to set microseconds to 0, you have to call setTime There actually is an easy way, you can pass microseconds absolute value in the constructor as well, it would like: `new DateTime("5 minutes ago, 0 microseconds")` As Dan said it is probably a mistkae in the code, really, and it is a very popular one, to think that `new DateTime("5 minutes ago") == new DateTime("5 minutes ago")`. If you sample it enough times even on pre-7 versions of PHP you will get inequality as well: https://3v4l.org/JV59e (sorry for overloading 3v4l a bit here). It is especially an undesirable mistake because it causes failures randomly, no one likes their test suite failing /sometimes/, it makes debugging a very unpleasant experience. When doing date manipulations, let's say the task is to generate a report for the previous month, you need to always have a base date as a point of reference: ``` $startDate = new DateTimeImmutable("first day of previous month, 00:00:00.0"); $endDate = $startDate->add(new DateInterval("P1M")); ``` -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] DateTime microseconds discussion
On Tue, 8 Nov 2016, Arjen Schol wrote: > Hi, > > Support for microseconds was added late in the 7.1 RC cycle, however is has > some issues: Some *additional* support for microseconds was added in the PHP 7.1 cycle, mostly to support bug fixes that have been around for a long time. > > 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? - You can't set just the seconds or minute portion alone either, through setTime(). Microseconds are just an extension of the time portion. - Using "midnight" also sets the microseconds to 0: [PHP: 7.1.0-dev ] derick@whisky:~ $ cat /tmp/midnight.php modify( 'midnight' ) ); ?> [PHP: 7.1.0-dev ] derick@whisky:~ $ php -n /tmp/midnight.php object(DateTimeImmutable)#1 (3) { ["date"]=> string(26) "2016-11-08 12:01:20.023680" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } object(DateTimeImmutable)#2 (3) { ["date"]=> string(26) "2016-11-08 00:00:00.00" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } > 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? A 3rd parameter to which function? > Or A second class DateTimeWithMicroseconds? Maybe not so fancy naming, > but it's very clear what will happen. But that would be a duplicate. The already existing DateTime and DateTimeImmutable classes already support microseconds. Adding an extra class for them, would just be confusing. > 3. The 4th parameter to setTime() should not cause BC breakage. Added additional new arguments with a default value, is not a BC break. cheers, Derick -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] DateTime microseconds discussion
Hi, On Tue, 8 Nov 2016, Arjen Schol wrote: > I think you make some bad assumptions here. Please don't top-reply on this list. > The examples provided by Sjon are > scripts submitted to 3v4l.org They may have bad assumptions, but are real life > examples of DateTime usage. And they will break. They already could break. As Dan wrote better: > Having bugs happen more frequently is a good thing. It stops > you from ignoring edge cases and programming by coincidence. > We have two issues in our codebase. We ARE testing RC release, I think > feedback should be taken seriously. Taking things seriously does not mean having to agree. > 1. We overloaded DateTime::setTime, this needed an update because of the extra > parameter (point 3). BC break. PHP throws a warning here, but the expected result of the code execusion is correct: [PHP: 7.1.0-dev ] derick@whisky:/tmp $ cat doo.php setTime( 2, 5, 10 ); var_dump( $a, $b ); ?> [PHP: 7.1.0-dev ] derick@whisky:/tmp $ php -n doo.php Warning: Declaration of MyDate::setTime($h, $i, $s) should be compatible with DateTimeImmutable::setTime($hour, $minute, $second = NULL, $microseconds = NULL) in /tmp/doo.php on line 9 object(MyDate)#1 (3) { ["date"]=> string(26) "2016-11-08 13:30:06.371428" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } object(MyDate)#2 (3) { ["date"]=> string(26) "2016-11-08 02:10:10.00" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } This is at *most* a BC break because it shows a warning. The code itself isn't broken. In PHP 5.6, this was a E_STRICT warning, which got converted to E_WARNING in PHP 7.0: [PHP: 5.6.28-dev ] derick@whisky:~/dev/php/php-src.git $ php -n -derror_reporting=-1 -ddate.timezone=UTC /tmp/doo.php Strict Standards: Declaration of MyDate::setTime() should be compatible with DateTimeImmutable::setTime($hour, $minute, $second = NULL) in /tmp/doo.php on line 9 … [PHP: 7.0.13-dev ] derick@whisky:~/dev/php/php-src.git $ php -n -derror_reporting=-1 -ddate.timezone=UTC /tmp/doo.php Warning: Declaration of MyDate::setTime($h, $i, $s) should be compatible with DateTimeImmutable::setTime($hour, $minute, $second = NULL) in /tmp/doo.php on line 9 … As this specific overloading does *not* violate the Liskov Substitution Principle, I would argue that it is this *warning* that is the BC break in PHP 7.0, and not the addition of the extra argument-with-default-value. > 2. We test the following: > > A. $date = new DateTime; > B. Store in database. > C. Retrieve datetime. > D. $fromDatabase = new DateTime(storedDateTime); > E. $fromDatabase == $date (which fails now, because $date has microsecond > precision while $fromDatabase doesn't). If you don't store microseconds in the database, then I expect that not to work. It's like if you wouldn't store seconds in the database, that you wouldn't get the same result either. > Where does this have a bad assumption? We tested an assumption (DateTime has > seconds precision), and now it fails. Is that a bad assumption? I think it's > just backward breaking with no good way to fix the code. The bad assumption is, that DateTime has always had microseconds precision, and the bug that got fixes was, that they did not always get correctly initialised: [PHP: 7.0.13-dev ] derick@whisky:~/dev/php/php-src.git $ cat doo2.php [PHP: 7.0.13-dev ] derick@whisky:~/dev/php/php-src.git $ php -n doo2.php object(DateTimeImmutable)#1 (3) { ["date"]=> string(26) "2016-11-08 13:39:04.123456" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } > We could set microseconds to 0, which is cumbersome because of a > missing setMicroseconds method. One needs to call setTime with > retrieved hours, minutes, seconds or setTimestamp($dt->format('U'))... As you're overloading the DateTime class anyway, that's merely a minor inconvenience. cheers, Derick -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] DateTime microseconds discussion
On 11/08/2016 04:16 AM, Arjen Schol wrote: Hi Dan, I think you make some bad assumptions here. The examples provided by Sjon are scripts submitted to 3v4l.org They may have bad assumptions, but are real life examples of DateTime usage. And they will break. They are already broken. That's the point. Rarely will the current breakage trigger but it will trigger. Personally I prefer it when bad code obviously triggers, it can be very difficult (read expensive) to track bugs in code that only rarely and randomly trigger. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] DateTime microseconds discussion
On Tue, Nov 8, 2016 at 6:28 AM, Alice Wonder wrote: > On 11/08/2016 04:16 AM, Arjen Schol wrote: > >> Hi Dan, >> >> I think you make some bad assumptions here. The examples provided by >> Sjon are scripts submitted to 3v4l.org They may have bad assumptions, >> but are real life examples of DateTime usage. And they will break. >> > > They are already broken. That's the point. > > Rarely will the current breakage trigger but it will trigger. > > Personally I prefer it when bad code obviously triggers, it can be very > difficult (read expensive) to track bugs in code that only rarely and > randomly trigger. I have discussed this with Joe, and we are OK with the warning BC break; everything else is not considered a BC break. We should definitely document some best practices around relative dates, e.g. using midnight, or explicitly setting the time. Thanks, - Davey
Re: [PHP-DEV] DateTime microseconds discussion
On 11/08/2016 02:49 PM, Derick Rethans wrote: Hi, On Tue, 8 Nov 2016, Arjen Schol wrote: I think you make some bad assumptions here. Please don't top-reply on this list. The examples provided by Sjon are scripts submitted to 3v4l.org They may have bad assumptions, but are real life examples of DateTime usage. And they will break. They already could break. As Dan wrote better: > Having bugs happen more frequently is a good thing. It stops > you from ignoring edge cases and programming by coincidence. We have two issues in our codebase. We ARE testing RC release, I think feedback should be taken seriously. Taking things seriously does not mean having to agree. Taking things seriously means reading the complete message and not focussing on a detail of an example (https://3v4l.org/YUhFF faces the same problem without constructor argument; e.g. not easy to set microseconds to 0). I've never said we were facing new DateTime == new DateTime problems, but Dan gives a lesson on bad assumptions... No comment on not being able to set microtime to zero. 1. We overloaded DateTime::setTime, this needed an update because of the extra parameter (point 3). BC break. PHP throws a warning here, but the expected result of the code execusion is correct: [PHP: 7.1.0-dev ] derick@whisky:/tmp $ cat doo.php setTime( 2, 5, 10 ); var_dump( $a, $b ); ?> [PHP: 7.1.0-dev ] derick@whisky:/tmp $ php -n doo.php Warning: Declaration of MyDate::setTime($h, $i, $s) should be compatible with DateTimeImmutable::setTime($hour, $minute, $second = NULL, $microseconds = NULL) in /tmp/doo.php on line 9 object(MyDate)#1 (3) { ["date"]=> string(26) "2016-11-08 13:30:06.371428" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } object(MyDate)#2 (3) { ["date"]=> string(26) "2016-11-08 02:10:10.00" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } This is at *most* a BC break because it shows a warning. The code itself isn't broken. In PHP 5.6, this was a E_STRICT warning, which got converted to E_WARNING in PHP 7.0: [PHP: 5.6.28-dev ] derick@whisky:~/dev/php/php-src.git $ php -n -derror_reporting=-1 -ddate.timezone=UTC /tmp/doo.php Strict Standards: Declaration of MyDate::setTime() should be compatible with DateTimeImmutable::setTime($hour, $minute, $second = NULL) in /tmp/doo.php on line 9 … [PHP: 7.0.13-dev ] derick@whisky:~/dev/php/php-src.git $ php -n -derror_reporting=-1 -ddate.timezone=UTC /tmp/doo.php Warning: Declaration of MyDate::setTime($h, $i, $s) should be compatible with DateTimeImmutable::setTime($hour, $minute, $second = NULL) in /tmp/doo.php on line 9 … As this specific overloading does *not* violate the Liskov Substitution Principle, I would argue that it is this *warning* that is the BC break in PHP 7.0, and not the addition of the extra argument-with-default-value. I think it DOES violate (and hence the warning) LSP. If you DO use the 4th microseconds argument in your application, you cannot replace a DateTime object with CustomDate which does not have this 4th argument, even tough DateTime has a default value for it. If you do want to save a time with microseconds by setTime, you end up without microseconds if you replace DateTime with CustomDate. "[..] if S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e., an object of the type T may be substituted with its subtype object of the type S) without altering any of the desirable properties of that program (correctness, task performed, etc.)" 2. We test the following: A. $date = new DateTime; B. Store in database. C. Retrieve datetime. D. $fromDatabase = new DateTime(storedDateTime); E. $fromDatabase == $date (which fails now, because $date has microsecond precision while $fromDatabase doesn't). If you don't store microseconds in the database, then I expect that not to work. It's like if you wouldn't store seconds in the database, that you wouldn't get the same result either. This used to work for 7 years, and now it's not. If I hadn't stored seconds, it wouldn't have worked for 7 years. Where does this have a bad assumption? We tested an assumption (DateTime has seconds precision), and now it fails. Is that a bad assumption? I think it's just backward breaking with no good way to fix the code. The bad assumption is, that DateTime has always had microseconds precision, and the bug that got fixes was, that they did not always get correctly initialised: First, it was virtually impossible to create a DateTime object with the current time WITH microseconds set (DateTime::createFromFormat('U.u', sprintf('%.f', microtime(true))); only method AFAIK) and now it is cumbersome to create a
Re: [PHP-DEV] DateTime microseconds discussion
On 11/08/2016 01:46 PM, Derick Rethans wrote: On Tue, 8 Nov 2016, Arjen Schol wrote: Hi, Support for microseconds was added late in the 7.1 RC cycle, however is has some issues: Some *additional* support for microseconds was added in the PHP 7.1 cycle, mostly to support bug fixes that have been around for a long time. 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? - You can't set just the seconds or minute portion alone either, through setTime(). Microseconds are just an extension of the time portion. - Using "midnight" also sets the microseconds to 0: new DateTime('now midnight') obviously won't work... [PHP: 7.1.0-dev ] derick@whisky:~ $ cat /tmp/midnight.php modify( 'midnight' ) ); ?> [PHP: 7.1.0-dev ] derick@whisky:~ $ php -n /tmp/midnight.php object(DateTimeImmutable)#1 (3) { ["date"]=> string(26) "2016-11-08 12:01:20.023680" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } object(DateTimeImmutable)#2 (3) { ["date"]=> string(26) "2016-11-08 00:00:00.00" ["timezone_type"]=> int(3) ["timezone"]=> string(3) "UTC" } 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? A 3rd parameter to which function? With 3rd argument I meant the constructor: public DateTime::__construct ([ string $time = "now" [, DateTimeZone $timezone = NULL, [ $initialize_microseconds = false ]]] ) When microseconds are NOT specified in $time, don't set them unless $initialize_microseconds is true. Or A second class DateTimeWithMicroseconds? Maybe not so fancy naming, but it's very clear what will happen. But that would be a duplicate. The already existing DateTime and DateTimeImmutable classes already support microseconds. Adding an extra class for them, would just be confusing. 3. The 4th parameter to setTime() should not cause BC breakage. Added additional new arguments with a default value, is not a BC break. cheers, Derick -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] DateTime microseconds discussion
On 08/11/16 12:46, Derick Rethans wrote: >> Support for microseconds was added late in the 7.1 RC cycle, however is has >> > some issues: > Some *additional* support for microseconds was added in the PHP 7.1 > cycle, mostly to support bug fixes that have been around for a long > time. We had a similar problem when the resolution of time from Firebird was increased from milliseconds to microseconds. A lot of code that was expecting no sub second time started breaking because of the extra detail. The fix was 'CURRENT_TIME [(precision)]' but a compromise on defaults results in CURRENT_TIMESTAMP != CURRENT_DATE + CURRENT_TIME unless you actually set precision of both to match. On PHP it was actually less of a problem because the data returned has to be converted into a string and ibase.timestampformat filtered the fractional part anyway, and even the seconds can be trimmed if appropriate, so a 'default' string format would solve the problem of comparing time data from different sources? -- Lester Caine - G8HFL - Contact - http://lsces.co.uk/wiki/?page=contact L.S.Caine Electronic Services - http://lsces.co.uk EnquirySolve - http://enquirysolve.com/ Model Engineers Digital Workshop - http://medw.co.uk Rainbow Digital Media - http://rainbowdigitalmedia.co.uk -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php