Hi Dmytro
пт, 7 лист. 2025 р. о 12:57 Tim Düsterhus <[email protected]> пише: > > > > Thank you. The updated RFC is looking good to me. I also appreciate that > > you kept a changelog within the RFC. > > > > One thing you could mention and that should be done as part of the RFC > > is migrating all existing classes with `@not-serializable` to make use > > of the attribute instead. That's also what has been done when the > > `#[\Deprecated]` attribute was introduced. Other than that, I don't have > > any further comments. > > > > Thank you for the review and the helpful suggestion. > > I agree that all internal classes currently annotated with > @not-serializable should also receive the corresponding > #[\NoSerialize] attribute as part of this RFC. This ensures that > internal metadata, reflection, and userland tooling remain fully > aligned, and that the behavior is consistently exposed at both the > engine and stub levels. > > The RFC has been updated to explicitly state that these attributes > will be added for all such internal classes. > > I also have one technical question regarding stub generation: > When attributes are emitted into generated arginfo.h files, should the > generator automatically include zend_attributes.h? > This would avoid requiring extensions to include zend_attributes.h > explicitly when they do not use attributes themselves but only include > generated stubs that reference them. Before making any implementation > adjustments, I’d appreciate confirmation that this approach is > acceptable within the current stub-generation design. > > ``` > /* This is a generated file, edit the .stub.php file instead. > * Stub hash: 2fc12d1fde65efbec4305f4934a3f4b25282a552 */ > > #include "zend_attributes.h" > ... > zend_add_class_attribute(class_entry, > ZSTR_KNOWN(ZEND_STR_NO_SERIALIZE), 0); > ... > ``` Thanks for the RFC. I think it’s an easy sell, but not necessarily for the right reasons. Let me explain. First, on property-level usage: The RFC states that the attribute covers a common use case: transient or resource-based properties in frameworks and libraries. On which fact is this based? 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 (ignoring BC). Symfony usually exhibits most common and uncommon patterns, so if this were widespread, I would expect to see some. More generally, having objects that embed both value-state and a PDO connection looks like a design smell: such objects behave differently before and after serialization, which is inherently brittle. Such cases exist in the wild, of course, but making them a first-class, favored use case in the language seems questionable. Second, on class-level not-serializability: This is a very common need. We had a thread a year ago discussing a #[NotSerializable] attribute with identical semantics ( https://externals.io/message/121969). My position was and still is: attaching this semantics to classes fits an interface far better than an attribute. Even if it’s a negative contract, it’s still a contract, and contracts propagate naturally to child classes. Attributes don’t, and making this one an exception raises the question: why break attribute rules when PHP already has the right tool - interfaces - to express this? The engine currently uses a flag internally, but it could just as well use an interface, removing a special case and aligning internal and userland behavior. This follows a long and good tradition of replacing ad-hoc engine features with userland-visible constructs. Finally, a practical concern: I wrote and still maintain the symfony/var-exporter <https://packagist.org/packages/symfony/var-exporter> component, which implements PHP’s serialization semantics in userland but outputs executable PHP code. It is used in performance-sensitive cache systems. With the proposed attribute, we would need reflection-based checks on every property during both serialization and unserialization. Even with caching, this would noticeably hurt performance. This is another argument in favor of an interface BTW: instanceof/is_subclass_of() checks are much cheaper than reading attributes for userland (not even counting the proposed inheritance rule). Thanks for considering my feedback. Best regards, Nicolas
