[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[bot]]

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 94f0192f2f515bf06795c635c53137ac15867eec

[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]

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

[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,
  }

[ju5t]

@hafezdivandari this wouldn't be my preferred solution. It's too much 'boilerplate' for me.

[hafezdivandari]

I think the solution is to implement RETURNING SQL clause on the query builder, all DBs except MySQL support it. What workaround can we use on MySQL?

DB	Support version	Docs
SQLite	3.35.0+	https://www.sqlite.org/lang_returning.html
MariaDB	10.5.0+	https://mariadb.com/kb/en/insertreturning
PostgreSQL	8.4.0+	https://www.postgresql.org/docs/current/dml-returning.html
SQL Server	2016+	https://learn.microsoft.com/en-us/sql/t-sql/queries/output-clause-transact-sql
MySQL	N/A	N/A

[macropay-solutions]

We solved this issue more than 3 years ago with: https://github.com/macropay-solutions/laravel-crud-wizard-free/blob/e6a9bb60ba28e644bb1cf5c5979e0c6b267fca20/src/Providers/CrudProvider.php#L24

It does not cause problems.

[driesvints]

@hafezdivandari any news on this PR? Otherwise it's probably best to let this one go.

[hafezdivandari]

I'll close this one for now as I don't like the current solution, as I said in this comment, the better solution may be to implement returning clause when inserting/updating.