Dear all,
Le mer. 15 juil. 2026, 17:52, Nicolas Grekas <[email protected]> a écrit : > 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 > After sleeping on this for a few days, I worked on v0.3 of this RFC. Unlike was I suggested previously, I didn't reduce the proposal to Reflection only but to the other way: I removed reflection-related changes and kept only the serialize() part. This makes things simpler and solve the generic case in a better way: while reflection would have helped the implementation I already built, I'm mostly interested in providing a solution that doesn't require any dependency. The problem is generic, so should be the solution. serialize() is the one feature that most/all cache systems are built on, so that's the place that needs the improvement. I also reworded the body of the RFC to hopefully address Ilia's concerns about it, which others might share. I'm looking forward to new feedback on this new version. https://wiki.php.net/rfc/serializable_closures Cheers, Nicolas >
