-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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] Add $attributesToReloadOnSave
property for Eloquent model
#51014
base: 11.x
Are you sure you want to change the base?
[11.x] Add $attributesToReloadOnSave
property for Eloquent model
#51014
Conversation
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
Failing test is not related to this PR but this commit 94f0192 |
I'll be honest, I'm not sure how much longer I want to keep changing the framework to support a feature I don't like (barfing on "missing attributes"). The feature hasn't been documented in a while, and I would rather just tell people not to use it at all. It's not a great feature. It breaks packages. It breaks core framework features, and I'm getting weary of working around it with more and more edge-case PRs. 😅 |
@taylorotwell I totally understand, but this PR is not about |
// after the model instance is saved just before firing "saved" event. | ||
$this->refreshRawAttributes(); | ||
|
||
parent::finishSave($options); |
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.
What would be the implications of doing this actually inside the finishSave
method instead of building a trait?
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.
This actually runs a query to retrive and reset the attributes' values. It's not always needed but only when the model has generated columns.
@taylorotwell I'm a bit torn on this one. It's apparently an actual bug in framework with I'm not sure the trait is the correct solution. But at the same time I cannot think about anything else other than putting a note in the docs for Eloquent. Can you maybe give your opinion again now that you know |
We use generated columns in our company, but I don't like the idea that the finishSave() method automatically retrieves a fresh instance on save, this would risk creating a N+1 issue if saving multiple models in a loop. I think that if you are using generated/virtual columns you need to be aware of the drawbacks and implement reloading manually in those places you know that you need a fresh instance with updated information. Doing a forced refresh on every save is not a good solution, sure it will solve some use cases, but I think it will create more issues in any case where you don't need the fresh data anyway. It also needs to take into account any loaded relationships and make sure those are not discarded when reloading the attributes - it doesn't look like you are discarding them, but there have been so many eloquent changes lately that have broken unrelated things, a test for this would be awesome (I think). If an automatic solution is the way to go, then I think this trait needs to be smarter in the way it handles generated columns, some ideas:
// maybe:
$generatedAttributes = [
// 'attribute' => ['dependant columns'],
'tax' => ['price'],
'total' => ['price'],
];
I don't know if my ideas here are the best solutions, but I think they are better than a one size fits all solution that is proposed in this PR. |
I'm not a fan of this either. Overall if you have generated columns and you want to use them after saving you usual just refresh the model in that one or two spots it actually matters. Currently having actual generated attributes in Eloquent leads you to tracking what those columns are and excluding them from saving because in some cases you unset/null/update them php side on value change etc. and it crashes by wanting to save them to database because they're "dirty". So in a way having a trait |
Not sure where this fits in the conversation, but I would also like to point out that this could also be used in places where columns have default values, which is a much more common use case. Currently a |
@Riley19280 That's exactly why I marked this as draft, I was thinking about better solutions to cover both situations. I'll update this soon. |
@johanrosenson I agree that having some additional logic may be useful to customise how it works, but I also feel that it shouldn't require too much 'set up' to get this working. I think all it should take is adding a trait, or perhaps a contract, if that's possible without unwanted side-effects as N+1. I don't know enough on the internals of eloquent to comment on that.
@donnysim Can you explain? I think the problem you describe is unrelated to this one as you can use a
@Riley19280 100%. |
@ju5t fillable doesn't change anything as it's not related. For example you can have a virtual column |
* | ||
* @var array<int, string> | ||
*/ | ||
protected $attributesToReloadOnSave = []; |
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.
Not sure if it's too generic but maybe just $reload
?
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.
Yeah, I wasn't sure about naming too, $reload
feels like reloading relations? specially when we have load
method on eloquent.
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.
That's true.
Currently, the latest approach has 3 problems in my mind:
|
HasGeneratedColumns
trait for Eloquent model$attributesToReloadOnSave
property for Eloquent model
This isn't applicable to virtual columns, but you can define property defaults using the
|
@hafezdivandari this wouldn't be my preferred solution. It's too much 'boilerplate' for me. |
I think the solution is to implement
|
WIP
This PR adds
$attributesToReloadOnSave
property on the eloquent model.Columns with default value
If an Eloquent model has columns with default value on its table, the value of these attributes will be
null
after creating a new model instance:Example
Let's assume we have the following table:
With this model:
class Product extends Model { protected $fillable = ['price', 'in_stock']; + protected $attributesToReloadOnSave = ['in_stock']; }
Then:
Generated Columns
If an Eloquent model has generated columns (e.g. stored, virtual, or computed) on its table, the value of these generated attributes are
null
when creating a new model instance and would be incorrect when updating the model instance.Example
Let's assume we have the following table:
With this model:
class Product extends Model { protected $fillable = ['price']; + protected $attributesToReloadOnSave = ['tax', 'total']; }
Then: