Exclude property hooks on return of Model::__sleep()

[rafaelqueiroz]

From 57357

[shaedrich]

Is there a reason, you didn't just replace get_object_vars() with get_mangled_object_vars() as suggested in #55801, #57443, and #57199?

[rafaelqueiroz]

@shaedrich Yes, I saw that, there are too many reasons:

• Use the same approach as the project, they use ReflectionClass
• The function get_mangled_object_vars never been used on project
• Prevent breaking changes, backward compatibility

The maintainers always will try keep the things simple and use the same approach.
You need read more code than write, that's the magic.

[shaedrich]

The function get_mangled_object_vars never been used on project

There's always a first time. If that wasn't so, Laravel would still use PHP 5.3.

With get_mangled_object_vars() you wouldn't have to essentially maintain a pre PHP 8.4 and a post PHP 8.4 version, as you would have to with ReflectionProperty::getHooks() as this is a PHP 8.4 method and Laravel 12.x is PHP 8.2, while 13.x will be 8.3, so still not there to remove the condition in which it is wrapped.

The maintainers always will try keep the things simple

get_mangled_object_vars() is simple as it is a one-liner. That just can't be said about your solution.

[taylorotwell]

This honestly feels like the safest solution I've seen thus far.

[AndrewMast]

I was trying be nice here fixing the issue, but your still going bad anyway...

@shaedrich you simple one-line solution, come with a freak show test class, also change the behavior of the framework, looks like someone not deep into the issue.

@marius-ciclistu about perfomance issue, the framework have ReflectionClass on many places, you should read more before you talk.

I'm open to discussion, but only if we can be respectful; otherwise, get out of my way. I saw "Wtf taylorotwell ???" on main issue, there is no space for that on open source projects, BE NICE.

@rafaelqueiroz Hey you are saying "be nice" but also at the same time saying "get out of my way", "freak show", "should read more before they talk", and saying people aren't looking deep into the issue. Including such "un-nice" language in the same message as telling people to "be nice" comes across as rather duplicitous.

Being more respectful is something everyone, including myself, should try to do, especially when collaborating and speaking through written English where so much context and inflection is missing (and English isn't everyone's first language).

[rafaelqueiroz]

@AndrewMast sure thing, you're right, I tried explain, doesn't work and I almost leave as it which was the best. Anyway, you're right, sorry for it and thank you.

[marius-ciclistu]

@rafaelqueiroz congratulations. Your solution got merged. The fastest solution is to leave it as it was. The active reacord does not get along with property hooks.

For someone that will use php 8.4 and will NOT use property hooks, the serialization will take a small hit in terms of performance.

Whoever would use property hooks in the model, overriding the __sleep would had been the best solution. And to make that even simpler, that return could had been extracted into a protected function that could be overridden in a trait or a base model.

@shaedrich There were numerous attempts with get_mangled_object_vars that were rejected before.

[shaedrich]

@shaedrich There were numerous attempts with get_mangled_object_vars that were rejected before.

I know. I just tried to get a good explanation since Taylor isn't known for these. I brought good points that weren't refuted yet.

I was trying be nice here fixing the issue, but your still going bad anyway...
@shaedrich you simple one-line solution, come with a freak show test class, also change the behavior of the framework, looks like someone not deep into the issue.
@marius-ciclistu about perfomance issue, the framework have ReflectionClass on many places, you should read more before you talk.
I'm open to discussion, but only if we can be respectful; otherwise, get out of my way. I saw "Wtf taylorotwell ???" on main issue, there is no space for that on open source projects, BE NICE.

@rafaelqueiroz Hey you are saying "be nice" but also at the same time saying "get out of my way", "freak show", "should read more before they talk", and saying people aren't looking deep into the issue. Including such "un-nice" language in the same message as telling people to "be nice" comes across as rather duplicitous.

Being more respectful is something everyone, including myself, should try to do, especially when collaborating and speaking through written English where so much context and inflection is missing (and English isn't everyone's first language).

My thoughts exactly. Thank you, @AndrewMast 👍🏻 My choice of words might not have been ideal, but I didn't write in that direct rudeness that was expressed towards the two of us. However, as English is not native language of many, both sides might phrase things suboptimal as the other side might misinterpret it.

[marius-ciclistu]

@rafaelqueiroz check out this question inspired by your PR being merged #31778 (comment)

[rafaelqueiroz]

@marius-ciclistu to me, this is completely random, It seems like a lost cause from 2020, good luck.

[marius-ciclistu]

@rafaelqueiroz Thank you for the inspiration and sorry for upsetting you. It lead to that good ideea for avoiding casts but still having casts via property hooks:)

[NickSdot]

Hey @rafaelqueiroz,

how would you feel following up with an optimisation PR as below? It doesn't change your approach. But it avoids some extra work and extra memory usage:

• No unnecessary call to get_object_vars() followed by new \ReflectionClass($this))->getProperties() (8.4)
• Targeted key collection instead of multiple unset() calls

$keys = [];

if (version_compare(PHP_VERSION, '8.4.0') >= 0) {
    foreach ((new \ReflectionClass($this))->getProperties() as $property) {
        if ($property->hasHooks() === false) {
            $keys[] = $property->getName();
        }
    }
} else {
    $keys = array_keys(get_object_vars($this));
}

return $keys;

It's not a massive increase, but I believe if we can add a new feature in a more optimised way we should do so. Things add up. ✌️

[marius-ciclistu]

@NickSdot

Extracting that into a method + early returns would empower the developer that does not use hooks in model to avoid that reflection with an override and make the code cleaner:

 protected function getPropertiesForSerialization(): array
{
    if (version_compare(PHP_VERSION, '8.4.0') < 0) {
        return \array_keys(\get_object_vars($this));
    }

    $keys = [];

    foreach ((new \ReflectionClass($this))->getProperties() as $property) {
        if (!$property->hasHooks()) {
            $keys[] = $property->getName();
        }
    }

    return $keys;
}

[NickSdot]

@marius-ciclistu sure, adds another advantage in top. 👍 Let's see if @rafaelqueiroz wants to PR it otherwise you or I can make an attempt.

[rafaelqueiroz]

@NickSdot my work is done here, you can do and check if the maintainers will accept.