Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:96776
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Message-ID: <42.65.23587.00622285@pb1.pair.com>
To: internals@lists.php.net
References: <46.92.05967.9AB91285@pb1.pair.com>
 <CA+kxMuR1wVzyAA3RkN2VxmgUNFDm2Z-h02HChFVwT+=imxG4dQ@mail.gmail.com>
 <DB.40.23587.332C1285@pb1.pair.com>
 <alpine.DEB.2.20.1611081321150.4870@whisky.home.derickrethans.nl>
Date: Tue, 8 Nov 2016 20:22:37 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <alpine.DEB.2.20.1611081321150.4870@whisky.home.derickrethans.nl>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] DateTime microseconds discussion
From: arjen@parse.nl (Arjen Schol)

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
> 	<?php
> 	class MyDate extends DateTimeImmutable
> 	{
> 		function setTime( $h, $i, $s )
> 		{
> 			return parent::setTime( $h, $i + 5, $s );
> 		}
> 	}
>
> 	$a = new MyDate();
> 	$b = $a->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.000000"
> 	  ["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 DateTime object without microseconds 
initialized/set to 0: DateTime::createFromFormat('U', time()); or new 
DateTime('@' . time()); or afterwards by calling setTime()...

This has been the case since 5.3.1.. You can't seriously argue DateTime 
always had microsecond precision and fixing the initialization took 
almost 7 years..

DateTime had some obscure way to initialize it with the current time 
including microseconds, the default __construct() did not show a sign of 
microsecond suppport. Changing this behaviour 7 years after introduction 
cannot be called a bugfix (is this going into 5.6 and 7.0 as well?), 
it's just a feature.

Did you ever Googled "microsecond site:php.net/manual"?
Only the documentation (comments not included) about createFromFormat 
mentions 'microsecond'.

Making the assumption DateTime supports microtime would only be based on 
internal, non-documented details. THAT would be a bad assumption.

See https://3v4l.org/aCRU0 and https://3v4l.org/4Oid8


>
> 	[PHP: 7.0.13-dev  ] derick@whisky:~/dev/php/php-src.git $ cat doo2.php
> 	<?php
> 	$date = new DateTimeImmutable( "13:39:04.123456" );
> 	var_dump( $date );
> 	?>
>
> 	[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
>