Re: [RFC][Discussion] Add #[NoSerialize] attribute for excluding properties or classes from serialization

Hi Nicolas, Thank you for the review, and sorry for the delay. > I checked code bases I know well, Symfony in particular, and I had a > hard time finding any implementation of __serialize()/__sleep() that > the property-level attribute would replace You are right, and the RFC was wrong to imply otherwise. There is no such __sleep() in Symfony. The class-level case is there, six times, written identically in Lock\Lock, Semaphore\Semaphore, Process\Pipes\UnixPipes, ErrorHandler\BufferingLogger, HttpClient\Chunk\ErrorChunk and Form\Util\OrderedHashMapIterator: public function __serialize(): array { throw new \BadMethodCallException('Cannot serialize '.__CLASS__); } For the property-level case, Magento 2 has 31 classes with __sleep(), chained so that each level strips its own injected services. Each class below extends the one above it: Framework\Model\AbstractModel: -8 properties Framework\Model\AbstractExtensibleModel: -2 Eav\Model\Entity\Attribute\AbstractAttribute: -12 Eav\Model\Entity\Attribute: -4 Catalog\Model\ResourceModel\Eav\Attribute: -4 Every level is array_diff(parent::__sleep(), [...]) with a mirrored __wakeup() re-injecting the same services, so each property is named twice. Same shape in their collection hierarchy, in Store and in User (13 properties). Smaller whitelist form: Laravel's RateLimited and Doctrine's PersistentCollection. > having objects that embed both value-state and a PDO connection looks like a design smell Agreed as a principle, and PDO was a bad headline example. But this is the normal shape of an ORM model in Magento and Laravel, and it will not be redesigned away. __serialize() is not a substitute: it requires describing the whole format by hand, which is what produces those array_diff() chains. The introduction has been rewritten around these examples. > contracts propagate naturally to child classes. Attributes don't, and > making this one an exception raises the question: why break attribute > rules It is not an exception. #[AllowDynamicProperties] already behaves this way, since 8.2: #[AllowDynamicProperties] class P {} class C extends P {} $c = new C; $c->dyn = 1; // no deprecation: flag is inherited $r = new ReflectionClass('C'); $r->getAttributes(AllowDynamicProperties::class); // array(0) {} zend_inheritance.c copies ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES from the parent, alongside ZEND_ACC_NOT_SERIALIZABLE. The interface alternative was argued at length in the 2024 thread and the list did not converge: Eberlei, Banyard and Dusk for the attribute, you and Larry for the interface. One cost that was not raised there: this RFC migrates 107 internal classes across 64 stub files. As an interface, all of them would gain a new entry in class_implements() — observable userland state, and a larger BC surface than an attribute. If the list prefers an interface, that is the list's call, but it should be made deliberately rather than as a side effect here. > we would need reflection-based checks on every property during both > serialization and unserialization I checked var-exporter. The cost is per-class on 7.x, and on 8.1 it is not reflection at all. Class-level is free: it sets the existing ZEND_ACC_NOT_SERIALIZABLE. On 7.x Registry::unserialize() calls native unserialize(), which already guards that flag, and getClassReflector() probes with serialize($proto) once per class. ext-deepclone tests the flag directly (deepclone.c:519, :5234). Property-level on 8.1: VarExporter::export() delegates to deepclone_to_array(), whose fast path already iterates ce->properties_info_table and tests prop_info->flags — ZEND_ACC_STATIC, ZEND_ACC_PUBLIC, ZEND_ACC_PROTECTED_SET, ZEND_ACC_PRIVATE_SET (deepclone.c:2453-2489). ZEND_ACC_NO_SERIALIZE is one more condition in a loop that already runs. The polyfill does use reflection, but per class on both sides: $scopeMaps for export (DeepClone.php:785-795), $propertyScopes for hydration (:453). For the engine the comparison runs the other way: a ce_flags bit test is cheaper than instanceof, which is why the engine uses a flag. Three changes to the RFC since your message: * The introduction cites the codebases above instead of asserting that the use case is common. * Internal classes are migrated explicitly, as @deprecated was migrated to #[\Deprecated] in 8.4, rather than having the attribute injected during code generation. @not-serializable stays supported for third-party extensions. This follows Tim's suggestion. * unserialize() is now symmetric: a #[NoSerialize] property present in the payload is parsed and discarded rather than restored. The attribute now means "not part of the serialized form" in both directions, matching transient in Java and [NonSerialized] in C#, and removing an injection surface. The old behaviour is recorded under Rejected Features. Best regards, Dmytro Kulyk