[11.x] Add $attributesToReloadOnSave property for Eloquent model

[hafezdivandari]

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:

Schema::create('products', function (Blueprint $table) {
    $table->id();
    $table->integer('price');
    $table->boolean('in_stock')->default(true);
});

With this model:

class Product extends Model
{
    protected $fillable = ['price', 'in_stock'];
+   protected $attributesToReloadOnSave = ['in_stock'];
}

Then:

$product = Product::create(['price' => 20]);

$product->price    // 20
$product->in_stock // was `null` but returns `true` after this PR

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:

Schema::create('products', function (Blueprint $table) {
    $table->id();
    $table->integer('price');
    $table->integer('tax')->virtualAs('price * 0.6');
    $table->integer('total')->storedAs('price * 1.6');
    $table->timestamps();
});

With this model:

class Product extends Model
{
    protected $fillable = ['price'];
+   protected $attributesToReloadOnSave = ['tax', 'total'];
}

Then:

$product = Product::create(['price' => 20]);

$product->price // 20
$product->tax   // was `null` but returns 12 after this PR
$product->total // was `null` but returns 32 after this PR

$product = Product::first();
$product->update(['price' => 10]);

$product->price // 10
$product->tax   // was 12 incorrectly but returns 6 after this PR
$product->total // was 32 incorrectly but returns 16 after this PR

[github-actions]

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.

[hafezdivandari]

Failing test is not related to this PR but this commit 94f0192

[taylorotwell]

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. 😅

[hafezdivandari]

@taylorotwell I totally understand, but this PR is not about Model::preventAccessingMissingAttributes at all, but using generated columns with eloquent model.

[driesvints]

What would be the implications of doing this actually inside the finishSave method instead of building a trait?

[hafezdivandari]

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.

[driesvints]

@taylorotwell I'm a bit torn on this one. It's apparently an actual bug in framework with virtualAs and storedA and indeed doesn't have anything to do with preventAccessingMissingAttributes. As you can see @hafezdivandari's example from above doesn't uses it.

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 preventAccessingMissingAttributes isn't involved?

[johanrosenson]

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:

• Possibility to declare which attributes are generated.
• Possibility to declare which attributes a generated column is dependant upon (this would also allow the refreshRawAttributes method to be smarter and only selected generated attributes that needs to be refreshed).

// maybe:
$generatedAttributes = [
   // 'attribute' => ['dependant columns'],
   'tax' => ['price'],
   'total' => ['price'],
];

• Possibility to declare when/if an auto refresh should happen, refreshing the model is usually only needed when some specific attributes have been updated.
  For instance in your example, the model actually only needs to be refreshed if price is changed, if the update doesn't involve that column, lets say you have name column that was updated, there would be no need to refresh tax and total.
  Maybe this could be achieved with a refreshIf() : bool method in your trait that can be overridden to make the refresh logic smarter per model (or completely disable it for a specific model)

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.

[donnysim]

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 HasGeneratedColumns that just refreshes the model automatically doesn't make it any better to use.

[Riley19280]

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 ->refresh() call is also needed to retrieve those values that were not set on the model at creation but were set in the database.

[hafezdivandari]

@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.

[ju5t]

Maybe this could be achieved with a refreshIf() : bool method in your trait that can be overridden to make the refresh logic smarter per model (or completely disable it for a specific model)

@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.

So in a way having a trait HasGeneratedColumns that just refreshes the model automatically doesn't make it any better to use.

@donnysim Can you explain? I think the problem you describe is unrelated to this one as you can use a $fillable.

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.

@Riley19280 100%.

[donnysim]

@ju5t fillable doesn't change anything as it's not related. For example you can have a virtual column full_name as CONCAT(name, " ", last_name), and a factory that sets it manually so you don't need to refresh the model and on save it will crash because it tries to save full_name to database. The same goes for business logic.

[driesvints]

Not sure if it's too generic but maybe just $reload?

[hafezdivandari]

Yeah, I wasn't sure about naming too, $reload feels like reloading relations? specially when we have load method on eloquent.

[driesvints]

That's true.

[hafezdivandari]

Currently, the latest approach has 3 problems in my mind:

1. Naming the attribute: $attributesToReloadOnSave any better suggestion?
2. Separating this for create/update: for generated columns, we have to reload the value on save (both create/update) but for columns with default, we just need to reload on create. We may:
   • Either have $attributesToReloadOnCreate and $attributesToReloadOnUpdate
   • Or something like $attributesToReloadOnSave = ['update' => [], 'create' => []]
3. Order of events and accessing the reloaded value on each event: currently the events are firing in the following order:
   • creating / updating
   • created / updated (instance doesn't have reloaded attributes yet)
   • retrieved (instance only has reloaded values)
   • saved (instance has access to all attributes)

[ziadoz]

This isn't applicable to virtual columns, but you can define property defaults using the $attributes property:

> class Foo extends \Illuminate\Database\Eloquent\Model
{
        protected $attributes = ['in_stock' => true];
}
> new Foo;
= Foo {#7024
    in_stock: true,
  }
> Foo::create(['price' => 100]);
= Foo {#7018
    in_stock: true,
    price: 100,
    id: 1,
  }