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
