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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 72 additions & 31 deletions src/Illuminate/Queue/SerializesModels.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

namespace Illuminate\Queue;

use Illuminate\Contracts\Database\ModelIdentifier;
use ReflectionClass;
use ReflectionProperty;

Expand All @@ -11,67 +10,109 @@ trait SerializesModels
use SerializesAndRestoresModelIdentifiers;

/**
* The list of serialized model identifiers.
* Prepare the instance for serialization.
*
* @var array
* @return array
*/
protected $modelIdentifiers = [];
public function __sleep()
{
$properties = (new ReflectionClass($this))->getProperties();

foreach ($properties as $property) {
$property->setValue($this, $this->getSerializedPropertyValue(
$this->getPropertyValue($property)
));
}

return array_values(array_filter(array_map(function ($p) {
return $p->isStatic() ? null : $p->getName();
}, $properties)));
}

/**
* Prepare the instance for serialization.
* Restore the model after serialization.
*
* @return void
*/
public function __wakeup()
{
foreach ((new ReflectionClass($this))->getProperties() as $property) {
if ($property->isStatic()) {
continue;
}

$property->setValue($this, $this->getRestoredPropertyValue(
$this->getPropertyValue($property)
));
}
}

/**
* Prepare the instance values for serialization.
*
* @return array
*/
public function __sleep()
public function __serialize()
{
$values = [];

$properties = (new ReflectionClass($this))->getProperties();

$class = get_class($this);

foreach ($properties as $property) {
if ($property->getName() === 'modelIdentifiers') {
if ($property->isStatic()) {
continue;
}

$serializedValue = $this->getSerializedPropertyValue(
$value = $this->getPropertyValue($property)
);
$name = $property->getName();

if ($serializedValue instanceof ModelIdentifier) {
$this->modelIdentifiers[$property->getName()] = $serializedValue;

// Empty instance of the model or collection to support typed properties...
$property->setValue($this, new $value);
} else {
$property->setValue($this, $value);
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).

}

$values[$name] = $this->getSerializedPropertyValue($this->getPropertyValue($property));
}

return array_values(array_filter(array_map(function ($p) {
return $p->isStatic() ? null : $p->getName();
}, $properties)));
return $values;
}

/**
* Restore the model after serialization.
*
* @return void
* @param array $values
* @return array
*/
public function __wakeup()
public function __unserialize(array $values)
{
foreach ((new ReflectionClass($this))->getProperties() as $property) {
if ($property->isStatic() || $property->getName() === 'modelIdentifiers') {
$properties = (new ReflectionClass($this))->getProperties();

$class = get_class($this);

foreach ($properties as $property) {
if ($property->isStatic()) {
continue;
}

if (isset($this->modelIdentifiers[$property->getName()])) {
$value = $this->modelIdentifiers[$property->getName()];
} else {
$value = $this->getPropertyValue($property);
$name = $property->getName();

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

$property->setValue($this, $this->getRestoredPropertyValue(
$value
));
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.


$property->setAccessible(true);
$property->setValue($this, $this->getRestoredPropertyValue($values[$name]));
}

return $values;
}

/**
Expand Down
38 changes: 36 additions & 2 deletions tests/Integration/Queue/ModelSerializationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,8 @@ public function testItSerializesTypedProperties()

$this->assertSame('testbench', $unSerialized->user->getConnectionName());
$this->assertSame('[email protected]', $unSerialized->user->email);
$this->assertSame(5, $unSerialized->id);
$this->assertSame(['James', 'Taylor', 'Mohamed'], $unSerialized->names);
$this->assertSame(5, $unSerialized->getId());
$this->assertSame(['James', 'Taylor', 'Mohamed'], $unSerialized->getNames());

$serialized = serialize(new TypedPropertyCollectionTestClass(ModelSerializationTestUser::on('testbench')->get()));

Expand All @@ -304,6 +304,19 @@ public function testItSerializesTypedProperties()
$this->assertSame('testbench', $unSerialized->users[1]->getConnectionName());
$this->assertSame('[email protected]', $unSerialized->users[1]->email);
}

public function test_model_serialization_structure()
{
$user = ModelSerializationTestUser::create([
'email' => '[email protected]',
]);

$serialized = serialize(new ModelSerializationParentAccessibleTestClass($user, $user, $user));

$this->assertEquals(
'O:78:"Illuminate\\Tests\\Integration\\Queue\\ModelSerializationParentAccessibleTestClass":2:{s:4:"user";O:45:"Illuminate\\Contracts\\Database\\ModelIdentifier":4:{s:5:"class";s:61:"Illuminate\\Tests\\Integration\\Queue\\ModelSerializationTestUser";s:2:"id";i:1;s:9:"relations";a:0:{}s:10:"connection";s:9:"testbench";}s:8:"'."\0".'*'."\0".'user2";O:45:"Illuminate\\Contracts\\Database\\ModelIdentifier":4:{s:5:"class";s:61:"Illuminate\\Tests\\Integration\\Queue\\ModelSerializationTestUser";s:2:"id";i:1;s:9:"relations";a:0:{}s:10:"connection";s:9:"testbench";}}', $serialized
);
}
}

class ModelSerializationTestUser extends Model
Expand Down Expand Up @@ -420,6 +433,27 @@ public function __construct($user)
}
}

class ModelSerializationAccessibleTestClass
{
use SerializesModels;

public $user;
protected $user2;
private $user3;

public function __construct($user, $user2, $user3)
{
$this->user = $user;
$this->user2 = $user2;
$this->user3 = $user3;
}
}

class ModelSerializationParentAccessibleTestClass extends ModelSerializationAccessibleTestClass
{
//
}

class ModelRelationSerializationTestClass
{
use SerializesModels;
Expand Down
20 changes: 18 additions & 2 deletions tests/Integration/Queue/typed-properties.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,32 @@ class TypedPropertyTestClass

public ModelSerializationTestUser $user;

public int $id;
protected int $id;

public array $names;
private array $names;

public function __construct(ModelSerializationTestUser $user, int $id, array $names)
{
$this->user = $user;
$this->id = $id;
$this->names = $names;
}

/**
* @return int
*/
public function getId()
{
return $this->id;
}

/**
* @return array
*/
public function getNames()
{
return $this->names;
}
}

class TypedPropertyCollectionTestClass
Expand Down