Skip to content

[5.4] Use setRawAttributes in Pivot constructor to avoid casting twice #18149

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

Closed
wants to merge 1 commit into from
Closed

Conversation

jrhenderson1988
Copy link
Contributor

@jrhenderson1988 jrhenderson1988 commented Feb 28, 2017

At present, the Pivot constructor uses forceFill to set the attributes of the pivot model, which uses setAttribute to set each property and ends up casting the data provided as a string given by the database to the relevant type. Most notably, for attributes that are set to be cast as a JSON castable type (array, object etc.) this results in a JSON encoded string from the database being JSON encoded again when it's being set in the Pivot model. Therefore, accessing such an attribute results in a JSON encoded string being returned rather than an array or stdClass object as expected.

This pull request changes forceFill to setRawAttributes so that casting is not carried out when setting the Pivot attributes.

#18135

Note: This PR is causing 2 tests to fail. One which ensures mutators are called from the constructor of Pivot and another which ensures that a dummy date passed in has been mutated. My suggestion intentionally skips the mutators by using setRawAttributes, similar to when a Model is created, therefore I'm not sure what to do regarding this.

@jrhenderson1988 jrhenderson1988 changed the title Set raw attributes to avoid casting twice [5.4] Set raw attributes to avoid casting twice Feb 28, 2017
@jrhenderson1988 jrhenderson1988 changed the title [5.4] Set raw attributes to avoid casting twice [5.4] Use setRawAttributes in Pivot constructor to avoid casting twice Feb 28, 2017
@taylorotwell
Copy link
Member

I believe this was already fixed but not tag. Regardless, this would be the wrong change to fix it as it would break other scenarios.

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