[dev] Horde_Imap_Client test failure with latest PHP

[Michael M Slusarz]

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Remi Collet <remi at fedoraproject.org>:
>
>> After lot of discussion with other PHP developers:
>>
>> - code in previous PHP is not consistent.
>>
>> Other class constructors from ext/date extension return NULL on failure.
>>
>> - this could raise segfault on some situation (bug #67118)
>>
>> return NULL is the correct solution, fix #67118, but breaks some code,
>> including Horde.
>>
>>
>> So temporary fix have been applied in 5.4.30, 5.5.14 and 5.6.0:
>>
>> http://git.php.net/?p=php-src.git;a=commit;h=1fe9f1e4f572d7b4d5a3872f41ea61e71fb563bf
>>
>> Test inspired from Horde_Imap_Client_DateTime added:
>>
>> http://git.php.net/?p=php-src.git;a=commit;h=15d8c80ead75be976c18a66b0933cf52f3e6579f
>>
>>
>> But the correct behavior have be restored in master.
>>
>> http://git.php.net/?p=php-src.git;a=commit;h=f11f7f56013c5ee4e6009997602e9b5a64064909
>>
>> http://git.php.net/?p=php-src.git;a=commit;h=f1ef7018f036de3b0d149ea0bc4575767b9bb413
>>
>>
>> So current horde code will continue to work for some time,
>> giving you time for a better solution.
>>
>> Summary : parent::__construct could be called only once.

Thanks for your help on this, but I vehemently disagree with this last  
conclusion and the fix in master.

You've now made DateTime act entirely opposite of all other PHP classes.

Take this code (tested on PHP 5.4.29):

-----
<?php
class A {
     public function __construct()
     {
         throw new Exception();
     }
}

class B extends A {
     public function __construct()
     {
         try {
             parent::__construct(true);
             return;
         } catch (Exception $e) {}

         if (is_null($this)) {
             print "THIS IS NULL\n";
         } else {
             print "THIS STILL EXISTS\n";
         }
     }
}

class C extends DateTime {
     public function __construct($time = null, $timezone = null)
     {
         try {
             parent::__construct('XXXX');
             return;
         } catch (Exception $e) {}

         if (is_null($this)) {
             print "THIS IS NULL\n";
         } else {
             print "THIS STILL EXISTS\n";
         }
     }
}

new B();
new C();
-----

According to your statement "parent::__construct could be called only  
once", then this should be the result:

-----
THIS IS NULL
THIS IS NULL
-----

Instead, this is the output:

-----
THIS STILL EXISTS
THIS IS NULL
-----

I see no reason why DateTime should behave any different than any  
other PHP class.  And the latter's behavior is much preferable in my  
opinion ... it allows a child class to handle these kind of issues  
wholly within its constructor in order to salvage the original call.   
With the change in master, you are now requiring 2 entirely separate  
constructor calls in this use-case - one external DateTime  
instantiation to "verify" the input, and then another call to the  
parent constructor to actually set the input in the current object.   
The previous code allowed this to happen once.

Nowhere in the documentation that I can find is there any indication  
that a "built-in" class should act any differently than a user-space  
class.

Extending DateTime should be no different than me extending a  
user-defined class where all I am given is the constructor method  
signature (i.e. I have no details on how the parent class actually  
implements the body of __construct()).

I'll add this comment to bug #67118, which is probably the better  
place to continue this discussion.

michael

___________________________________
Michael Slusarz [slusarz at horde.org]