RFC: Deprecate json_encode() on classes marked as non-serializable

1 year ago by Philip Hofstetter — view source

unread

Hello,

As per my previous email to the list, I have now created the official RFC
to deprecate calling json_serialize() on instances of classes marked with
ZEND_ACC_NOT_SERIALIZABLE.

https://wiki.php.net/rfc/deprecate-json_encode-nonserializable

I have also created a PR with the implementation here:

https://github.com/php/php-src/pull/15724

I have considered other options, both constraining the implementation to
just Generator and/or to add special cases for Generator (and maybe
Iterator), but they either continue to keep the asymmetry between
serialize() and json_encode() and/or are making things even more
inconsistent.

Please tell me what you think, especially, if you agree that
blanked-deprecating all of ZEND_ACC_NOT_SERIALIZABLE classes is acceptable
BC-wise (after a bit of deliberation over the weekend, I think it is and
most json-serializations of such marked classes are probably unintentional).

Thanks in advance for all comments

Philip

1 year ago by Rob Landers — view source

unread

Hello,

As per my previous email to the list, I have now created the official RFC to deprecate calling json_serialize() on instances of classes marked with ZEND_ACC_NOT_SERIALIZABLE.

https://wiki.php.net/rfc/deprecate-json_encode-nonserializable

I have also created a PR with the implementation here:

https://github.com/php/php-src/pull/15724

I have considered other options, both constraining the implementation to just Generator and/or to add special cases for Generator (and maybe Iterator), but they either continue to keep the asymmetry between serialize() and json_encode() and/or are making things even more inconsistent.

Please tell me what you think, especially, if you agree that blanked-deprecating all of ZEND_ACC_NOT_SERIALIZABLE classes is acceptable BC-wise (after a bit of deliberation over the weekend, I think it is and most json-serializations of such marked classes are probably unintentional).

Thanks in advance for all comments

Philip

Hello Phillip.

I think it would be good to list these non-serializable objects in the RFC. It doesn't have to be exhaustive, but the list includes throwables, weak-maps, weak-references, closures, fibers, etc. Some people may be relying on this behavior (and I'd be curious to know that use case myself).

I just grepped for @not-serializable in php-src stubs, which appears to be the only use of ZEND_ACC_NOT_SERIALIZABLE.

— Rob

1 year ago by Philip Hofstetter — view source

unread

Hello Rob,

I think it would be good to list these non-serializable objects in the RFC.

It doesn't have to be exhaustive, but the list includes throwables,
weak-maps, weak-references, closures, fibers, etc. Some people may be
relying on this behavior (and I'd be curious to know that use case myself).

Thank you for your note. As I went through to the code-base in order to
compile the list for the RFC, it also occurred to me that all anonymous
classes are marked as ZEND_ACC_NOT_SERIALIZABLE, but json_encode()-ing such
classes is unproblematic and probably very much desired for user land
classes.

I have updated the RFC and attached implementation to allow anonymous
classes to be json_encode()’d, provided their parent class is not marked as
ZEND_ACC_NOT_SERIALIZABLE.

Thank you very much

Philip

1 year ago by drealecs@gmail.com — view source

unread

On Tue, Sep 3, 2024 at 2:27 PM Philip Hofstetter phofstetter@sensational.ch
wrote:

Hello,

As per my previous email to the list, I have now created the official RFC
to deprecate calling json_serialize() on instances of classes marked with
ZEND_ACC_NOT_SERIALIZABLE.

https://wiki.php.net/rfc/deprecate-json_encode-nonserializable

Sharing my experience, I never use json_encode on objects but on arrays
(that are both JSON objects or JSON arrays).
When I see an object implementing JsonSerializable, I think it is the wrong
approach, and I am usually able to find a better way.
Maybe if we could go back in time, we would not allow json_encode on an
object, except if it implemented JsonSerializable, but that ship sailed
long ago.

To your proposal, I think the BC break would be pretty big and I don't see
a way it would pass.
On https://www.php.net/json_encode we can read:

If a value to be serialized is an object, then by default only publicly
visible properties will be included.
So that's the expected behaviour.

Yes, you can say that encoding as JSON is just "another serialization
method", but the default method in PHP, using json_encode()/json_decode(),
is not symmetrical when using objects.
And here lies the difference with serialize()/unserialize(), as this one
aims to be symmetrical, and where it can't be, it has a way to stop the
serialization.

I am happy with the current way it works, getting an empty JSON object if
there are no public properties for a Generator or Closure.
And I don't think having an error for them would improve the language in a
meaningful way, and the BC break is not worth it.

Regards,
Alex

1 year ago by Rob Landers — view source

unread

Hello,

As per my previous email to the list, I have now created the official RFC to deprecate calling json_serialize() on instances of classes marked with ZEND_ACC_NOT_SERIALIZABLE.

https://wiki.php.net/rfc/deprecate-json_encode-nonserializable

Sharing my experience, I never use json_encode on objects but on arrays (that are both JSON objects or JSON arrays).
When I see an object implementing JsonSerializable, I think it is the wrong approach, and I am usually able to find a better way.
Maybe if we could go back in time, we would not allow json_encode on an object, except if it implemented JsonSerializable, but that ship sailed long ago.

To your proposal, I think the BC break would be pretty big and I don't see a way it would pass.
On https://www.php.net/json_encode we can read:

If a value to be serialized is an object, then by default only publicly visible properties will be included.
So that's the expected behaviour.

Yes, you can say that encoding as JSON is just "another serialization method", but the default method in PHP, using json_encode()/json_decode(), is not symmetrical when using objects.
And here lies the difference with serialize()/unserialize(), as this one aims to be symmetrical, and where it can't be, it has a way to stop the serialization.

I am happy with the current way it works, getting an empty JSON object if there are no public properties for a Generator or Closure.
And I don't think having an error for them would improve the language in a meaningful way, and the BC break is not worth it.

Regards,
Alex

To add to this, we apparently use json_encode at work to serialize custom exceptions, which appears to work. This RFC would break that, I think.

— Rob

1 year ago by Philip Hofstetter — view source

unread

Hello Rob,

To add to this, we apparently use json_encode at work to serialize custom
exceptions, which appears to work. This RFC would break that, I think.

Exception is not marked as ZEND_ACC_NOT_SERIALIZABLE and would not be
affected.

Philip

1 year ago by John Coggeshall — view source

unread

As per my previous email to the list, I have now created the official RFC to deprecate calling json_serialize() on instances of classes marked with ZEND_ACC_NOT_SERIALIZABLE.

I would suggest we take a step back from this and look at it with a bit more of a wider lens. It seems to me that this would be a good place to have an attribute (e.g. #[NotSerializable] ) that could be defined for any class (with ZEND_ACC_NOT_SERIALIZABLE being automatically given this attribute)? It just seems to be a more holistic approach that makes sense, rather than basing it on internal engine stuff and/or limiting it to internal objects.
Coogle

1 year ago by Rowan Tommins [IMSoP] — view source

unread

I would suggest we take a step back from this and look at it with a bit more of a wider lens. It seems to me that this would be a good place to have an attribute (e.g. #[NotSerializable] ) that could be defined for any class (with ZEND_ACC_NOT_SERIALIZABLE being automatically given this attribute)? It just seems to be a more holistic approach that makes sense, rather than basing it on internal engine stuff and/or limiting it to internal objects.

It's really the opposite: if we had an attribute, it would be used to toggle the existing internal flag.

ZEND_ACC_NOT_SERIALIZABLE is just one of a long list of class, property and method flags which the engine uses internally. You can see the full list here: https://heap.space/xref/php-src/Zend/zend_compile.h?r=58aa6fc8#212

You'll notice that includes things like "final" and "private", which refer to straight-forward syntax in the language, as well as completely internal details like "preloaded". It also includes ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES, which is the flag set if you add the #[AllowDynamicProperties] attribute on a class: https://heap.space/xref/php-src/Zend/zend_attributes.c?r=5a18279b#validate_allow_dynamic_properties

So if we had a #[NotSerializable] attribute (which I agree might be a good idea) it would be implemented by setting the ZEND_ACC_NOT_SERIALIZABLE flag on a class definition, and if this RFC passes, it would automatically apply to json_encode() as well as serialize().

Regards,

Rowan Tommins
[IMSoP]

1 year ago by John Coggeshall — view source

unread

So if we had a #[NotSerializable] attribute (which I agree might be a good idea) it would be implemented by setting the ZEND_ACC_NOT_SERIALIZABLE flag on a class definition, and if this RFC passes, it would automatically apply to json_encode() as well as serialize().

My mistake, I haven't familiarized myself with some of the more recent engine changes but this all makes sense. My point was more to ask the question "Would it be reasonable to expand the scope of this RFC to introduce a #[NotSerializable] attribute along with the rest of the proposal?"
Coogle

1 year ago by Claude Pache — view source

unread

Le 5 sept. 2024 à 18:03, John Coggeshall john@coggeshall.org a écrit :

As per my previous email to the list, I have now created the official RFC to deprecate calling json_serialize() on instances of classes marked with ZEND_ACC_NOT_SERIALIZABLE.

I would suggest we take a step back from this and look at it with a bit more of a wider lens. It seems to me that this would be a good place to have an attribute (e.g. #[NotSerializable] ) that could be defined for any class (with ZEND_ACC_NOT_SERIALIZABLE being automatically given this attribute)? It just seems to be a more holistic approach that makes sense, rather than basing it on internal engine stuff and/or limiting it to internal objects.

Coogle

Hi,

An attribute adds very little value. It doesn’t add new capability, because you can achieve the same effect with a serialiser that throws unconditionally; it is just a nicer syntax. People generally don’t bother making their classes unserialisable unless they have a good reason; having an attribute won’t really change that.

The core problem is that objects are json-serialisable by default, although most of them are not supposed to be json-serialised. Taking a second step back, if we really want to solve the issue, one should:

for internal classes, determine which ones could be json-serialisable (at least, stdClass); for all other classes, json_encode(...) shall be disabled (after a deprecation period);
for user-defined classes, the user shall opt into json-serialisability, either by extending a json-serialisable class, or by using an {attribute / magic method / interface} (chose your bikeshed colour).

—Claude

1 year ago by John Coggeshall — view source

unread

Le 5 sept. 2024 à 18:03, John Coggeshall john@coggeshall.org a écrit :

As per my previous email to the list, I have now created the official RFC to deprecate calling json_serialize() on instances of classes marked with ZEND_ACC_NOT_SERIALIZABLE.

I would suggest we take a step back from this and look at it with a bit more of a wider lens. It seems to me that this would be a good place to have an attribute (e.g. #[NotSerializable] ) that could be defined for any class (with ZEND_ACC_NOT_SERIALIZABLE being automatically given this attribute)? It just seems to be a more holistic approach that makes sense, rather than basing it on internal engine stuff and/or limiting it to internal objects.
Coogle

Hi,

An attribute adds very little value. It doesn’t add new capability, because you can achieve the same effect with a serialiser that throws unconditionally; it is just a nicer syntax. People generally don’t bother making their classes unserialisable unless they have a good reason; having an attribute won’t really change that.
There are plenty of examples in frameworks, etc. where userspace classes are defined that simply shouldn't be serialized for a variety of reasons. It seems odd to me to have the notion internally within the engine that a class can be flagged as not serializable, but yet not expose that same ability to userspace.
Coogle

1 year ago by Rob Landers — view source

unread

Le 5 sept. 2024 à 18:03, John Coggeshall john@coggeshall.org a écrit :

As per my previous email to the list, I have now created the official RFC to deprecate calling json_serialize() on instances of classes marked with ZEND_ACC_NOT_SERIALIZABLE.

I would suggest we take a step back from this and look at it with a bit more of a wider lens. It seems to me that this would be a good place to have an attribute (e.g. #[NotSerializable] ) that could be defined for any class (with ZEND_ACC_NOT_SERIALIZABLE being automatically given this attribute)? It just seems to be a more holistic approach that makes sense, rather than basing it on internal engine stuff and/or limiting it to internal objects.

Coogle

Hi,

An attribute adds very little value. It doesn’t add new capability, because you can achieve the same effect with a serialiser that throws unconditionally; it is just a nicer syntax. People generally don’t bother making their classes unserialisable unless they have a good reason; having an attribute won’t really change that.

You also need to ensure that you make it final, as someone could extend your class and make your borked serializer work.

The core problem is that objects are json-serialisable by default, although most of them are not supposed to be json-serialised. Taking a second step back, if we really want to solve the issue, one should:

for internal classes, determine which ones could be json-serialisable (at least, stdClass); for all other classes, json_encode(...) shall be disabled (after a deprecation period);

for user-defined classes, the user shall opt into json-serialisability, either by extending a json-serialisable class, or by using an {attribute / magic method / interface} (chose your bikeshed colour).

—Claude

I also agree with this to a point: there is the https://www.php.net/manual/en/class.jsonserializable.php interface, after all.

— Rob

11 months ago by tim@bastelstu.be — view source

unread

Am 2024-09-05 18:03, schrieb John Coggeshall:

I would suggest we take a step back from this and look at it with a bit
more of a wider lens. It seems to me that this would be a good place to
have an attribute (e.g. #[NotSerializable] ) that could be defined for
any class (with ZEND_ACC_NOT_SERIALIZABLE being automatically given
this attribute)? It just seems to be a more holistic approach that
makes sense, rather than basing it on internal engine stuff and/or
limiting it to internal objects.

FWIW There already is such an RFC:
https://wiki.php.net/rfc/not_serializable

Discussion thread is here: https://externals.io/message/121969#121969

Best regards
Tim Düsterhus

1 year ago by Vasilii Shpilchin — view source

unread

Hello Philip,

Thank you for your RFC, which is a great effort to improve serialization in
PHP. Generators are indeed super useful and working with them should not be
confusing.

I regret that I missed your previous emails, but I would like to suggest
that you consider the following concerns:

The original serialization and json_encode are not symmetric nor
interchangeable by definition, because JSON doesn't preserve protected
properties and class names. Hence, I don't see them to be "inconsistent" as
they were not designed to be consistent.
The reason why some objects are not serializable is because they
couldn't be fully functioning after deserialization. But this is not a
barrier for JSON encoding, which serializing public properties only.
Current JSON encoding has a simple, straight forward logic, not tied to
serialization in any way.

You have suggested to cast a non-serializable objects to array as a
workaround. Yet, array is the most popular type to JSON in PHP. To me, an
ideal move would be make json_encode to iterate any iterators from the
input structure. Well, this is a different topic.

Best regards,
Vasilii

On Tue, Sep 3, 2024, 7:26 AM Philip Hofstetter phofstetter@sensational.ch
wrote:

Hello,

As per my previous email to the list, I have now created the official RFC
to deprecate calling json_serialize() on instances of classes marked with
ZEND_ACC_NOT_SERIALIZABLE.

https://wiki.php.net/rfc/deprecate-json_encode-nonserializable

I have also created a PR with the implementation here:

https://github.com/php/php-src/pull/15724

I have considered other options, both constraining the implementation to
just Generator and/or to add special cases for Generator (and maybe
Iterator), but they either continue to keep the asymmetry between
serialize() and json_encode() and/or are making things even more
inconsistent.

Please tell me what you think, especially, if you agree that
blanked-deprecating all of ZEND_ACC_NOT_SERIALIZABLE classes is acceptable
BC-wise (after a bit of deliberation over the weekend, I think it is and
most json-serializations of such marked classes are probably unintentional).

Thanks in advance for all comments

Philip