Looks like it worked when Log::singleton was assigned to a static 
before, and only broke when we removed the static and didn't add the &.

Changes have been made in CVS for this ticket:

http://cvs.horde.org/diff.php/framework/Form/Form.php?r1=1.431&r2=1.432&ty=u
http://cvs.horde.org/diff.php/framework/Group/Group.php?r1=1.112&r2=1.113&ty=u
http://cvs.horde.org/diff.php/framework/History/History.php?r1=1.60&r2=1.61&ty=u
http://cvs.horde.org/diff.php/framework/Horde/Horde.php?r1=1.679&r2=1.680&ty=u
http://cvs.horde.org/diff.php/framework/Kolab/Kolab/IMAP.php?r1=1.23&r2=1.24&ty=u
http://cvs.horde.org/diff.php/framework/Kolab/Kolab/LDAP.php?r1=1.8&r2=1.9&ty=u
http://cvs.horde.org/diff.php/framework/Notification/Notification.php?r1=1.67&r2=1.68&ty=u
http://cvs.horde.org/diff.php/framework/Perms/Perms.php?r1=1.102&r2=1.103&ty=u
http://cvs.horde.org/diff.php/framework/UI/UI/VarRenderer/html.php?r1=1.174&r2=1.175&ty=u
http://cvs.horde.org/diff.php/framework/VC/VC.php?r1=1.26&r2=1.27&ty=u

Yes, this fixes the factory/singleton code for PHP4. You can drop that 
part if you want as I switched to PHP5 and PHP4 is out of maintenance 
(->doomed) anyway.

I only added it in places where the factory and singleton code is 
already defined as "function &factory()" and "function &singleton()", 
so it should make no difference. I'm really happy they changed the 
default "copy" behavior in PHP5 :-)



Regarding the logger change: The $logger variable is defined as a 
normal "static" var, so the code looked wrong to me. Do you have a 
test case where it failed in the past? Normal logging f.e. of IMAP 
logins still works for me with the new code.

This last patch is a bit confusing, since some places you're adding 
references too. Also, please be very very careful with that change to 
Horde::getLogger - we've had problems before when the shutdown 
function of PEAR_Log doesn't run, and nothing gets logged.

Changes have been made in CVS for this ticket:

   
http://cvs.horde.org/diff.php/horde/po/translation.php?r1=1.124&r2=1.125&ty=u
   http://cvs.horde.org/diff.php/imp/lib/IMAP/ACL.php?r1=1.5&r2=1.6&ty=u
   http://cvs.horde.org/diff.php/imp/lib/Quota.php?r1=1.35&r2=1.36&ty=u
   http://cvs.horde.org/diff.php/kronolith/lib/FBView.php?r1=1.28&r2=1.29&ty=u
   http://cvs.horde.org/diff.php/kronolith/lib/Storage.php?r1=1.7&r2=1.8&ty=u
   http://cvs.horde.org/diff.php/mnemo/lib/Driver.php?r1=1.41&r2=1.42&ty=u
   http://cvs.horde.org/diff.php/nag/lib/Driver.php?r1=1.116&r2=1.117&ty=u
   http://cvs.horde.org/diff.php/turba/lib/Driver/ldap.php?r1=1.89&r2=1.90&ty=u
   
http://cvs.horde.org/diff.php/turba/lib/Driver/sql.php?r1=1.113&r2=1.114&ty=u
   
http://cvs.horde.org/diff.php/turba/lib/Maintenance/Task/upgradelists.php?r1=1.7&r2=1.8&ty=u
   
http://cvs.horde.org/diff.php/turba/lib/Maintenance/Task/upgradeprefs.php?r1=1.11&r2=1.12&ty=u

Changes have been made in CVS for this ticket:

   
http://cvs.horde.org/diff.php/framework/Browser/Browser.php?r1=1.238&r2=1.239&ty=u
   http://cvs.horde.org/diff.php/framework/Crypt/Crypt.php?r1=1.48&r2=1.49&ty=u
   http://cvs.horde.org/diff.php/framework/Data/Data.php?r1=1.99&r2=1.100&ty=u
   
http://cvs.horde.org/diff.php/framework/DataTree/DataTree.php?r1=1.187&r2=1.188&ty=u
   
http://cvs.horde.org/diff.php/framework/Editor/Editor.php?r1=1.24&r2=1.25&ty=u
   
http://cvs.horde.org/diff.php/framework/Form/Form/Action.php?r1=1.29&r2=1.30&ty=u
   
http://cvs.horde.org/diff.php/framework/Form/Form/Renderer.php?r1=1.220&r2=1.221&ty=u
   
http://cvs.horde.org/diff.php/framework/IMAP/IMAP/ACL.php?r1=1.17&r2=1.18&ty=u
   http://cvs.horde.org/diff.php/framework/Image/Image.php?r1=1.67&r2=1.68&ty=u
   http://cvs.horde.org/diff.php/framework/NLS/NLS.php?r1=1.121&r2=1.122&ty=u
   
http://cvs.horde.org/diff.php/framework/Net_IMSP/IMSP.php?r1=1.39&r2=1.40&ty=u
   
http://cvs.horde.org/diff.php/framework/Prefs/Identity.php?r1=1.12&r2=1.13&ty=u
   http://cvs.horde.org/diff.php/framework/Token/Token.php?r1=1.50&r2=1.51&ty=u
   http://cvs.horde.org/diff.php/framework/VFS/lib/VFS.php?r1=1.6&r2=1.7&ty=u

Most important patch: Fix singletons in framework.



Please check the changes in Group/Group.php and Kolab/IMAP.php.

Those were the most "complex" ones.



Enjoy :-)

Simplify patch for framework

Simplify patch for imp.

Simplify patch for kronolith.

Simplify patch for mnemo.

Simplify patch for nag.

Fix singleton in Turba and also intitialize three variables properly.

Yipeeee, my segfaults are gone :-)



I've splitted the modifications into patches that really fix things 
and patches that are simplifying the singleton code.



Let's start with the patch for "horde".

Fixed upstream ;-)

http://bugs.php.net/bug.php?id=33595 states it's fixed in PHP5_3. Hmm.

circular references patch will not be implemented in PHP 5.3 because 
it did not have many votes.. See: 
http://news.php.net/php.internals/32330. You should clean up 
references by your own with a destroy function.  Adding this code into 
the "magic" __destruct() method is impossible because is called when 
an object can be destroyed – as is still referenced it means never.

Yeah I know, sorry, I wrote that comment before I learned more 
information as I just checked Horde_History, Group and Perms. It was 
not easy to find the actual problem as it's close to #5355, but not 
100% related.

Your upstream bug contains an incorrect statement:

The supplied code is the standard way Horde does it singletons. We 
always used the syntax of "$object = &new class" to make it work  with 
PHP4 and PHP5.



As Chuck mentioned, the code you provided in that bug report is most 
definitely *not* the "standard" syntax we use.  If that syntax does 
appear anywhere, it is an error and needs to be fixed (thus this bug 
report).  Regardless of whether PHP is leaking memory or not, this 
needs to be cleaned up regardless if just because semantically it is 
incorrect.

Everything seems to run smooth until one of those classes created by 
our "wrong" singleton contains an object like PEAR_Error. My 
Horde_History object does this as I don't use a SQL backend. 
PEAR_Error contains cyclic references internally.



PHP 5.2.x currently doesn't handle cleanup of cyclic references. PHP 
5.3.x should be fixed, but it doesn't work properly. I filed an 
upstream bug: http://bugs.php.net/?id=45178



My feeling tells me that something with all those (cyclic) references 
goes wrong during cleanup/reference counting/whatever. Let's see what 
happens after I have fixed the singletons.

Actually, I don't think this has anything to do with what you see, 
because the PHP manual  explicitly refers to Zend Engine 1.

Beside that, what Chuck said.

Taking the liberty of assigning to Thomas - thanks for looking in to this!

That is correct - we should never assign a reference to a static 
variable. Anywhere this crept in is an error.

Hello together,



I'm currently debugging a heap memory corruption in PHP (5.2.6) using Horde

and noticed some odd behavior regarding references and static variables.

Currently we implement some of our singletons like this:



-----------------------------

function &singleton()

{

     static $history;



     if (!isset($history)) {

         $history = &new Horde_History();

     }



     return $history;

}

-----------------------------



I've found out that the singleton is not working and it's related to this:

http://www.php.net/manual/en/language.variables.scope.php

The part with "References with global and static variables" is important.



-> The code has to be changed to "$history = new Horde_History();"

Singletons storing their references via an array are unaffected.



If there a no objections I'll start fixing the static variables.



I really hope this will solve my trouble (=segfaults) deep inside

the PHP memory manager during request shutdown. Spending two days

in gdb, debugging internal PHP memory structures is no fun at all :o)



Cheers,

Thomas