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
