-
-
Notifications
You must be signed in to change notification settings - Fork 717
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
Fix cast to array bug #681
Conversation
See #680 for more info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems good to me - will check it in depth tomorrow
@@ -38,6 +38,7 @@ | |||
}, | |||
"require-dev": { | |||
"ext-json": "*", | |||
"orchestra/testbench": "^5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still support L6 - so we also need Orchestra 4.
@@ -133,8 +133,9 @@ public static function logChanges(Model $model): array | |||
static::getModelAttributeJsonValue($model, $attribute) | |||
); | |||
} else { | |||
$changes[$attribute] = $model->getAttribute($attribute); | |||
|
|||
$changes[$attribute] = $model->hasCast($attribute, 'array') && is_array($model->attributes[$attribute]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The json
cast is the same. And does this happen with collections
/objects
too? 🤔
At the end I'm still not sure what we are doing wrong? 🤔 |
If this helps, I initially thought it was an issue in the framework, but it doesn't seem to be: laravel/framework#31790 |
We also had this issue in our projects. I did some digging. I think I found something. I'm not sure. Here it's calling I'm not sure this is the cause, but maybe you can investigate more. |
@canvural Interesting. Maybe it is this: https://laravel.com/docs/7.x/upgrade#factory-types
|
Yeah, that looks like the reason. So, maybe this issue can be solved by using |
Thanks @kapersoft for your work here! 🎉 I was able to use everything and have extended your PR/branch. |
Fixes #680; test for the issue is included.
Let me know if you have any questions or remarks.