Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 1.11.5

Bug #3645 Bad implementation of clone()
Submitted: 2005-02-28 18:59 UTC
From: ojai at nerim dot net Assigned: alan_k
Status: Closed Package: DB_DataObject
PHP Version: 4.3.10 OS: Linux Debian
Roadmaps: (Not assigned)    
Subscription  
Comments Add Comment Add patch


Anyone can comment on a bug. Have a simpler test case? Does it work for you on a different platform? Let us know! Just going to say 'Me too!'? Don't clutter the database with that please !
Your email address:
MUST BE VALID
Solve the problem : 25 - 16 = ?

 
 [2005-02-28 18:59 UTC] ojai at nerim dot net
Description: ------------ Hi, You implement a wrong clone() function in DB_Dataobject on line 190 (current cvs version). I do need a such compatibility trick, but your version of clone() does not call __clone() on the objects it duplicates. I think you should use PHP_Compat's one. Cheers

Comments

 [2005-03-01 02:46 UTC] alan_k
I'm not sure it's a good idea to call __clone() by default, as this is not the php4 default behavior, and there may be a speed penalty. (function calls are expensive) eg: while(...) $ar[] = clone($do); // goes from 1 func call per line to ~3 } This bug is also fixable by including the compat version before including dataobjects anyway. (as clone is only created by dataobjects if it does not exist) So It may just be worth leaving as is..
 [2005-03-01 09:55 UTC] ojai at nerim dot net
That's what I'm currrently doing : including PHP_Compat's version before DB_DataObject gets loaded. But your use of __clone() is very obscure to me. The point is that if you're concerned with a possible performance hit, then you should add an option to the Generator, to suppress this __clone() method, from the generated class. This way PHP_Compat's clone() method won't call anything, since if method_exists ($object, '__clone') will evaluate to false. Second point : I don't see why you return $this from DO::__clone(). I don't even see why this __clone() method is needed. The PHP manual does not mention anything about the return value. PHP5 clones the object by itself, and then it calls the __clone() method if it exists, so that internal references get replicated if the developer wishes so. Last, but not least : I understand your point about "clone is only created by dataobjects if it does not exist". But, I have a particular anger against this sort of behaviour : when some classes declare global identifiers, or change global settings, so that you suddenly observe a mysterious bug at the other end of your application, driving all your investigations to /dev/null. For small apps that's ok, but when your object tree looks like an old oak that's different... Anyway, it gets a bit more complicated with the #3649 bug that I just posted : some internal references seem pretty hard to replicate under PHP4. I'd greatly appreciate if you could give your opinion on this bug. Thanks
 [2005-03-01 10:49 UTC] alan_k
The comments in the generator sum this up. the generation of __clone() was put in as a Forward Compatibility method, before PHP5 decided to use clone() as the method to implement cloning. the old documentation said to do : while ($obj->fetch()) { $ar = $obj->__clone(); } which will now cause an error in PHP5. however there is still legacy code that is using this, and removing it from the generator would break old code. For PHP5, this method is not generated. I probably need to ponder the best approach for this... obviously PHP_Compat is not really compatible with dataobjects at present...
 [2005-03-02 03:56 UTC] alan_k
This bug has been fixed in CVS. In case this was a documentation problem, the fix will show up at the end of next Sunday (CET) on pear.php.net. In case this was a pear.php.net website problem, the change will show up on the website in short time. Thank you for the report, and for helping us make PEAR better. well, at least partially...