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

[6.x] Serialization of models on PHP 7.4 with compatibility with PHP 7.3 and below #30605

Merged
merged 6 commits into from
Nov 19, 2019
Merged

[6.x] Serialization of models on PHP 7.4 with compatibility with PHP 7.3 and below #30605

merged 6 commits into from
Nov 19, 2019

Conversation

dkulyk
Copy link
Contributor

@dkulyk dkulyk commented Nov 15, 2019

This is rewrited #30604 with using __serialize/__unserialize

Tests are leaved from #30604

This implementation no needed of additional property and backward compatibility with older version of PHP(<7.4)

@dkulyk dkulyk changed the title Serialization of models with compatibility with PHP 7.3 and below Serialization of models on PHP 7.4 with compatibility with PHP 7.3 and below Nov 15, 2019
));
if (! array_key_exists($name, $values)) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part will unfortunately break jobs which are already queued. If any app is upgraded with these changes those jobs won't be able to unserialize anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?
In previous implementation anything won't be able unserialize anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP <7.4 does not use __serialize. If you will upgrade only Laravel php still used __slee/__wakeup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because any already queued job will still have their model identifiers set on the properties. If you upgrade to php 7.4 then your queued jobs will break while with the current implementation you can upgrade to php 7.4 smoothly.

We can do the unserialize thing in a year or two perhaps when we know most people have transitioned to php 7.4

Copy link
Contributor Author

@dkulyk dkulyk Nov 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... then php pass all properties to __unserialize as array. See #30604 (comment)

Copy link
Contributor Author

@dkulyk dkulyk Nov 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added serialization structure test.
In PHP 7.4 and PHP <7.3 serialized objects are identical.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks for that. If @taylorotwell is okay with it then this PR is fine by me.

@driesvints driesvints changed the title Serialization of models on PHP 7.4 with compatibility with PHP 7.3 and below [6.x] Serialization of models on PHP 7.4 with compatibility with PHP 7.3 and below Nov 15, 2019
@driesvints
Copy link
Member

@dkulyk thanks for this! One concern atm with regards to BC for existing queued jobs.

if ($property->isPrivate()) {
$name = "\0{$class}\0{$name}";
} elseif ($property->isProtected()) {
$name = "\0*\0{$name}";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP adds \0*\0 for protected and \0className\0 for private properties in serialization.
serialization_objects_test, zend_mangle_property_name and usage 1, 2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed for backward compatibility with php7.3 workers.

A php7.3 serialize command will produce this name in the serialized array so by naming it this way a 7.3 worker can work together with a 7.4 worker (as long as it's not using typed properties).

@netpok
Copy link
Contributor

netpok commented Nov 18, 2019

One thing I noticed: the parent classes private variable are lost this way:

take the following code (which is basically the same without the model wrapping):

use ReflectionClass;

class A extends C
{
    use B;
    private $a = 1;
    protected $b = 2;
    public $c = 3;

    public function __serialize()
    {
        $values = [];
        $properties = (new ReflectionClass($this))->getProperties();
        $class = get_class($this);
        foreach ($properties as $property) {
            if ($property->isStatic()) {
                continue;
            }
            $name = $property->getName();
            if ($property->isPrivate()) {
                $name = "\0{$class}\0{$name}";
            } elseif ($property->isProtected()) {
                $name = "\0*\0{$name}";
            }
            $property->setAccessible(true);
            $values[$name] = $property->getValue($this);
        }
        return $values;
    }

}

trait B{
    private $d = 4;
}

class C{
    private $e = 5;
}

calling serialize(new A) with the __serialize method defined will result in:

"O:5:"App\A":4:{s:8:"\0App\A\0a";i:1;s:4:"\0*\0b";i:2;s:1:"c";i:3;s:8:"\0App\A\0d";i:4;}"

calling serialize(new A) without the __serialize method defined will result in:

"O:5:"App\A":5:{s:8:"\0App\A\0a";i:1;s:4:"\0*\0b";i:2;s:1:"c";i:3;s:8:"\0App\C\0e";i:5;s:8:"\0App\A\0d";i:4;}"

Meaning the C::e variable were lost in the serialization process.


A possible solution would be to travel up to the root class and get all the private properties:

    public function __serialize()
    {
        $values = [];
        $reflectionClass = new ReflectionClass($this);

        $properties = ($reflectionClass)->getProperties();

        $class = $reflectionClass->getName;
        foreach ($properties as $property) {
            if ($property->isStatic()) {
                continue;
            }
            $name = $property->getName();
            if ($property->isPrivate()) {
                $name = "\0{$class}\0{$name}";
            } elseif ($property->isProtected()) {
                $name = "\0*\0{$name}";
            }
            $property->setAccessible(true);
            $values[$name] = $property->getValue($this);
        }

        while ($reflectionClass = $reflectionClass->getParentClass()) {
            $class = $reflectionClass->getName;

            foreach ($reflectionClass->getProperties(ReflectionProperty::IS_PRIVATE) as $property) {
                $name = $property->getName();
                $name = "\0{$class}\0{$name}";
                $property->setAccessible(true);
                $values[$name] = $property->getValue($this);
            }
        }

        return $values;
    }

@dkulyk
Copy link
Contributor Author

dkulyk commented Nov 18, 2019

One thing I noticed: the parent classes private variable are lost this way:
I've added test for this. for 7.3 also private fields not present for parent class

@netpok
Copy link
Contributor

netpok commented Nov 18, 2019

@dkulyk I sent a possible solution, of course the serialization logic need to be changed to the correct one


Also some performance measurements should be done, maybe somehow cache the serializeable property names if needed.

@dkulyk
Copy link
Contributor Author

dkulyk commented Nov 18, 2019

__sleep

It is not possible for __sleep() to return names of private properties in parent classes. Doing this will result in an E_NOTICE level error.

But it can be implemented in __serialize

@netpok
Copy link
Contributor

netpok commented Nov 18, 2019

I only tested the latter so there is a chance the other one never worked.

Tested with current implementation, it does indeed remove parent private variables, maybe it would be preferred to keep the exact working (as it can be implemented in a way more optimized fashion).

@netpok
Copy link
Contributor

netpok commented Nov 18, 2019

Probably the best way would be to drop private variable support altogether in v7.x, that would allow to access parameters with $this->{$name} meaning the reflection could be "booted" once per class per session.

On first run class public and protected properties are cached, on each subsequent call it is only pulled form the cache.

Also private variables could be used after this to handle internal non-serializable stuff.

@driesvints
Copy link
Member

@netpok excellent point. Maybe it's indeed better to only serialize protected and public and leave private alone?

@dkulyk
Copy link
Contributor Author

dkulyk commented Nov 19, 2019

This PR implements same behavior like for PHP 7.3. If @taylorotwell will decided what need remove private properties or add from parent classes - I'll change it.

@driesvints
Copy link
Member

@dkulyk that's true 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants