ActiveRecord::refresh() should not unset all relations but only the ones whose corresponding foreign key has changed

[PowerGamer1]

The #13618 introduced "magic" functionality that makes it look like changing foreign key attribute of a model also changes loaded relation model dependent on this foreign key. This was achieved by simply unsetting the affected relation (whose foreign key has changed).

This was actually a BC breaking change later documented in UPGRADE.md as:

Active Record relations are now being reset when corresponding key fields are changed.

Unfortunately, the original implementation didn't follow its own logic thoroughly and left the ActiveRecord::refresh() implemented incorrectly - unsetting ALL relations regardless of changes to the foreign keys. And so, if relations loaded before refresh() are used again after refresh() additional queries against database are executed to fetch the same data recently unset by refresh().

What steps will reproduce the problem?

$project = Project::findOne(123);
$project->manager; // Load relation.
$project->manager_id = 456; // Change relation foreign key.
var_dump($project->isRelationPopulated('manager')); // false - OK: foreign key changed, previously loaded relation is considered outdated and is unset.

$project = Project::findOne(123);
$project->manager;
$project->refresh(); // Assuming manager_id has not changed.
var_dump($project->isRelationPopulated('manager')); // false - BUG (expected true): foreign key is the same but relation is unset.
$project->manager; // If the relation is used later, another query must be performed against DB that loads the same data (unset by refresh()).

What is the expected result?

false
true

What do you get instead?

false
false

Additional info

Q	A
Yii version	2.0.48
PHP version	irrelevant
Operating system	irrelevant

[samdark]

Yes. Ideally it should behave like you've described. Do you have time for fixing it?

[PowerGamer1]

Yes. Ideally it should behave like you've described. Do you have time for fixing it?

This particular issue causes a bunch of problems in my production code, I will submit PR shortly.

[rhertogh]

@PowerGamer1 Just to be sure, your example states var_dump($project->isRelationPopulated('project')); should that have been var_dump($project->isRelationPopulated('manager'));?

[PowerGamer1]

@PowerGamer1 Just to be sure, your example states var_dump($project->isRelationPopulated('project')); should that have been var_dump($project->isRelationPopulated('manager'));?

Yes, you are correct, I edited my original post and fixed it.

[rhertogh]

@samdark

Yes. Ideally it should behave like you've described.

Are you sure about that? The BaseActiveRecord::refresh() description states "Repopulates this active record with the latest data".
To me that reads as that the entire state of the ActiveRecord should be invalidated and synchronized with the database.
Since we don't know the state of the relations it makes sense to me that they are cleared.

[PowerGamer1]

The BaseActiveRecord::refresh() description states "Repopulates this active record with the latest data". To me that reads as that the entire state of the ActiveRecord should be invalidated and synchronized with the database.

Your quote from docs mentions only "... THIS active record ..." and the relations are DIFFERENT active records. But that is irrelevant - see below.

Since we don't know the state of the relations it makes sense to me that they are cleared.

You don't know but the user does. So why are you screwing the user by force-clearing his relations instead of letting him handle the clearing himself if needed (see example 7 from #19788)?

P.S. The problem in this issue is a part of much bigger problem in #19788 and must not be considered in separation.