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] Allow model serialization for typed properties #30604

Merged
merged 5 commits into from Nov 15, 2019

Conversation

@driesvints
Copy link
Member

driesvints commented Nov 15, 2019

At the moment it's not possible to use model serialization on jobs with typed properties (which is coming to PHP 7.4). The reason for this is that the property values get replaced with a ModelIdentifier instance which contains all the info to unserialize the model or collection later on.

This new implementation takes the approach of saving those ModelIdentifiers on a separate $identifiers property and replaces the original property values with empty instances of those models or collections. This way, type safety is preserved and we can use typed properties on queueable jobs.

This is technically a breaking change because if you expect the properties to contain the ModelIdentifier you'll now get an empty model or collection after serialization. But this is also only happening if you're doing anything with the model after serialization and before sending it to the queue, which seems unlikely.

Fixes #30494

Small side note: one small other thing that I thought of was if people would be overriding a model constructor with different arguments other than the default that this wouldn't work but I found out that in the current unserialization process the same approach is taken and thus that wouldn't work anyway at the moment. It's also my personal opinion that people shouldn't override a collection or model's constructor but best use named constructors in these cases.

driesvints added 2 commits Nov 15, 2019
@driesvints

This comment has been minimized.

Copy link
Member Author

driesvints commented Nov 15, 2019

It seems StyleCI is failing because it doesn't supports typed properties yet.

cc @GrahamCampbell

@driesvints driesvints force-pushed the php74-fixes branch from 3d7b61f to ea89fe8 Nov 15, 2019
@driesvints

This comment has been minimized.

Copy link
Member Author

driesvints commented Nov 15, 2019

I've ignored the file in StyleCI for now.

@dkulyk

This comment has been minimized.

Copy link
Contributor

dkulyk commented Nov 15, 2019

I think need to use __serialize/__unserialize for this. This also added on PHP 7.4

@driesvints

This comment has been minimized.

Copy link
Member Author

driesvints commented Nov 15, 2019

@dkulyk that would change the entire narrative of this functionality. The idea behind the current approach is that a fresh instance is retrieved while if we'd serialize everything then that would mean the exact object as is would be serialized.

@driesvints

This comment has been minimized.

Copy link
Member Author

driesvints commented Nov 15, 2019

@dkulyk also: we'll probably only be able to support that when PHP 7.4 is the new minimum version which won't be before September 2021.

@dkulyk

This comment has been minimized.

Copy link
Contributor

dkulyk commented Nov 15, 2019

For PHP 7.3 and below will work old behavior. for 7.4+ new.
BTW

class A{
    public $a = 2;
    protected $b = "ss";
    private $c = false;

    public function __serialize()
    {
        return [
            'a' => $this->a,
            '�*�b' => $this->b, 
            '�A�c' => $this->c 
        ];
    }
serialize(new A) -> unserialized on 7.3 and below:
O:1:"A":3:{s:1:"a";i:1;s:4:"�*�b";s:1:"s";s:4:"�A�c";b:1;} //PHP 7.3
O:1:"A":3:{s:1:"a";i:1;s:4:"�*�b";s:1:"s";s:4:"�A�c";b:1;} //PHP 7.4
@taylorotwell taylorotwell merged commit 054b9a3 into 6.x Nov 15, 2019
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details
@driesvints driesvints deleted the php74-fixes branch Nov 15, 2019
@GrahamCampbell

This comment has been minimized.

Copy link
Member

GrahamCampbell commented Nov 16, 2019

@driesvints Yes, StyleCI parses files as PHP 7.3 right now. Support for PHP 7.4 is likely to land Q1 2020. We haven't decided on a launch date yet for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.