Skip to content

Commit

Permalink
Remove magic synchronization for relations (issue yiisoft#19788).
Browse files Browse the repository at this point in the history
  • Loading branch information
@PowerGamer1
PowerGamer1 committed Jul 27, 2023
1 parent c8c0ea9 commit bfe2ef9
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 77 deletions.
1 change: 1 addition & 0 deletions framework/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Yii Framework 2 Change Log
2.0.49 under development
------------------------

- Chg #19788: Removed all (often non-functional) attempts of Yii2 to automatically synchronize ActiveRecord relations with corresponding foreign key values. The new guarantee provided by Yii2 is: once set ActiveRecord relations are never automatically or silently changed/unset by the engine (PowerGamer1)
- Bug #19899: Fixed `GridView` in some cases calling `Model::generateAttributeLabel()` to generate label values that are never used (PowerGamer1)
- Bug #9899: Fix caching a MSSQL query with BLOB data type (terabytesoftw)
- Bug #16208: Fix `yii\log\FileTarget` to not export empty messages (terabytesoftw)
Expand Down
23 changes: 23 additions & 0 deletions framework/UPGRADE.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,29 @@ Upgrade from Yii 2.0.48
* The function signature for `yii\console\Controller::select()` and `yii\helpers\BaseConsole::select()` have changed.
They now have an additional `$default = null` parameter. In case those methods are overwritten you will need to
update your child classes accordingly.
* The engine no longer attempts to provide an automatic synchronization between ActiveRecord relations and corresponding foreign keys.
Such synchronization never worked in many cases, came with ActiveRecord performance and memory costs and in some cases is impossible to achieve (see https://github.com/yiisoft/yii2/issues/19788 for details).
The new guarantee provided by Yii2 is: once set ActiveRecord relations are never automatically or silently changed/unset by the engine.
All places in existing code that use already loaded relation after it is expected to change need to manually unset such relation. For example, in the code below:
```php
$project = Project::findOne(123);
$oldManager = $project->manager;
$project->load(Yii::$app->getRequest()->post()); // $project->manager_id may change here.
$project->update();
$newManager = $project->manager;
// Notify $oldManager and $newManager about the reassignment by email.
```
the access to `$project->manager` after update should be preceded by unsetting that relation:
```PHP
// ... (same as above).
$project->update();
unset($project->manager);
$newManager = $project->manager;
// Notify $oldManager and $newManager about the reassignment by email.
```
Another notable example is using `ActiveRecord::refresh()`. If the refreshed model had relations loaded before the call to `refresh()`
and these relations are expected to change, unset them explicitly with `unset()` before using again.


Upgrade from Yii 2.0.46
-----------------------
Expand Down
59 changes: 0 additions & 59 deletions framework/db/BaseActiveRecord.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,6 @@ public function __get($name)
}
$value = parent::__get($name);
if ($value instanceof ActiveQueryInterface) {
$this->setRelationDependencies($name, $value);
return $this->_related[$name] = $value->findFor($name, $this);
}

Expand All @@ -311,12 +310,6 @@ public function __get($name)
public function __set($name, $value)
{
if ($this->hasAttribute($name)) {
if (
!empty($this->_relationsDependencies[$name])
&& (!array_key_exists($name, $this->_attributes) || $this->_attributes[$name] !== $value)
) {
$this->resetDependentRelations($name);
}
$this->_attributes[$name] = $value;
} else {
parent::__set($name, $value);
Expand Down Expand Up @@ -350,9 +343,6 @@ public function __unset($name)
{
if ($this->hasAttribute($name)) {
unset($this->_attributes[$name]);
if (!empty($this->_relationsDependencies[$name])) {
$this->resetDependentRelations($name);
}
} elseif (array_key_exists($name, $this->_related)) {
unset($this->_related[$name]);
} elseif ($this->getRelation($name, false) === null) {
Expand Down Expand Up @@ -460,10 +450,6 @@ protected function createRelationQuery($class, $link, $multiple)
*/
public function populateRelation($name, $records)
{
foreach ($this->_relationsDependencies as &$relationNames) {
unset($relationNames[$name]);
}

$this->_related[$name] = $records;
}

Expand Down Expand Up @@ -521,12 +507,6 @@ public function getAttribute($name)
public function setAttribute($name, $value)
{
if ($this->hasAttribute($name)) {
if (
!empty($this->_relationsDependencies[$name])
&& (!array_key_exists($name, $this->_attributes) || $this->_attributes[$name] !== $value)
) {
$this->resetDependentRelations($name);
}
$this->_attributes[$name] = $value;
} else {
throw new InvalidArgumentException(get_class($this) . ' has no attribute named "' . $name . '".');
Expand Down Expand Up @@ -1081,8 +1061,6 @@ protected function refreshInternal($record)
$this->_attributes[$name] = isset($record->_attributes[$name]) ? $record->_attributes[$name] : null;
}
$this->_oldAttributes = $record->_oldAttributes;
$this->_related = [];
$this->_relationsDependencies = [];
$this->afterRefresh();

return true;
Expand Down Expand Up @@ -1196,8 +1174,6 @@ public static function populateRecord($record, $row)
}
}
$record->_oldAttributes = $record->_attributes;
$record->_related = [];
$record->_relationsDependencies = [];
}

/**
Expand Down Expand Up @@ -1731,41 +1707,6 @@ public function offsetUnset($offset)
}
}

/**
* Resets dependent related models checking if their links contain specific attribute.
* @param string $attribute The changed attribute name.
*/
private function resetDependentRelations($attribute)
{
foreach ($this->_relationsDependencies[$attribute] as $relation) {
unset($this->_related[$relation]);
}
unset($this->_relationsDependencies[$attribute]);
}

/**
* Sets relation dependencies for a property
* @param string $name property name
* @param ActiveQueryInterface $relation relation instance
* @param string|null $viaRelationName intermediate relation
*/
private function setRelationDependencies($name, $relation, $viaRelationName = null)
{
if (empty($relation->via) && $relation->link) {
foreach ($relation->link as $attribute) {
$this->_relationsDependencies[$attribute][$name] = $name;
if ($viaRelationName !== null) {
$this->_relationsDependencies[$attribute][] = $viaRelationName;
}
}
} elseif ($relation->via instanceof ActiveQueryInterface) {
$this->setRelationDependencies($name, $relation->via);
} elseif (is_array($relation->via)) {
list($viaRelationName, $viaQuery) = $relation->via;
$this->setRelationDependencies($name, $viaQuery, $viaRelationName);
}
}

/**
* @param mixed $newValue
* @param mixed $oldValue
Expand Down
37 changes: 19 additions & 18 deletions tests/framework/db/ActiveRecordTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1165,7 +1165,7 @@ public function testRelationWhereParams($orderTableName, $orderItemTableName)
OrderItem::$tableName = null;
}

public function testOutdatedRelationsAreResetForNewRecords()
public function testOutdatedRelationsAreNotResetForNewRecords()
{
$orderItem = new OrderItem();
$orderItem->order_id = 1;
Expand All @@ -1176,17 +1176,17 @@ public function testOutdatedRelationsAreResetForNewRecords()
// Test `__set()`.
$orderItem->order_id = 2;
$orderItem->item_id = 1;
$this->assertEquals(2, $orderItem->order->id);
$this->assertEquals(1, $orderItem->item->id);
$this->assertEquals(1, $orderItem->order->id);
$this->assertEquals(3, $orderItem->item->id);

// Test `setAttribute()`.
$orderItem->setAttribute('order_id', 2);
$orderItem->setAttribute('item_id', 2);
$this->assertEquals(2, $orderItem->order->id);
$this->assertEquals(2, $orderItem->item->id);
$this->assertEquals(1, $orderItem->order->id);
$this->assertEquals(3, $orderItem->item->id);
}

public function testOutdatedRelationsAreResetForExistingRecords()
public function testOutdatedRelationsAreNotResetForExistingRecords()
{
$orderItem = OrderItem::findOne(1);
$this->assertEquals(1, $orderItem->order->id);
Expand All @@ -1195,17 +1195,17 @@ public function testOutdatedRelationsAreResetForExistingRecords()
// Test `__set()`.
$orderItem->order_id = 2;
$orderItem->item_id = 1;
$this->assertEquals(2, $orderItem->order->id);
$this->assertEquals(1, $orderItem->order->id);
$this->assertEquals(1, $orderItem->item->id);

// Test `setAttribute()`.
$orderItem->setAttribute('order_id', 3);
$orderItem->setAttribute('item_id', 1);
$this->assertEquals(3, $orderItem->order->id);
$this->assertEquals(1, $orderItem->order->id);
$this->assertEquals(1, $orderItem->item->id);
}

public function testOutdatedCompositeKeyRelationsAreReset()
public function testOutdatedCompositeKeyRelationsAreNotReset()
{
$dossier = Dossier::findOne([
'department_id' => 1,
Expand All @@ -1214,26 +1214,26 @@ public function testOutdatedCompositeKeyRelationsAreReset()
$this->assertEquals('John Doe', $dossier->employee->fullName);

$dossier->department_id = 2;
$this->assertEquals('Ann Smith', $dossier->employee->fullName);
$this->assertEquals('John Doe', $dossier->employee->fullName);

$dossier->employee_id = 2;
$this->assertEquals('Will Smith', $dossier->employee->fullName);
$this->assertEquals('John Doe', $dossier->employee->fullName);

unset($dossier->employee_id);
$this->assertNull($dossier->employee);
$this->assertEquals('John Doe', $dossier->employee->fullName);

$dossier = new Dossier();
$this->assertNull($dossier->employee);

$dossier->employee_id = 1;
$dossier->department_id = 2;
$this->assertEquals('Ann Smith', $dossier->employee->fullName);
$this->assertNull($dossier->employee);

$dossier->employee_id = 2;
$this->assertEquals('Will Smith', $dossier->employee->fullName);
$this->assertNull($dossier->employee);
}

public function testOutdatedViaTableRelationsAreReset()
public function testOutdatedViaTableRelationsAreNotReset()
{
$order = Order::findOne(1);
$orderItemIds = ArrayHelper::getColumn($order->items, 'id');
Expand All @@ -1243,17 +1243,18 @@ public function testOutdatedViaTableRelationsAreReset()
$order->id = 2;
sort($orderItemIds);
$orderItemIds = ArrayHelper::getColumn($order->items, 'id');
$this->assertSame([3, 4, 5], $orderItemIds);
$this->assertSame([1, 2], $orderItemIds);

unset($order->id);
$this->assertSame([], $order->items);
$orderItemIds = ArrayHelper::getColumn($order->items, 'id');
$this->assertSame([1, 2], $orderItemIds);

$order = new Order();
$this->assertSame([], $order->items);

$order->id = 3;
$orderItemIds = ArrayHelper::getColumn($order->items, 'id');
$this->assertSame([2], $orderItemIds);
$this->assertSame([], $orderItemIds);
}

public function testAlias()
Expand Down

0 comments on commit bfe2ef9

Please sign in to comment.