Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC][Discussion] NotSerializable attribute »

[RFC][Discussion] NotSerializable attribute

php.internals

Max Semenik

2 years ago
Hi, I'd like to propose a new attribute, #[NotSerializable]. This functionality is already available for internal classes - userspace should benefit from it, too. The RFC: https://wiki.php.net/rfc/not_serializable Proposed implementation: https://github.com/php/php-src/pull/12788 Please let me know what you think.
-- Best regards, Max Semenik

Unnamed Person

2 years ago
Max, I love this idea. Would it make sense to flip the design though and add `#[Serializable]` with a new `php.ini` setting? I’m thinking that almost every class I use, if not every single class, should not be serialized. If PHP had a flag to say “prevent all classes from serializing” then I could toggle that flag and avoid changing every single one of the potentially thousands of classes in my codebase, and I wouldn’t have to worry about ensuring that everyone creating a new class remembers to add this attribute. For existing sites with serializable classes we can leave the default of this new flag to the permissive legacy behavior: every class is serializable. However, if you are willing and have vetted your code you can flip that flag and now _unless_ you explicitly declare serializability support, your classes don’t serialize or unserialize. That could point to a future where _no_ class serializes _unless_ it implements `__sleep()` and `__wakeup()` or `Serializable`, which seems quite reasonable in my mind as a tradeoff between explicitness and surprise. Warmly, Dennis Snell

Robert Landers

2 years ago
On Sat, Dec 9, 2023 at 4:32 PM Dennis Snell via internals <internals@lists.php.net> wrote:
> > Max, I love this idea. > > Would it make sense to flip the design though and add `#[Serializable]` with a new `php.ini` setting? > > I’m thinking that almost every class I use, if not every single class, should not be serialized. > If PHP had a flag to say “prevent all classes from serializing” then I could toggle that flag and > avoid changing every single one of the potentially thousands of classes in my codebase, and > I wouldn’t have to worry about ensuring that everyone creating a new class remembers to add > this attribute. > > For existing sites with serializable classes we can leave the default of this new flag to the > permissive legacy behavior: every class is serializable. However, if you are willing and have > vetted your code you can flip that flag and now _unless_ you explicitly declare serializability > support, your classes don’t serialize or unserialize. > > That could point to a future where _no_ class serializes _unless_ it implements `__sleep()` > and `__wakeup()` or `Serializable`, which seems quite reasonable in my mind as a tradeoff > between explicitness and surprise. > > Warmly, > Dennis Snell > > > On Dec 9, 2023, at 1:30 PM, Max Semenik <maxsem.wiki@gmail.com> wrote: > > > > Hi, I'd like to propose a new attribute, #[NotSerializable]. This > > functionality is already available for internal classes - userspace should > > benefit from it, too. > > > > The RFC: https://wiki.php.net/rfc/not_serializable > > Proposed implementation: https://github.com/php/php-src/pull/12788 > > > > Please let me know what you think. > > > > -- > > Best regards, > > Max Semenik > > > > --0000000000006c4879060c12de83-- > > > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: https://www.php.net/unsub.php >
Hi Dennis! The biggest downside for the reverse of #[NotSerializable] is that it would break almost everything that uses serialize (e.g., WordPress caching, some Symfony/Doctrine things, SQS transports, etc.). Also, there is the question of whether this would apply to json_encode() or just serialize(). PS. Please don't forget to bottom post on this list. Robert Landers Software Engineer Utrecht NL

Unnamed Person

2 years ago
> On Dec 9, 2023, at 4:55 PM, Robert Landers <landers.robert@gmail.com> wrote: > > On Sat, Dec 9, 2023 at 4:32 PM Dennis Snell via internals > <internals@lists.php.net> wrote: >> >> Max, I love this idea. >> >> Would it make sense to flip the design though and add `#[Serializable]` with a new `php.ini` setting? >> >> I’m thinking that almost every class I use, if not every single class, should not be serialized. >> If PHP had a flag to say “prevent all classes from serializing” then I could toggle that flag and >> avoid changing every single one of the potentially thousands of classes in my codebase, and >> I wouldn’t have to worry about ensuring that everyone creating a new class remembers to add >> this attribute. >> >> For existing sites with serializable classes we can leave the default of this new flag to the >> permissive legacy behavior: every class is serializable. However, if you are willing and have >> vetted your code you can flip that flag and now _unless_ you explicitly declare serializability >> support, your classes don’t serialize or unserialize. >> >> That could point to a future where _no_ class serializes _unless_ it implements `__sleep()` >> and `__wakeup()` or `Serializable`, which seems quite reasonable in my mind as a tradeoff >> between explicitness and surprise. >> >> Warmly, >> Dennis Snell >> >>> On Dec 9, 2023, at 1:30 PM, Max Semenik <maxsem.wiki@gmail.com> wrote: >>> >>> Hi, I'd like to propose a new attribute, #[NotSerializable]. This >>> functionality is already available for internal classes - userspace should >>> benefit from it, too. >>> >>> The RFC: https://wiki.php.net/rfc/not_serializable >>> Proposed implementation: https://github.com/php/php-src/pull/12788 >>> >>> Please let me know what you think. >>> >>> -- >>> Best regards, >>> Max Semenik >>> >>> --0000000000006c4879060c12de83-- >>> >> >> -- >> PHP Internals - PHP Runtime Development Mailing List >> To unsubscribe, visit: https://www.php.net/unsub.php >> > > Hi Dennis! > > The biggest downside for the reverse of #[NotSerializable] is that it > would break almost everything that uses serialize (e.g., WordPress > caching, some Symfony/Doctrine things, SQS transports, etc.). Also, > there is the question of whether this would apply to json_encode() or > just serialize().
Thanks Robert, My idea was that the flag, by default, would preserve the existing behavior. A large codebase could decide to wait to disable serialization until every class is appropriately marked. Either way I guess a large refactor is resolved. With `#[Serializable]` the onus is on making sure that features requiring serialization are marked as such. With `#[NotSerializable]` the onus is on classes (which are not concerned with serialization) to opt-out of something unrelated. The RFC calls this out and it is awkward to implement `__wakeup()` with a throwing exception. `#[Serializable]` takes that a step further in linking the two concepts only where they are related.
> > PS. Please don't forget to bottom post on this list.
Thank you!

Niels Dossche

2 years ago
Hi Max On 12/9/23 13:30, Max Semenik wrote:
> Hi, I'd like to propose a new attribute, #[NotSerializable]. This > functionality is already available for internal classes - userspace should > benefit from it, too. > > The RFC: https://wiki.php.net/rfc/not_serializable > Proposed implementation: https://github.com/php/php-src/pull/12788 > > Please let me know what you think. >
Thanks for this proposal, it is a sensible addition in my opinion. I do have a question/remark though. The example you wrote in your RFC (with MyClass implementing __sleep and __awake) is not equivalent to adding #[NotSerializable]. This is because if you create a child class of MyClass: ``` class MyClassChild extends MyClass { public function __sleep(): array { return ...; // real implementation instead of throwing } public function __wakeup(): void { ... // real implementation instead of throwing } } ``` Then this subclass MyClassChild will actually be serializable. If you instead put #[NotSerializable] on the parent class MyClass, then the child class won't be serializable even if you implement the serialization methods in the child. Is this intentional? If yes, this should probably be clarified in the text. Kind regards Niels

Larry Garfield

2 years ago
On Sat, Dec 9, 2023, at 10:17 AM, Niels Dossche wrote:
> Hi Max > > On 12/9/23 13:30, Max Semenik wrote: >> Hi, I'd like to propose a new attribute, #[NotSerializable]. This >> functionality is already available for internal classes - userspace should >> benefit from it, too. >> >> The RFC: https://wiki.php.net/rfc/not_serializable >> Proposed implementation: https://github.com/php/php-src/pull/12788 >> >> Please let me know what you think. >> > > Thanks for this proposal, it is a sensible addition in my opinion. > > I do have a question/remark though. > The example you wrote in your RFC (with MyClass implementing __sleep > and __awake) is not equivalent to adding #[NotSerializable]. > This is because if you create a child class of MyClass: > > ``` > class MyClassChild extends MyClass > { > public function __sleep(): array > { > return ...; // real implementation instead of throwing > } > > public function __wakeup(): void > { > ... // real implementation instead of throwing > } > } > ``` > > Then this subclass MyClassChild will actually be serializable. > If you instead put #[NotSerializable] on the parent class MyClass, then > the child class won't be serializable even if you implement the > serialization methods in the child. > Is this intentional? If yes, this should probably be clarified in the > text.
Attributes do not inherit automatically, so it's the opposite. A child that is not marked #[NotSerializable] would be serializable. (Whether that's good or not is a separate question.) Behavior-changing ini settings are generally viewed as a terrible idea and a mistake of the past, so let's not consider that option. Given the existing code, that means a #[NotSerializable] attribute would be the only option. (Or I suppose a #[Serializable(false)], which a default of true?) My main concern is that, as noted, *most* classes probably aren't realistically serializable. Basically any service class should never be serialized, ever, and anything that depends on a service class should never be serialized, ever. So for this attribute to have the desired effect, it would need to be added to most classes in a codebase. Which seems... like a lot of work. In an ideal world serializability would be opt-in, but we do not live in that world. How significant is the problem today? The RFC doesn't make an especially compelling argument for this functionality given the current state of things, just that it's really easy to do (which is a positive, certainly) and "non-serializable objects exist". I'd like to see a stronger argument for how this will make code bases better/more resilient/harder to break/whatever. It's also unclear if this would apply only to serialize/unserialize, or also to json_encode/json_decode. If a class has this attribute, is implementing __sleep/__wakeup/__serialize/__unserialize an error? Should it be? Also, is the expectation that user-space serializers (Symfony/Serializer, JMSSerializer, Crell/Serde, etc.) would also honor this attribute? There's nothing forcing them to, naturally, but is the intent that they should respect it? I'm not opposed to this proposal, but there's lots of questions still to answer before I would be in favor. --Larry Garfield

Niels Dossche

2 years ago
On 12/9/23 17:28, Larry Garfield wrote:
> On Sat, Dec 9, 2023, at 10:17 AM, Niels Dossche wrote: >> Hi Max >> >> On 12/9/23 13:30, Max Semenik wrote: >>> Hi, I'd like to propose a new attribute, #[NotSerializable]. This >>> functionality is already available for internal classes - userspace should >>> benefit from it, too. >>> >>> The RFC: https://wiki.php.net/rfc/not_serializable >>> Proposed implementation: https://github.com/php/php-src/pull/12788 >>> >>> Please let me know what you think. >>> >> >> Thanks for this proposal, it is a sensible addition in my opinion. >> >> I do have a question/remark though. >> The example you wrote in your RFC (with MyClass implementing __sleep >> and __awake) is not equivalent to adding #[NotSerializable]. >> This is because if you create a child class of MyClass: >> >> ``` >> class MyClassChild extends MyClass >> { >> public function __sleep(): array >> { >> return ...; // real implementation instead of throwing >> } >> >> public function __wakeup(): void >> { >> ... // real implementation instead of throwing >> } >> } >> ``` >> >> Then this subclass MyClassChild will actually be serializable. >> If you instead put #[NotSerializable] on the parent class MyClass, then >> the child class won't be serializable even if you implement the >> serialization methods in the child. >> Is this intentional? If yes, this should probably be clarified in the >> text. > > Attributes do not inherit automatically, so it's the opposite. A child that is not marked #[NotSerializable] would be serializable. (Whether that's good or not is a separate question.) >
I know attributes aren't inherited, but the ZEND_ACC flag that this uses _is_ inherited. Try this out yourself: ``` <?php #[NotSerializable] class MyClass {} class Foo extends MyClass {} $foo = new Foo(); $serialized = serialize($foo); ``` Results in Fatal error: Uncaught Exception: Serialization of 'Foo' is not allowed

Rowan Collins

2 years ago
On 9 December 2023 12:30:29 GMT, Max Semenik <maxsem.wiki@gmail.com> wrote:
>Hi, I'd like to propose a new attribute, #[NotSerializable]. This >functionality is already available for internal classes - userspace should >benefit from it, too.
If this ends up approximately the same as implementing serialisation as an exception, it feels quite a thin feature. If you put __sleep and __wakeup as shown into a trait, it's already as short and explicit as "use NotSerializable;" What would make it more compelling is if the engine itself could do more with the attribute. For instance, a direct isSerializable() on ReflectionClass that covered both internal and attribute-marked classes. It would also be useful to have some interface for classes that are *sometimes* serializable, because they contain open-ended collections of other objects. An example being exceptions, which may collect objects as part of the backtrace information. Such a class could iterate its contained objects, checking if they are unserializable classes, or classes which should recursively be asked if the instance is serializable. Regards,
-- Rowan Tommins [IMSoP]

Girgias

2 years ago
The implementation is simple and straight to the point, and uses machinery that has been added to the engine to deal more consistently with internal classes. In an ideal world (IMHO) serialization would be opt-in and __serialize() would be used to enable and describe the serialization format. However, this is not the case, and IMHO implementing __serialize() should only be done to change the *default* serialization format *not* to disable it. Exposing this to userland makes sense, as this would prevented the mess that was the Serializable interface that people used to disable serialization which was less then ideal. Moreover, having this as an attribute means, that even without adding a new method to ReflectionClass, you could determine via Reflection if this class is serializable or not. Because currently, the only way to know if it is *actually* serializable is to call serialize() on the object and see what happens. On Sat, 9 Dec 2023 at 17:16, Rowan Tommins <rowan.collins@gmail.com> wrote:
> On 9 December 2023 12:30:29 GMT, Max Semenik <maxsem.wiki@gmail.com> > wrote: > >Hi, I'd like to propose a new attribute, #[NotSerializable]. This > >functionality is already available for internal classes - userspace should > >benefit from it, too. > > If this ends up approximately the same as implementing serialisation as an > exception, it feels quite a thin feature. If you put __sleep and __wakeup > as shown into a trait, it's already as short and explicit as "use > NotSerializable;" >
If the alternative solution is a trait, then it is, IMHO, a bad alternative solution. What would make it more compelling is if the engine itself could do more
> with the attribute. For instance, a direct isSerializable() on > ReflectionClass that covered both internal and attribute-marked classes. >
I do agree, adding a isSerializable() function to ReflectionClass is a good idea. Best regards, Gina P. Banyard

Max Semenik

2 years ago
On Sat, Dec 9, 2023 at 7:18 PM Niels Dossche <dossche.niels@gmail.com> wrote:
> If you instead put #[NotSerializable] on the parent class MyClass, then > the child class won't be serializable even if you implement the > serialization methods in the child. > Is this intentional? If yes, this should probably be clarified in the text.
Elaborated about the inheritance in the RFC and added a test around it in the PR. On Sat, Dec 9, 2023 at 7:29 PM Larry Garfield <larry@garfieldtech.com> wrote:
> My main concern is that, as noted, *most* classes probably aren't > realistically serializable. Basically any service class should never be > serialized, ever, and anything that depends on a service class should never > be serialized, ever. So for this attribute to have the desired effect, it > would need to be added to most classes in a codebase. Which seems... like > a lot of work. In an ideal world serializability would be opt-in, but we > do not live in that world. >
Indeed, adding it to every service would be an annoying overkill, but I wanted to use it on classes that should *really* not be (de)serialized. On Sun, Dec 10, 2023 at 2:46 AM G. P. B. <george.banyard@gmail.com> wrote:
> Moreover, having this as an attribute means, that even without adding a new >
method to ReflectionClass, you could determine via Reflection if this class
> is serializable or not. > Because currently, the only way to know if it is *actually* serializable is > to call serialize() on the object and see what happens. >
I've amended the RFC with proposed ReflectionClass additions.
-- Best regards, Max Semenik

Nicolas Grekas

2 years ago
Hi Max, Hi, I'd like to propose a new attribute, #[NotSerializable]. This
> functionality is already available for internal classes - userspace should > benefit from it, too. > > The RFC: https://wiki.php.net/rfc/not_serializable > Proposed implementation: https://github.com/php/php-src/pull/12788 > > Please let me know what you think. >
Regarding the inheritance-related behavior ("The non-serializable flag is inherited by descendants"), this is very unlike any other attributes, and this actively prevents writing a child class that'd make a parent serializable if it wants to. To me, if this is really the behavior we want, then the attribute should be replaced by a maker interface. Then, a simple "instanceof NotSerializable" would be enough instead of adding yet another method to ReflectionClass. Cheers, Nicolas

Dusk

2 years ago
On Jan 2, 2024, at 23:11, Nicolas Grekas <nicolas.grekas+php@gmail.com> wrote:
> Regarding the inheritance-related behavior ("The non-serializable flag is > inherited by descendants"), this is very unlike any other attributes, and > this actively prevents writing a child class that'd make a parent > serializable if it wants to.
That seems appropriate. Consider a class which has set itself as #[NotSerializable] because it contains something which mustn't be serialized (like a secret or an identifier returned by an FFI library), or because the lifecycle of objects of that class needs to be tightly controlled (like a singleton or a scope guard). It shouldn't be possible to make any of these serializable by creating a subclass.

Michał Brzuchalski

2 years ago
Hi Nicolas, śr., 3 sty 2024 o 08:12 Nicolas Grekas <nicolas.grekas+php@gmail.com> napisał(a):
> Hi Max, > > Hi, I'd like to propose a new attribute, #[NotSerializable]. This > > functionality is already available for internal classes - userspace > should > > benefit from it, too. > > > > The RFC: https://wiki.php.net/rfc/not_serializable > > Proposed implementation: https://github.com/php/php-src/pull/12788 > > > > Please let me know what you think. > > > > Regarding the inheritance-related behavior ("The non-serializable flag is > inherited by descendants"), this is very unlike any other attributes, and > this actively prevents writing a child class that'd make a parent > serializable if it wants to. > > To me, if this is really the behavior we want, then the attribute should be > replaced by a maker interface. > Then, a simple "instanceof NotSerializable" would be enough instead of > adding yet another method to ReflectionClass. >
This should be possible without ReflectionClass, see https://3v4l.org/N3fmO A property redeclare doesn't inherit the attributes, therefore this might be a solution for the problem you presented. Another ad-hoc way could be implementing `__serialize()` in the child class. I like this feature and the only thing I'd like to consider is different naming options for voting. From the serialization libraries you can find similar attributes, these are: * https://github.com/Crell/Serde `#[Crell\Serde\Attributes\Field(exclude: true)]` * https://github.com/symfony/serializer `#[Symfony\Component\Serializer\Attribute\Ignore]` * https://github.com/schmittjoh/serializer `#[JMS\Serializer\Annotation\Exclude]` In the Java world also: * Java EE or Jakarta EE and using JAX-RS `@Transient` * Jackson `@JsonIgnore` In the C# world * .NET _System.Text.Json.Serialization_ `[JsonIgnore]` So there might be some other naming proposals like maybe just `Ignore` or `Exclude` ? What is worth mentioning are solutions allowing to change the strategy on inheritance which JMS/Serializer supports https://jmsyst.com/libs/serializer/master/reference/annotations#exclusionpolicy The `#[ExclusionPolicy]` allows to declare that all properties should be excluded or none. Worth mentioning a future scope could introduce more interesting solutions like: * Exclude/Ignore conditionally on class or property level, for eg: a. if the value is null, b. or is undefined or c. its value is the default value - which allows to skip serialization of properties with default values that will be unserialized back with the default value - see https://3v4l.org/6b1Y6 Similar solutions used by: * JMS/Serializer `#[JMS\Serializer\Annotation\SkipWhenEmpty] * .NET `[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]` see https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonignorecondition?view=net-8.0 Cheers, Michał Marcin Brzuchalski

Nicolas Grekas

2 years ago
> Hi Nicolas, > > śr., 3 sty 2024 o 08:12 Nicolas Grekas <nicolas.grekas+php@gmail.com> > napisał(a): > >> Hi Max, >> >> Hi, I'd like to propose a new attribute, #[NotSerializable]. This >> > functionality is already available for internal classes - userspace >> should >> > benefit from it, too. >> > >> > The RFC: https://wiki.php.net/rfc/not_serializable >> > Proposed implementation: https://github.com/php/php-src/pull/12788 >> > >> > Please let me know what you think. >> > >> >> Regarding the inheritance-related behavior ("The non-serializable flag is >> inherited by descendants"), this is very unlike any other attributes, and >> this actively prevents writing a child class that'd make a parent >> serializable if it wants to. >> >> To me, if this is really the behavior we want, then the attribute should >> be >> replaced by a maker interface. >> Then, a simple "instanceof NotSerializable" would be enough instead of >> adding yet another method to ReflectionClass. >> > > This should be possible without ReflectionClass, see > https://3v4l.org/N3fmO >
Sure. My main point is : why use an attribute? this should be an interface to me. All semantics match an interface.
> From the serialization libraries you can find similar attributes >
Those a very different, because they tackle the problem from the *external* angle: the attributes there describe how a system *external* to the class itself should best serialize an object. There, attributes make sense, because they enrich the description of the class without forcibly telling what to do with the object But in the RFC, we're talking about the object deciding itself how it should be (not) serialized. This is enforced and thus belongs to the typesystem - not to an attribute. IMHO Cheers, Nicolas

Michał Brzuchalski

2 years ago
śr., 3 sty 2024 o 10:09 Nicolas Grekas <nicolas.grekas+php@gmail.com> napisał(a):
> Hi Nicolas, >> >> śr., 3 sty 2024 o 08:12 Nicolas Grekas <nicolas.grekas+php@gmail.com> >> napisał(a): >> >>> Hi Max, >>> >>> Hi, I'd like to propose a new attribute, #[NotSerializable]. This >>> > functionality is already available for internal classes - userspace >>> should >>> > benefit from it, too. >>> > >>> > The RFC: https://wiki.php.net/rfc/not_serializable >>> > Proposed implementation: https://github.com/php/php-src/pull/12788 >>> > >>> > Please let me know what you think. >>> > >>> >>> Regarding the inheritance-related behavior ("The non-serializable flag is >>> inherited by descendants"), this is very unlike any other attributes, and >>> this actively prevents writing a child class that'd make a parent >>> serializable if it wants to. >>> >>> To me, if this is really the behavior we want, then the attribute should >>> be >>> replaced by a maker interface. >>> Then, a simple "instanceof NotSerializable" would be enough instead of >>> adding yet another method to ReflectionClass. >>> >> >> This should be possible without ReflectionClass, see >> https://3v4l.org/N3fmO >> > > Sure. > > My main point is : why use an attribute? this should be an interface to > me. All semantics match an interface. > > >> From the serialization libraries you can find similar attributes >> > > Those a very different, because they tackle the problem from the > *external* angle: the attributes there describe how a system *external* to > the class itself should best serialize an object. There, attributes make > sense, because they enrich the description of the class without forcibly > telling what to do with the object > > But in the RFC, we're talking about the object deciding itself how it > should be (not) serialized. This is enforced and thus belongs to the > typesystem - not to an attribute. >
But then what should implement the NotSerializable interface? If you want to ignore a string-typed property there would be no option to mark it with a NotSerializable interface Consider "baz" property in this example: class Foo { protected int $bar = 1; #[NotSerializable] protected string $baz = 2; } Cheers, Michał Marcin Brzuchalski

Nicolas Grekas

2 years ago
> > >>> śr., 3 sty 2024 o 08:12 Nicolas Grekas <nicolas.grekas+php@gmail.com> >>> napisał(a): >>> >>>> Hi Max, >>>> >>>> Hi, I'd like to propose a new attribute, #[NotSerializable]. This >>>> > functionality is already available for internal classes - userspace >>>> should >>>> > benefit from it, too. >>>> > >>>> > The RFC: https://wiki.php.net/rfc/not_serializable >>>> > Proposed implementation: https://github.com/php/php-src/pull/12788 >>>> > >>>> > Please let me know what you think. >>>> > >>>> >>>> Regarding the inheritance-related behavior ("The non-serializable flag >>>> is >>>> inherited by descendants"), this is very unlike any other attributes, >>>> and >>>> this actively prevents writing a child class that'd make a parent >>>> serializable if it wants to. >>>> >>>> To me, if this is really the behavior we want, then the attribute >>>> should be >>>> replaced by a maker interface. >>>> Then, a simple "instanceof NotSerializable" would be enough instead of >>>> adding yet another method to ReflectionClass. >>>> >>> >>> This should be possible without ReflectionClass, see >>> https://3v4l.org/N3fmO >>> >> >> Sure. >> >> My main point is : why use an attribute? this should be an interface to >> me. All semantics match an interface. >> >> >>> From the serialization libraries you can find similar attributes >>> >> >> Those a very different, because they tackle the problem from the >> *external* angle: the attributes there describe how a system *external* to >> the class itself should best serialize an object. There, attributes make >> sense, because they enrich the description of the class without forcibly >> telling what to do with the object >> >> But in the RFC, we're talking about the object deciding itself how it >> should be (not) serialized. This is enforced and thus belongs to the >> typesystem - not to an attribute. >> > > But then what should implement the NotSerializable interface? > If you want to ignore a string-typed property there would be no option to > mark it with a NotSerializable interface > Consider "baz" property in this example: > > class Foo { > protected int $bar = 1; > #[NotSerializable] > protected string $baz = 2; > } >
The attribute is #[Attribute(Attribute::TARGET_CLASS] so I'm not sure why you consider this as it's not mentioned in the RFC (and I'm not sure it would make sense).

Michał Brzuchalski

2 years ago
śr., 3 sty 2024 o 11:10 Nicolas Grekas <nicolas.grekas+php@gmail.com> napisał(a):
> >>>> śr., 3 sty 2024 o 08:12 Nicolas Grekas <nicolas.grekas+php@gmail.com> >>>> napisał(a): >>>> >>>>> Hi Max, >>>>> >>>>> Hi, I'd like to propose a new attribute, #[NotSerializable]. This >>>>> > functionality is already available for internal classes - userspace >>>>> should >>>>> > benefit from it, too. >>>>> > >>>>> > The RFC: https://wiki.php.net/rfc/not_serializable >>>>> > Proposed implementation: https://github.com/php/php-src/pull/12788 >>>>> > >>>>> > Please let me know what you think. >>>>> > >>>>> >>>>> Regarding the inheritance-related behavior ("The non-serializable flag >>>>> is >>>>> inherited by descendants"), this is very unlike any other attributes, >>>>> and >>>>> this actively prevents writing a child class that'd make a parent >>>>> serializable if it wants to. >>>>> >>>>> To me, if this is really the behavior we want, then the attribute >>>>> should be >>>>> replaced by a maker interface. >>>>> Then, a simple "instanceof NotSerializable" would be enough instead of >>>>> adding yet another method to ReflectionClass. >>>>> >>>> >>>> This should be possible without ReflectionClass, see >>>> https://3v4l.org/N3fmO >>>> >>> >>> Sure. >>> >>> My main point is : why use an attribute? this should be an interface to >>> me. All semantics match an interface. >>> >>> >>>> From the serialization libraries you can find similar attributes >>>> >>> >>> Those a very different, because they tackle the problem from the >>> *external* angle: the attributes there describe how a system *external* to >>> the class itself should best serialize an object. There, attributes make >>> sense, because they enrich the description of the class without forcibly >>> telling what to do with the object >>> >>> But in the RFC, we're talking about the object deciding itself how it >>> should be (not) serialized. This is enforced and thus belongs to the >>> typesystem - not to an attribute. >>> >> >> But then what should implement the NotSerializable interface? >> If you want to ignore a string-typed property there would be no option to >> mark it with a NotSerializable interface >> Consider "baz" property in this example: >> >> class Foo { >> protected int $bar = 1; >> #[NotSerializable] >> protected string $baz = 2; >> } >> > > > The attribute is #[Attribute(Attribute::TARGET_CLASS] so I'm not sure why > you consider this as it's not mentioned in the RFC (and I'm not sure it > would make sense). >
Apologies, apparently I didn't read. Cheers

Max Semenik

2 years ago
On Wed, Jan 3, 2024 at 10:11 AM Nicolas Grekas <nicolas.grekas+php@gmail.com> wrote:
> > Regarding the inheritance-related behavior ("The non-serializable flag is > inherited by descendants"), this is very unlike any other attributes, and > this actively prevents writing a child class that'd make a parent > serializable if it wants to. > > To me, if this is really the behavior we want, then the attribute should > be replaced by a maker interface. > Then, a simple "instanceof NotSerializable" would be enough instead of > adding yet another method to ReflectionClass. >
As explained in the RFC, this implementation hooks to the already existing feature. Additionally, checking a bit in zend_class_entry::ce_flags is much faster than checking inheritance.
> this actively prevents writing a child class that'd make a parent
serializable if it wants to. Wouldn't this violate LSP?
-- Best regards, Max Semenik

Girgias

2 years ago
On Wed, 3 Jan 2024 at 13:17, Max Semenik <maxsem.wiki@gmail.com> wrote:
> > this actively prevents writing a child class that'd make a parent > serializable if it wants to. > > Wouldn't this violate LSP? >
No it doesn't. Making a child class unserializable if the parent is serializable however is violating LSP, and probably a test should be added for this. Best regards, Gina P. Banyard

Larry Garfield

2 years ago
On Wed, Jan 3, 2024, at 7:11 AM, Nicolas Grekas wrote:
> Hi Max, > > Hi, I'd like to propose a new attribute, #[NotSerializable]. This >> functionality is already available for internal classes - userspace should >> benefit from it, too. >> >> The RFC: https://wiki.php.net/rfc/not_serializable >> Proposed implementation: https://github.com/php/php-src/pull/12788 >> >> Please let me know what you think. >> > > Regarding the inheritance-related behavior ("The non-serializable flag is > inherited by descendants"), this is very unlike any other attributes, and > this actively prevents writing a child class that'd make a parent > serializable if it wants to. > > To me, if this is really the behavior we want, then the attribute should be > replaced by a maker interface. > Then, a simple "instanceof NotSerializable" would be enough instead of > adding yet another method to ReflectionClass. > > Cheers, > Nicolas
I think I'm inclined to agree with Nicolas. If the intent here is to impact mainly serialize()/unserialize(), then a marker interface seems like it has the desired logic "built in" more than an attribute does. It would only work at the object level, but that seems correct in this case. For excluding individual properties, __serialize()/__unserialize() already support that use case. That's one of the main uses for them. User-space serializers (Crell/Serde, JMS, Symfony, etc.) could easily be modified to honor the interface as well for external serialization. --Larry Garfield

Benjamin Eberlei

320 days ago
Am 03.01.2024, 20:21:22 schrieb Larry Garfield <larry@garfieldtech.com>:
> On Wed, Jan 3, 2024, at 7:11 AM, Nicolas Grekas wrote: > > Hi Max, > > > Hi, I'd like to propose a new attribute, #[NotSerializable]. This > > > functionality is already available for internal classes - userspace > should > > > benefit from it, too. > > > > > > The RFC: https://wiki.php.net/rfc/not_serializable > > > Proposed implementation: https://github.com/php/php-src/pull/12788 > > > > > > Please let me know what you think. > > > > > > Regarding the inheritance-related behavior ("The non-serializable flag is > > inherited by descendants"), this is very unlike any other attributes, and > > this actively prevents writing a child class that'd make a parent > > serializable if it wants to. > > > To me, if this is really the behavior we want, then the attribute should be > > replaced by a maker interface. > > Then, a simple "instanceof NotSerializable" would be enough instead of > > adding yet another method to ReflectionClass. > > > Cheers, > > Nicolas > > > I think I'm inclined to agree with Nicolas. If the intent here is to > impact mainly serialize()/unserialize(), then a marker interface seems like > it has the desired logic "built in" more than an attribute does. It would > only work at the object level, but that seems correct in this case. For > excluding individual properties, __serialize()/__unserialize() already > support that use case. That's one of the main uses for them. > > User-space serializers (Crell/Serde, JMS, Symfony, etc.) could easily be > modified to honor the interface as well for external serialization. >
I have to disagree, and would favor this more when it becomes an attribute vs a marker interface. Attributes are precisely there to avoid marker interfaces. And there is no precedent in PHP of something similar that suggests we should use a marker interface. The impact on inheritance is not related to this being an attribute, it is already the behavior of the engine with the ZEND_ACC_NOT_SERIALIZABLE flag, you can’t extend PDO and serialize its decendent for example. Ant there is no way to overwrite that in the child class either. Yes, with an attribute it requires considerably more code to check for serializability in a user-space code, than to check for a Marker interface. But thats not a good enough reason to introduce a new pattern into the language. The same argument could be made to anything related attributes why we added that to reflection and not added „easier functions“.