Thanks for having a look Le mer. 15 juil. 2026 à 00:20, Ilija Tovilo <[email protected]> a écrit :
> 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? > Attributes are one metadata source among several, in systems designed around loaders: Doctrine entity metadata, Symfony Serializer mappings, Validator constraints, to name a few widely used ones. Loaders read XML, YAML, phpdoc in its day, attributes now, and produce a common representation, merged and indexed along the way. That derived representation is what goes into cache layers or gets dumped as PHP (a cache layer too). So the comparison is not newInstance() vs unserialize() for one attribute: it is re-running the whole load+merge+index pipeline on every request vs one cache fetch. https://github.com/symfony/symfony/issues/63228 describes how closures in attributes disrupt this: the engine gives no way to put them in a cache (via serialize() or exported PHP), so affected classes fall back to runtime extraction. What you suggest is what is implemented today, and it is the measured slow path. This RFC closes the gap by letting the already-optimized representation be cached. > 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. > Concrete pointers to unclear spots would help, I'll happily rework them. The short description on the PR may be a better entry point: https://github.com/php/php-src/pull/22716. The RFC is long because it maps the problem space and the alternatives I explored; that context felt necessary to judge the choices. > > 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. > The failure set is narrower than that. References are scoped to their declaring element and carry a hash of the closure's code: shifting elements around, adding methods, reordering, moving the class in the file, none of that invalidates anything. A reference fails when its own closure's code changed, when closures were added/removed in its own element, or when the element or class was renamed. And when the closure's code changed, failing is the correct outcome: the cached metadata was derived from the old code, so the entry must be rebuilt, not partially reused. Also, today's behavior on the comparable event (a renamed class) is arguably worse than an exception: __PHP_Incomplete_Class propagates silently until first use. Caches cannot return data they fail to restore per PSR-6/16; treating a failed unserialize as a miss is how conforming implementations behave. > > 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. We did add allowed_classes, and we do defer __wakeup()/__unserialize() calls until the whole payload is reconstructed, both because some gadget surfaces were too risky despite that doc note. Security is not black and white; hardening is about bounding consequences when users err. In Symfony we also reject gadget chains as non-CVE, and we still fix them as hardening. See https://github.com/ambionics/phpggc/tree/master/gadgetchains for how effective chains are in practice. The magnitude here is specific. Today a chain needs app-specific gadget classes; that is why phpggc is a per-framework catalog. Name-based closure unserialization would ship a universal, app-independent gadget in the engine: name system, feed args from the same payload, done. The declared-set boundary reduces that to swapping one closure a class already declares for another. I won't commit to an RFC that turns every serialized payload into that kind of gadget, and I don't think we should, either. That said, I offered to split the RFC and drop the serialize part from the first one. The reflection API is the piece Symfony needs most: with it, symfony/cache can export the closures itself for any PSR-6/16 consumer. What only the engine can provide is provenance, "is this closure declared by an attribute, and which one". I tried to build it in userland and in an extension; the extension has to instrument ReflectionAttribute and mirror private engine structs, and the result is still an approximation. 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. > The target types are the ones the language gives attributes; covering fewer would leave arbitrary holes ("this closure serializes, that one doesn't, because it hangs on a constant"). On size: two flag lines in the compiler, everything else in one file, no new persisted state, no opcache changes. Overall, I'm sadly not in favor of this RFC. > I hope that's not the last word, because the problem is real and only the engine can address it; referencing closures found in attributes is generic, not a Symfony quirk. Would the reflection-only subset address your complexity concern? That would help me decide whether to reshape the proposal or its wording. Nicolas
