[RFC][Discussion] NotSerializable attribute View as Markdown

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

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--

--

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

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.

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

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 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

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.

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

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

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