Feature: Eager loading in ActiveRecord::refresh()

[chriscpty]

As of current, ActiveRecord::refresh() and BaseActiveRecord::refresh() only refresh the record itself and lose all relations. This is the expected behaviour, but if you then need to iterate over nested relations afterwards, lazy loading them leads to an excessive amount of DB queries.

My proposal would thus be to add an optional parameter with to refresh:

public function refresh ($with = [])

The implementation in ActiveRecord uses the corresponding model's ActiveQuery anyway, so it might as well call ->with(...$with) on it in the process.

Q	A
Yii version	dev-master
PHP version	8.1
Operating system	Debian 12

I'd be happy to implement this, but want to be sure this makes sense and I haven't missed anything relevant before :)

[samdark]

I find it interesting. @yiisoft/yii2, @yiisoft/reviewers thoughts?

[bizley]

This would be great, but if we want to add this in Yii 2 we need to do this with method arguments discovery, like Symfony does sometimes (for BC).

[samdark]

What's "method arguments discovery"?

[bizley]

Sorry, my term :P I mean this func_get_args() inside to allow flexibility.

[chriscpty]

I don't think I see why that'd be necessary instead of just having the $with argument have a default value (which as far as I can tell is supported for all PHP versions yii supports)? Existing implementations that don't use this argument still work then, and while overrides may show a warning, they won't break either.

The only case of breaking I could see (that would happen afterwards though) is if a user overrides refresh and adds differently named arguments, then tries to pass with as a named argument - but I don't think that's a BC break.

[bizley]

Adding argument to refresh(), even optional one, will break user's apps that extend this method.

[chriscpty]

Just tested it and yeah, you are right - I misremembered whether PHP can deal with arguments being missing in overrides :D

func_get_args() it is then

[schmunk42]

Should go into 2.2, if implemented.

[samdark]

2.2 may break compatibility.

[lubosdz]

Perhaps simpler variant without BC break, but somewhat less flexible .. ?

public function refresh ($relations = true)

true = default, no BC break
false = don't clear relations

[chriscpty]

I don't think that solves the BC break? The compatibility issue is that overrides of this function that don't have the additional optional parameter break.

[mtangoo]

So I see two options: 1. Add it in 2.0.x the way @bizley suggested or only implement in 2.2.x where such BC breaking is allowable. @chriscpty are you open to do it Bizley way?

[chriscpty]

I'd say I try to do both, one without the BC break and one with, and do separate pull requests? Then one can be merged into 2.0.x (so we can use it already) and one can be merged into 2.2.x (for a cleaner solution)?

[rob006]

Do we really need $with param? The fact that refresh() do not respect settings from with() looks more like a bug to me and IMO it should respect it by default.

[chriscpty]

I don't think I understand? Do you mean that refresh should eagerly load all relations that were loaded before calling it (for example through with() on the query the record was loaded with)? in which case, that's worth a thought too, but I'd personally rather have the control to decide which relations I want to load when refreshing

[rob006]

Yes, IMO these 2 samples should work the same:

$model = MyModel::find()->with($withSettings)->where(['id' => $id])->one();
$model->name = 'test';
$model->save();
$model->refresh();

$model = MyModel::find()->with($withSettings)->where(['id' => $id])->one();
$model->name = 'test';
$model->save();
$model = MyModel::find()->with($withSettings)->where(['id' => $id])->one();

[chriscpty]

That could also be an alternative, but i'd rather have control over which relations are loaded in the refresh independently of the relations loaded before.

Otherwise, you're forced to always reload relations even if you don't need them anymore afterwards

[rob006]

We could add new method refreshWith($relations). Then you could overwrite with() settings using $refresh param (and pass empty array if you don't want to load relations).

[mtangoo]

We could add new method refreshWith($relations). Then you could overwrite with() settings using $refresh param (and pass empty array if you don't want to load relations).

That is another good option with total freedom. But if current method can be extended without affecting existing code (extending the classes and what not) then I would go for that first.

What you guys think? @samdark @lubosdz @schmunk42 @bizley

[lubosdz]

Adding new method seems cleaner, as it is usual way of extending core functionality with @since docvar.
Args discovery might not be quite cheap & simple.