PHP Warning raised when Virtual Property Hooks are serialized on Models

[natepisarski]

Laravel Version

12.12.0

PHP Version

8.4.3

Database Driver & Version

No response

Description

Virtual property hooks on Models seem to cause PHP Warnings whenever the Model is serialized. The exact warning is: WARNING serialize(): "<HOOK_NAME>" returned as member variable from __sleep() but does not exist in <PROJECT>eval()'d code.

There is a workaround: overriding __sleep from Illuminate\Database\Eloquent\Model and changing its implementation from:

    public function __sleep()
    {
        $this->mergeAttributesFromCachedCasts();

        $this->classCastCache = [];
        $this->attributeCastCache = [];
        $this->relationAutoloadCallback = null;
        $this->relationAutoloadContext = null;

        return array_keys(get_object_vars($this));
    }

to

    public function __sleep()
    {
        $this->mergeAttributesFromCachedCasts();

        $this->classCastCache = [];
        $this->attributeCastCache = [];
        $this->relationAutoloadCallback = null;
        $this->relationAutoloadContext = null;

        return array_keys(get_mangled_object_vars(this));
    }

Steps To Reproduce

1. Create a new Laravel project
2. In the User model, add this virtual property hook:

    public string $prop {
        get => 'prop';
    }

3. Serialize and Unserialize the model. For example, in a Tinker console:

unserialize(serialize(new User))

This results in the serialization working, but it raises a PHP warning.

[macropay-solutions]

Hi. Besides the fact that the issue is legit and the proposed solution solves it, can you please elaborate why would you use property hooks on a model? Is this related to columns from db?

[natepisarski]

@macropay-solutions There's a few good reasons to do it. If your codebase uses Accessors / Mutators a lot, they can be 1-to-1 replaced with hooks now that they're a feature of the language itself.

You can also use hooks to enforce classes having certain properties via an interface, which you could never do before. So you can define an interface that forces your model to have certain database columns (e.g created_at / updated_at)

[macropay-solutions]

In laravel Accesors/Mutators are not to be replaced with property hooks because the Model (active record) does not work like that in regards to db columns. You can use property hooks for other things and overwrite the __sleep function in an abstract BaseModel or use a trait to overwrite it to fix this issue. There are several discussions about this and people still think The Model is a DTO or Entity like in Symfony.

[macropay-solutions]

@natepisarski see this #55395 (comment)
and this #55395 (reply in thread)

[natepisarski]

@macropay-solutions Thanks for the links. I actually agree fully. I do think the case can be made for virtual hooks specifically (which have no underlying database column). But yeah, using a backed hook with ordinary columns might be awkward.

In my real-world use case I was using virtual hooks to access settings tied to a model in a different table. So I just had a ton that looked like

public string $allowedToEmail {
  get => $this->settings()->get(Setting::AllowedToEmail),
  set => $this->settings()->set(Setting::AllowedToEmail, $value);
}

And these were replacing Laravel accessors/mutators the same.

[macropay-solutions]

Sure you can cover also this case of virtual property hook.

set => $this->settings()->set(Setting::AllowedToEmail, $value);

If settings is a relation, set is not a method in relation. Is that a macro? What is it doing? Updating the relation?

[crynobone]

framework/src/Illuminate/Queue/SerializesModels.php

Lines 39 to 41 in ae955ad

	if (method_exists($property, 'isVirtual') && $property->isVirtual()) {
	continue;
	}

Model are already handled correctly for queue, so I'm not sure manual serialize() and unserialize() should be handled as well. Moving this to Ideas for now.

[natepisarski]

@crynobone It's true that queues work correctly - but this error occurs elsewhere in the framework, and manifested itself in our live application as a crash. I'd recommend promoting it up to an Issue, but it's definitely a little niche.

Other than raw serialize, I've seen it in at least 2 places:

When calling Cache::set / Cache::get on a model:

<?php
declare(strict_types=1);

namespace App\Console\Commands;

use Cache;
use Illuminate\Console\Command;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Foundation\Events\Dispatchable;
use Illuminate\Queue\SerializesModels;

class SampleModel extends Model
{
    public $prop {
        get => 'prop';
    }
}

class TestBug extends Command
{
    protected $signature = 'test:bug';
    protected $description = 'Test bug command';

    public function handle(): int
    {
        Cache::set('badModel', new SampleModel());
        $model = Cache::get('badModel');

        echo $model->prop;
        return Command::SUCCESS;
    }
}

This produces a stack trace in the console that looks like this:

   ErrorException

  serialize(): "prop" returned as member variable from __sleep() but does not exist

  at vendor/laravel/framework/src/Illuminate/Cache/RedisStore.php:454
    450▕      * @return mixed
    451▕      */
    452▕     protected function serialize($value)
    453▕     {
  ➜ 454▕         return $this->shouldBeStoredWithoutSerialization($value) ? $value : serialize($value);
    455▕     }
    456▕
    457▕     /**
    458▕      * Determine if the given value should be stored as plain value.

      +8 vendor frames

  9   app/Console/Commands/TestBug.php:26
      Illuminate\Support\Facades\Facade::__callStatic()
      +13 vendor frames

  23  artisan:13
      Illuminate\Foundation\Application::handleCommand()

And we had it manifest in a different way in our app (I just haven't been able to reproduce it outside of our codebase). The stacktrace in that scenario was this:

Error: Property App\Models\OurModel::$ourProperty is read-only in ourproject/vendor/laravel/serializable-closure/src/Serializers/Native.php:288
Stack trace:
#0 ourproject/vendor/laravel/serializable-closure/src/Serializers/Native.php(288): ReflectionProperty->setValue()
#1 ourproject/vendor/laravel/serializable-closure/src/Serializers/Native.php(285): Laravel\SerializableClosure\Serializers\Native::wrapClosures()
#2 ourproject/vendor/laravel/serializable-closure/src/Serializers/Native.php(125): Laravel\SerializableClosure\Serializers\Native::wrapClosures()
#3 [internal function]: Laravel\SerializableClosure\Serializers\Native->__serialize()
#4 ourproject/vendor/laravel/serializable-closure/src/Serializers/Signed.php(68): serialize()

It's very possible that instance is related to Bus::batch

[santigarcor]

I'm getting the same error. Do we have a solution to this? Or the solution is not to use the language feature?

[macropay-solutions]

@santigarcor the alternative is in the question it self.

[santigarcor]

@macropay-solutions, but this will require to override that method. Should the framework support this, shouldn't it?

[macropay-solutions]

@santigarcor We opted for not using property hooks on the model because they are not fully functional. Setting a value on an array via property hook does not work as stated in the php documentation. The model has many properties in it (being an active record) so we opted for using $attributes via a property $a to avoid clashes between the db column and a model php property. laravel offers getAttributeValue and setAttribute functions for that situation. Laravel is not Symfony and the Model is not an Entity (DTO).

[macropay-solutions]

Safe alternative for property hooks in eloquent model:
https://marius-ciclistu.medium.com/serializable-property-hooks-in-eloquent-model-without-conflicts-398271774cba

[EranNL]

I think this is addressed in this change: #57557

[mrFANRA]

Here is possible situation when getter start hard logic. get_object_vars calling this getter every time...

It might be better to replace get_object_vars with ReflectionClass ...

[marius-ciclistu]

That would add another overhead brake to the execution. Property hooks in active record are like oil in watter.

[mrFANRA]

But in the end we use it together (get_object_vars, ReflectionClass) ...

$keys = [];
foreach (new ReflectionClass($Model)->getProperties() as $property) {
            if (!$property->hasHooks() && !$property->isStatic() && $property->isPublic()) {
                $keys[] = $property->getName();
            }
        }
return $keys;

It is look better in my opinion.

[marius-ciclistu]

For php 8.4 you are right. for php < 8.4 no.

[marius-ciclistu]

You could try a PR with php version check.