Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121975
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-1178-geeaf0069a7-fm-20231114.001-geeaf0069
MIME-Version: 1.0
Message-ID: <f8a10415-6fc2-4083-9ac9-89044a7ca30e@app.fastmail.com>
In-Reply-To: <20add1b7-24ed-42be-ae84-99ef81480fa2@gmail.com>
References: 
 <CAGgaK7+w_zdp30Pi5nJ-y+GPEkfc2xHfFUW+DwdpNtY8iPjMHg@mail.gmail.com>
 <20add1b7-24ed-42be-ae84-99ef81480fa2@gmail.com>
Date: Sat, 09 Dec 2023 10:28:38 -0600
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] [RFC][Discussion] NotSerializable attribute
From: larry@garfieldtech.com ("Larry Garfield")

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