Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove magic synchronization for relations (issue #19788). #19918

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove magic synchronization for relations (issue #19788). #19918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions framework/CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading