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

[8.x] Init the traits when the model is being unserialized #37492

Merged
merged 2 commits into from
May 27, 2021
Merged

[8.x] Init the traits when the model is being unserialized #37492

merged 2 commits into from
May 27, 2021

Conversation

mtawil
Copy link
Contributor

@mtawil mtawil commented May 26, 2021

This change calls the trait initializers when the model is being unserialized; the same happened with the model boot methods.

@GrahamCampbell GrahamCampbell changed the title Init the traits when the model is being unserialized [8.x] Init the traits when the model is being unserialized May 26, 2021
@GrahamCampbell
Copy link
Member

Thanks for this. Please add some tests. :)

@mtawil
Copy link
Contributor Author

mtawil commented May 26, 2021

@GrahamCampbell Sure! just gimme a few hours, and I will be back with tests 👍🏼

@taylorotwell
Copy link
Member

taylorotwell commented May 26, 2021

If the model has already been booted, isn't the trait boot now going to run twice? Possible even more times if another model is unserialized?

Edit: Nevermind - I see this is for initialization and not booting.

@taylorotwell
Copy link
Member

Can you provide an example of the problem you are trying to solve in your code?

@mtawil
Copy link
Contributor Author

mtawil commented May 27, 2021

@taylorotwell, I believe I can think of any scenario of any model that uses any traits with initializations; the initializations won't be executed when the model being unserialized.

Also, check the Model __construct here:

/**
 * Create a new Eloquent model instance.
 *
 * @param  array  $attributes
 * @return void
 */
public function __construct(array $attributes = [])
{
    $this->bootIfNotBooted();

    $this->initializeTraits();

    $this->syncOriginal();

    $this->fill($attributes);
}

It makes sense that the magic method __wakeup should behave the same as __construct do

@mtawil
Copy link
Contributor Author

mtawil commented May 27, 2021

@GrahamCampbell I've added the tests.

@taylorotwell taylorotwell merged commit 41c47ef into laravel:8.x May 27, 2021
@gaborbencebekesi
Copy link

gaborbencebekesi commented Mar 16, 2023

I ran into a problem with this. I put typed property into a trait and initialized in the initialization method. (This sounds a logical design pattern for me, but correct me if I'm wrong.) Then I put the model into cache and fetched; serialize and unserialize methods were called, so __wakeup was activated. Needless to say, trait initialization method ran again and properties were set to default, I lost their state. Example:

trait TestTrait
{
    protected string|null $property;

    protected function initializeTestTrait(): void
    {
        $this->property = null;
    }
}

class TestModel {
    use TestTrait;
}

I'm not sure whether this should be the expected behaviour. I would say no and here're my reasons.

  • Before serialize is called on any model at any time, initialization has been completed. Serialization doesn't change this, so running initialization again is more likely to be reinitialization rather then initialization.

  • By default between serializing and unserializing, a PHP object should keep its state. In current implementation a trait initialization method is not idempotent out of the box. (But boot method is.)

I have some ideas to solve this, but first please share your option on this. Thank you.

@mtawil
Copy link
Contributor Author

mtawil commented Mar 19, 2023

Hi @gaborbencebekesi,

I believe you can check the property state first before resetting it, like this way:

trait TestTrait
{
    protected string|null $property;

    protected function initializeTestTrait(): void
    {
        if (! isset($this->property)) {
            $this->property = null;
        }
    }
}

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