Fix GH-8996: DOMNode serialization on PHP ^8.1 #12388
Conversation
PHP 8.1 introduced a seemingly unintentional BC break in ca94d55 by blocking the (un)serialization of DOM objects. This was done because the serialization never really worked and just resulted in an empty object, which upon unserialization just resulted in an object that you can't use. Users can however implement their own serialization methods, but the commit made that impossible as the ACC flag gets passed down to the child class. An approach was tried in php#10307 with a new ACC flag to selectively allow serialization with subclasses if they implement the right methods. However, that was found to be too ad hoc. Instead, let's abuse how the __sleep and __wakeup methods work to throw the exception instead. If the child class implements the __serialize / __unserialize method, then the throwing methods won't be called. Similarly, if the child class implements __sleep and __wakeup, then they're overridden and it doesn't matter that they throw. For the user, this PR has the exact same behaviour for (sub)classes that don't implement the serialization methods: an exception will be thrown. For code that previously implemented subclasses with these methods, this approach will make that code work again. This approach should be both BC preserving and unbreak user's code. For the test: Co-authored-by: wazelin <contact@sergeimikhailov.com>
There was a problem hiding this comment.
This is actually kind of genius.
Only thing I would like to see is a test case with a subclass which implements Serializable
|
What do you think of the wording of the exception? It may need something extra to indicate how a developer can still use serialization? |
I think the suggestion is good, but I might mention |
I have a solution to the problem that can work on 8.1.
Implementing a real serialization and unserialization method isn't really feasible for stable branches, and I'm not even sure how they would correctly behave due to the transient nature even on master, but that's a discussion for another time I guess...
PHP 8.1 introduced a seemingly unintentional BC break in ca94d55 by blocking the (un)serialization of DOM objects.
This was done because the serialization never really worked and just resulted in an empty object, which upon unserialization just resulted in an object that you can't use.
Users can however implement their own serialization methods, but the commit made that impossible as the ACC flag gets passed down to the child class. An approach was tried in #10307 with a new ACC flag to selectively allow serialization with subclasses if they implement the right methods. However, that was found to be too ad hoc.
Instead, let's abuse how the __sleep and __wakeup methods work to throw the exception instead. If the child class implements the __serialize / __unserialize method, then the throwing methods won't be called. Similarly, if the child class implements __sleep and __wakeup, then they're overridden and it doesn't matter that they throw.
For the user, this PR has the exact same behaviour for (sub)classes that don't implement the serialization methods: an exception will be thrown. For code that previously implemented subclasses with these methods, this approach will make that code work again. This approach should be both BC preserving and unbreak user's code.