From bfe2ef974f425c95179373a1c7f79271ad103273 Mon Sep 17 00:00:00 2001 From: PowerGamer1 Date: Thu, 27 Jul 2023 18:15:03 +0300 Subject: [PATCH] Remove magic synchronization for relations (issue #19788). --- framework/CHANGELOG.md | 1 + framework/UPGRADE.md | 23 ++++++++++ framework/db/BaseActiveRecord.php | 59 ------------------------- tests/framework/db/ActiveRecordTest.php | 37 ++++++++-------- 4 files changed, 43 insertions(+), 77 deletions(-) diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index dc620a07486..03fc5c1cee8 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -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) diff --git a/framework/UPGRADE.md b/framework/UPGRADE.md index 7d2ca03eb95..04c542035e2 100644 --- a/framework/UPGRADE.md +++ b/framework/UPGRADE.md @@ -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 ----------------------- diff --git a/framework/db/BaseActiveRecord.php b/framework/db/BaseActiveRecord.php index 88fb2f11d7c..69faca84608 100644 --- a/framework/db/BaseActiveRecord.php +++ b/framework/db/BaseActiveRecord.php @@ -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); } @@ -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); @@ -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) { @@ -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; } @@ -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 . '".'); @@ -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; @@ -1196,8 +1174,6 @@ public static function populateRecord($record, $row) } } $record->_oldAttributes = $record->_attributes; - $record->_related = []; - $record->_relationsDependencies = []; } /** @@ -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 diff --git a/tests/framework/db/ActiveRecordTest.php b/tests/framework/db/ActiveRecordTest.php index f1370f47282..003813a6530 100644 --- a/tests/framework/db/ActiveRecordTest.php +++ b/tests/framework/db/ActiveRecordTest.php @@ -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; @@ -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); @@ -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, @@ -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'); @@ -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()