Engine calls to private destructors cause errors

[jnvsor]

Description

https://3v4l.org/bKijA

While private magic method declarations result in a visibility warning, they still work when invoked by the engine as in the cases of __debugInfo (and __sleep since 8.1)

Private destructors however cause errors whenever an instance goes out of scope. The engine should always be able to invoke a destructor when destroying an instance.

PHP Version

8.4

Operating System

No response

[iluuu1994]

But what's the right behavior? The other warnings indicate that the magic methods aren't really intended to be non-public. For __construct, where it is allowed, it has special meaning, i.e. it prevents the object from being constructed from a public scope. If the intention were just to prevent users from calling __destruct, that's not possible with __construct either. So, IMO we should just introduce a warning when declaring __destruct as non-public and eventually promote this to an error for all affected magic methods.

Let's wait for others to chime in.

[jnvsor]

If the intention were just to prevent users from calling __destruct, that's not possible with __construct either

That's what I thought too until __adrian on libera.chat#php pointed out you can actually call all of the magic methods manually:

https://3v4l.org/nSO3i

I'd agree that users probably shouldn't be able to call magic methods directly but at this point that would introduce a lot of BC breaks

[iluuu1994]

That's what I referred to here:

If the intention were just to prevent users from calling __destruct, that's not possible with __construct either.

I.e. it is not possible to prevent the user from calling __construct without also preventing them from constructing an object using new. I think this is quite a similar situation.

Edit: Sorry for quoting the same text. My brain auto-skipped the quote. 😄

[jnvsor]

That's what I referred to here

Ah, I misread you sorry.

we should just introduce a warning when declaring __destruct as non-public

On the contrary. In the case of __destruct it probably shouldn't be public at all. Destructors are supposed to be called when the object is destroyed and letting users call it before then can leave you with broken state like closed file pointers, closed database connections, killed sub-processes etc.

A quick symbol search shows me a lot of libraries that have had to add error checking (or silencing) in destructors that they wouldn't have had to if they could declare it private or protected without causing errors.

In the few cases where you actually want to reuse the destructor functionality from outside you can always just make a public method and call it from inside __destruct

The reason __destruct, __construct and __clone are different from the other magic methods is because they are effectively public stubs on all objects by default. You can use new and clone even on classes with no methods defined. (Which makes private __clone on a child class a liskov violation which is a whole other thing: https://3v4l.org/6evWR)

Contrast that with stuff like __get which will give you "Undefined property" errors if you try to use it on a class where it isn't defined.

---

Anyway, I didn't plan on making this a big discussion about magic method visibility, I just thought it was obvious that the engine should be able to call a private __destruct when destroying an object, especially since the docs confirm private destructors are supposed to be a thing:

All magic methods, with the exception of __construct(), __destruct(), and __clone(), must be declared as public

[iluuu1994]

On the contrary. In the case of __destruct it probably shouldn't be public at all. Destructors are supposed to be called when the object is destroyed and letting users call it before then can leave you with broken state like closed file pointers, closed database connections, killed sub-processes etc.

Right. My point was that __construct is essentially the exact same. It's possible to do $foo = new Foo(); $foo->__construct();, which will lead to a double initialization which is something a lot of code doesn't handle correctly, and something internal classes have to handle specially. However, users can't simply make constructors private, because it has a different semantic meaning. What I'm saying is it probably doesn't make sense to diverge from __construct here, when it essentially leads to the exact same issue.

In any case, since this is a behavioral change, it would make sense to discuss this on the mailing list. If that's something you'd like to kick off, please send a mail to the internals mailing list.

[jnvsor]

I don't think it's necessary to change the way __construct and __destruct behave to fix this specific issue though.

If you just let the engine call destructors regardless of visibility, that brings behavior in line with documentation (And the clear intent since there's no warning on private destructor declaration) and doesn't break BC. (Unless someone relied on private destructors crashing the application in lieu of exit)

But OK, I'll send a mail and ask for feedback there

[DeveloperRob]

It seems there is a possibility to need the current behaviour; although likely not particularly used as such:

• If __destruct is not public, even if the exception is caught (so execution continues) it isn't called (which is likely sensible): https://3v4l.org/kEc2Z
• You can use a static method to destroy the class 'in scope' by taking a reference and setting the variable to null which calls the destructor: https://3v4l.org/BWc6J

[jnvsor]

even if the exception is caught (so execution continues) it isn't called

Well it doesn't magically come back in scope somehow so it's just leaking whatever the destructor was there to clean up. Since the destructor is always executed by the engine in global scope (apparently) there's no situation where it wouldn't throw -- you'd be just as well off not having a destructor at all and going straight to the catch code

You can use a static method to destroy the class 'in scope' by taking a reference and setting the variable to null

That only works if that variable's the last reference to the object. Remember that objects are always by reference: https://3v4l.org/K5ErQ

xdebug_debug_zval helps understand this:

php > class Y{
php {     public static function destroyY(Y &$y){
php {         xdebug_debug_zval('y');
php {         $y = null;
php {     }
php {     
php {     private function __destruct() {
php {         echo "Y Destruct called\n";
php {     }
php { }
php > 
php > $y = new Y();
php > xdebug_debug_zval('y');
y: (refcount=1, is_ref=0)=class Y {  }
php > $y2 = $y;
php > xdebug_debug_zval('y');
y: (refcount=2, is_ref=0)=class Y {  }
php > xdebug_debug_zval('y2');
y2: (refcount=2, is_ref=0)=class Y {  }
php > Y::destroyY($y);
y: (refcount=2, is_ref=1)=class Y {  }
php > xdebug_debug_zval('y');
y: (refcount=1, is_ref=1)=NULL
php > xdebug_debug_zval('y2');
y2: (refcount=1, is_ref=0)=class Y {  }

Basically PHP has COW objects as well as COW strings