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

[11.x] Don’t serialise virtual properties in SerializesModels #53898

Draft
wants to merge 1 commit into
base: 11.x
Choose a base branch
from

Conversation

remipelhate
Copy link

With the introduction of property hooks in php 8.4, developers now have the ability to create virtual properties. As these properties are in fact virtual, I believe they may be excluded from being serialised in Illuminate\Queue\SerializesModels.

With the current implementation, read-only virtual properties will fail when trying to set their value when unserialising an instance. Excluding virtual properties from serialisation should solve this.

Note

I couldn't find test coverage for the specific cases defined in SerializesModels (such as excluding static or uninitialised properties), so I'm not 100% sure how this would be tested. Any feedback or guidance is welcome.

Backwards compatibility

As virtual properties were introduced in php 8.4 and Laravel currently supports anything above 8.2, this implementation first checks whether the isVirtual() method exists on ReflectionProperty. This should ensure these changes don't break Illuminate\Queue\SerializesModels on earlier php versions.

@taylorotwell
Copy link
Member

Could we still serialize them, just with their underlying value? Is there a way to access and set that via reflection?

@taylorotwell taylorotwell marked this pull request as draft December 14, 2024 21:23
@remipelhate
Copy link
Author

Could we still serialize them, just with their underlying value? Is there a way to access and set that via reflection?

They can be serialised with the underlying value, not sure about setting them through reflection. I can investigate it though.

However, I wonder what the value would be to serialise them as I believe they can always be reconstructed from other non-virtual properties (unless I'm unaware of use-cases where this isn't true). So to me, it feels like these values would be redundant in the serialised payload.

@remipelhate
Copy link
Author

@taylorotwell looks to me like there isn't an obvious way to set read-only virtual properties via reflection. I took the example from the php docs (which funnily enough throws an error as hooked properties cannot be readonly🙃) and applied a simplified version of the serialization process done in SerializesModels. Reflection actually fails these properties being readonly: https://3v4l.org/LRdao#v8.4.1

But the again, I don't think there is a need to serialise these virtual properties.

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.

2 participants