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

Implement SerializesCastableAttributes in StateCaster #251

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Implement SerializesCastableAttributes in StateCaster #251

merged 2 commits into from
Mar 1, 2024

Conversation

piotrjoniec
Copy link
Contributor

Currently, calling toArray on models will not serialize model states to their underlying values. This doesn't seem like correct behavior, as Laravel tries to serialize all objects to their primitive values whenever possible (for example Carbon/DateTime -> string).

This PR implements SerializesCastableAttributes in StateCaster to support this serialization.

@freekmurze
Copy link
Member

The tests seem to be failing, could you take a look?

@piotrjoniec
Copy link
Contributor Author

@freekmurze Fixed. All versions are passing now for sure.

@freekmurze freekmurze merged commit d3f7be0 into spatie:main Mar 1, 2024
53 checks passed
@freekmurze
Copy link
Member

Thank you!

@mikerockett
Copy link

@freekmurze This has, unfortunately, broken my application, which uses and relies on a jsonSerialize method on the root state class. This method returns an array that is then casted to JSON for my SPA to consume. The change in this PR forces the underlying value to be passed, which isn't desired (frontend relies on properties sent in the object, such as a friendly display name, a colour, etc). I have now locked my app to ~2.6.0. If this PR isn't reverted, what can I do to cleanly ensure my loaded state is passed as an object to my SPA (outside of Eloquent resources or Laravel Data, neither of which are being used in my current context)?

@piotrjoniec
Copy link
Contributor Author

@mikerockett That's strange, because jsonSerialize hasn't changed at all:

https://github.com/spatie/laravel-model-states/blame/main/src/State.php#L259C1-L263C6

But if you want the whole class to be serialized to JSON, wouldn't overriding jsonSerialize like this work?:

    public function jsonSerialize(): mixed
    {
        return $this;
    }

@mikerockett
Copy link

@piotrjoniec – my state classes have been implementing that method all along. I think the state caster is taking priority, which is why it's breaking my frontend. I have also played with Arrayable + toArray and Jsonable + toJson to try my luck, but nothing took effect.

@piotrjoniec
Copy link
Contributor Author

@mikerockett I guess a solution would be to adjust the StateCaster like this:

    public function serialize($model, string $key, $value, array $attributes)
    {
-        return $value instanceof State ? $value->getValue() : $value;
+        return $value instanceof State ? $value->jsonSerialize() : $value;
    }

This way calling toArray and toJson on models would produce consistent results. And overriding jsonSerialize would work as well.

If this makes sense I will submit a PR for this.

@mikerockett
Copy link

@piotrjoniec Thanks, I think that’s going to be the best approach. If someone wants different (inconsistent?) behaviour, I guess they could use their own state caster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants