Skip to content

Conversation

DarkGhostHunter
Copy link
Contributor

@DarkGhostHunter DarkGhostHunter commented Sep 29, 2025

The problem

When adding virtual properties to models, serializing them (like when happens when these are stored in a cache store) will return an error.

For example, consider the following model:

class Cart extends Model
{

    public $total {
        get => $this->items->sumBy('net') * $this->items->sumBy('amount');
        set {
            $count = $this->items->sumBy('amount');

            $this->items->each(fn ($item) => $item->net = $value / $count);
        }
    }

    // ..
}

When trying to serialize it with serialize($model), like it happens when using the application cache, it will return an error like this:

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

This may be cumbersome if someone is adding virtual properties to a model for whatever reason, like live calculation or else, because the patchwork is to alter the __sleep() method on each offending class, or use alternative abstract Model and hope nothing breaks in between.

The Fix

The fix uses a simple (array) cast to the model instance to get the non-virtual keys, and an array_map to clean the name of each key returned.

@taylorotwell
Copy link
Member

Can this have a test?

@taylorotwell taylorotwell marked this pull request as draft September 29, 2025 09:23
@DarkGhostHunter
Copy link
Contributor Author

There. I fixed it. Took some magic because it's PHP 8.4 code running on previous versions, so I had to add a reusable rule to the styleci.yml and use eval so PHP didn't pick up the code that wasn't able to understand.

@DarkGhostHunter DarkGhostHunter marked this pull request as ready for review September 29, 2025 20:40
@taylorotwell
Copy link
Member

Is there a way to do this without the array cast?

@taylorotwell taylorotwell marked this pull request as draft September 30, 2025 12:51
@dkulyk
Copy link
Contributor

dkulyk commented Sep 30, 2025

    public function __serialize(): array
    {
        $this->mergeAttributesFromCachedCasts();

        $props = get_mangled_object_vars($this);

        // Remove the attributes that are should be unserialized with default values
        unset(
            $props["\0*\0classCastCache"],
            $props["\0*\0attributeCastCache"],
            $props["\0*\0relationAutoloadCallback"],
            $props["\0*\0relationAutoloadContext"],
        );

        return $props;
    }

It is only suggestion.

  1. __serialize will make the same serialized output like __sleep except compared to the current implementation - get_mangled_object_vars will returns all private props for all inherited classes, which is more correct behavior

  2. We no need reset any caches, only remove them from serialized data.

get_mangled_object_vars($obj) is equivalent of (array)$obj and ignores virtual and unitialized props.

@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Oct 1, 2025

Using get_mangled_object_vars() still doesn't save you from cleaning the property names, and from what I know, is slower than just (array) $this while giving the exact same results.

I modified the PR with your suggestion for removing the cache keys from the props instead of the instance.

Another alternative is to use ReflectionObject but not a fan of that since it adds some overhead that you may skip if you set another cache in the class, and even then it wouldn't be a solution if a developer sets dynamic properties into the model.

@DarkGhostHunter DarkGhostHunter marked this pull request as ready for review October 1, 2025 18:44
@dkulyk
Copy link
Contributor

dkulyk commented Oct 1, 2025

still doesn't save you from cleaning the property names

You no need to clean them if you will use __serialize instead __sleep as in the example

@DarkGhostHunter
Copy link
Contributor Author

DarkGhostHunter commented Oct 1, 2025 via email

@DarkGhostHunter
Copy link
Contributor Author

Can someone te run this again. Seems that docker failed.

@crynobone crynobone closed this Oct 5, 2025
@crynobone crynobone reopened this Oct 5, 2025
@taylorotwell
Copy link
Member

May revisit this for Laravel 13.x. Will note it on my todo list.

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