Re: [PHP-DEV] [RFC] [Discussion] Serializable closures from constant expressions

Hi Nicolas

On 10.06.26 19:02, Nicolas Grekas wrote:
> I'd like to open the discussion on a new RFC:
> https://wiki.php.net/rfc/serializable_closures

Thank you for your RFC. Sorry for being late to the party.

I'm trying to understand the premise of the RFC. Why does Symfony need 
to cache attributes at all? Attribute construction is cheap, at least 
when compared to restoring them using unserialize() or even Symfony's 
VarExporter. Is this an artifact from the doc block attributes era when 
parsing them was expensive, or is there a practical reason today?

I found the RFC quite hard to read. It uses a lot of ambiguous 
terminology, sentences are long and rarely explained with an example. 
It's also very technical, though maybe necessarily so.

 > When the class's source changes, a stored reference either stops 
resolving or is rejected by the hash check; both throw an Exception on 
unserialize(), which cache layers already treat as a miss.

I suppose Symfony handles unserialize() errors gracefully, but an 
unserialize() call today will not fail unless an obvious BC break was 
made, e.g. by removing a serialized class. This is much more predictable 
than shifting some elements around or tweaking a closure implementation. 
I'm not convinced all caching layers today handle errors gracefully.

 > Security model

IMO the security argument for unserialize() is redundant. unserialize() 
is documented to be unsafe when called with unsanitized inputs. For 
php-src, we close all security reports related to unserialize() as invalid.

https://www.php.net/manual/en/function.unserialize.php

 > Do not pass untrusted user input to unserialize() regardless of the 
options value of allowed_classes. Unserialization can result in code 
being loaded and executed due to object instantiation and autoloading, 
and a malicious user may be able to exploit this.

Hence, I think security is a good reason for making the serialized 
format more complex than it needs to be.

The format is very complex and deals with a ton of edge cases, e.g. 
handling so many attribute target types. It's mostly hidden from users, 
but not completely (referring to the reflection API).

The implementation is also very large and complex.

Overall, I'm sadly not in favor of this RFC.

Ilija