Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DOMNode serialization on PHP ^8.1 #8996

Closed
wazelin opened this issue Jul 13, 2022 · 1 comment
Closed

DOMNode serialization on PHP ^8.1 #8996

wazelin opened this issue Jul 13, 2022 · 1 comment

Comments

@wazelin
Copy link

wazelin commented Jul 13, 2022

Description

The following code:

<?php

class SerializableDomDocument extends DOMDocument
{
    private $xmlData;

    public function __sleep(): array
    {
        $this->xmlData = $this->saveXML();
        return ['xmlData'];
    }

    public function __wakeup(): void
    {
        $this->loadXML($this->xmlData);
    }
}


$dom = new SerializableDomDocument('1.0', 'UTF-8');
$dom->loadXML('<tag>value</tag>');

$serialized = serialize($dom);
$unserialized = unserialize($serialized);

echo "Serialized:\n-----------\n$serialized\n-----------\nRestored:\n-----------\n{$unserialized->saveXml()}";

Resulted in this output:

Fatal error: Uncaught Exception: Serialization of 'SerializableDomDocument' is not allowed in script.php:20
Stack trace:
#0 script.php(20): serialize(Object(SerializableDomDocument))
#1 {main}
  thrown in script.php on line 20

But I expected this output instead:

Serialized:
-----------
O:23:"SerializableDomDocument":1:{s:32:"SerializableDomDocumentxmlData";s:39:"<?xml version="1.0"?>
<tag>value</tag>
";}
-----------
Restored:
-----------
<?xml version="1.0"?>
<tag>value</tag>

It appears to be sort of an expected behavior of PHP 8.1 due to ca94d55, but I would still expect:

  1. the change to be marked as a breaking one in the PHP 8.1 release notes;
  2. serialization to be allowed if the descendant class explicitly states it is serializable by providing __serialize() / __unserialize() or __sleep() / __wakeup() method pairs.

PHP Version

8.1.0 - 8.1.9

Operating System

No response

@cmb69
Copy link
Member

cmb69 commented Jul 13, 2022

Oh, it likely has been overlooked that this might break code if the class is subclassed. I agree that we need to document this change (some other classes/extensions may have the same issue), and that we should look into supporting (un)serialization for respective subclasses.

nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 12, 2023
Co-authored-by: wazelin <contact@sergeimikhailov.com>
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 12, 2023
…sses if the appropriate methods are present

This introduces the class flag ZEND_ACC_SUBCLASS_SERIALIZABLE, which
allows classes which are not serializable / deserializable to become
that if a subclass implements the appropriate methods.
Setting this flag through the stubs can be done using
@subclass-serializable.
Introducing this through behaviour a new flag allows for opt-in
behaviour, which is backwards compatible.

Fixes phpGH-8996
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 12, 2023
Co-authored-by: wazelin <contact@sergeimikhailov.com>
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 13, 2023
…sses if the appropriate methods are present

This introduces the class flag ZEND_ACC_SUBCLASS_SERIALIZABLE, which
allows classes which are not serializable / deserializable to become
that if a subclass implements the appropriate methods.
Setting this flag through the stubs can be done using
@subclass-serializable.
Introducing this through behaviour a new flag allows for opt-in
behaviour, which is backwards compatible.

Fixes phpGH-8996
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 13, 2023
Co-authored-by: wazelin <contact@sergeimikhailov.com>
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 19, 2023
…sses if the appropriate methods are present

This introduces the class flag ZEND_ACC_SUBCLASS_SERIALIZABLE, which
allows classes which are not serializable / deserializable to become
that if a subclass implements the appropriate methods.
Setting this flag through the stubs can be done using
@subclass-serializable.
Introducing this through behaviour a new flag allows for opt-in
behaviour, which is backwards compatible.

Fixes phpGH-8996
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 19, 2023
Co-authored-by: wazelin <contact@sergeimikhailov.com>
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 19, 2023
…sses if the appropriate methods are present

This introduces the class flag ZEND_ACC_SUBCLASS_SERIALIZABLE, which
allows classes which are not serializable / deserializable to become
that if a subclass implements the appropriate methods.
Setting this flag through the stubs can be done using
@subclass-serializable.
Introducing this through behaviour a new flag allows for opt-in
behaviour, which is backwards compatible.

Fixes phpGH-8996
nielsdos added a commit to nielsdos/php-src that referenced this issue Jan 19, 2023
Co-authored-by: wazelin <contact@sergeimikhailov.com>
nielsdos added a commit to nielsdos/php-src that referenced this issue Oct 8, 2023
…sses if the appropriate methods are present

This introduces the class flag ZEND_ACC_SUBCLASS_SERIALIZABLE, which
allows classes which are not serializable / deserializable to become
that if a subclass implements the appropriate methods.
Setting this flag through the stubs can be done using
@subclass-serializable.
Introducing this through behaviour a new flag allows for opt-in
behaviour, which is backwards compatible.

Fixes phpGH-8996.
nielsdos added a commit to nielsdos/php-src that referenced this issue Oct 8, 2023
Co-authored-by: wazelin <contact@sergeimikhailov.com>
@nielsdos nielsdos self-assigned this Oct 8, 2023
nielsdos added a commit to nielsdos/php-src that referenced this issue Oct 8, 2023
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>
nielsdos added a commit that referenced this issue Oct 9, 2023
* PHP-8.1:
  Fix GH-8996: DOMNode serialization on PHP ^8.1
  Fix GH-12380: JIT+private array property access inside closure accesses private property in child class
nielsdos added a commit that referenced this issue Oct 9, 2023
* PHP-8.2:
  Fix GH-8996: DOMNode serialization on PHP ^8.1
  Fix GH-12380: JIT+private array property access inside closure accesses private property in child class
nielsdos added a commit that referenced this issue Oct 9, 2023
* PHP-8.3:
  Fix GH-8996: DOMNode serialization on PHP ^8.1
  Fix GH-12380: JIT+private array property access inside closure accesses private property in child class
nielsdos added a commit to nielsdos/php-src that referenced this issue Oct 10, 2023
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>
isfedorov added a commit to JetBrains/phpstorm-stubs that referenced this issue Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants