From 96c153f110c42af6df61b3f38ba0e238c97e9c5c Mon Sep 17 00:00:00 2001 From: Alban Jubert Date: Thu, 12 Oct 2017 16:50:31 +0200 Subject: [PATCH] * Small refactoring. * Changed the way exceptions are thrown and handled during afterSave event (add error to relation attribute of the owner model, forced rollback if relevant) --- CHANGELOG.md | 2 +- README.md | 7 ++- src/SaveRelationsBehavior.php | 97 ++++++++++++++++++++++++------------- tests/SaveRelationsBehaviorTest.php | 25 ++++++++-- tests/models/Project.php | 4 +- 5 files changed, 92 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f37a5f..55e30da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ - False positive testLoadRelationsShouldSucceed test case ### Changed -- afterSave throw exception if a related record fail to be saved +- afterSave throw exception if a related record fail to be saved. In that case, a database rollback is triggered (when relevant) and an error is attached to the according relation attribute - related record are now correctly updated based on there primary key (Thanks to @DD174) ## [1.2.0] diff --git a/README.md b/README.md index a13c8e2..56bd593 100644 --- a/README.md +++ b/README.md @@ -150,9 +150,14 @@ For instance, in the following configuration, the `links ` related records will } ... ``` -> **Tips :** +> **Tips:** > For relations not involving a junction table by using the `via()` or `viaTable()` methods, you should remove the attributes pointing to the owner model from the 'required' validation rules to be able to pass the validations. +> **Note:** +> If an error occurs for any reason during the saving process of related records in the afterSave event, a `yii\db\Exception` will be thrown on the first occurring error. +> An error message will be attached to the relation attribute of the owner model. +> In order to be able to handle these cases in a user-friendly way, one will have to catch `yii\db\Exception` exceptions. + Populate the model and its relations with input data ---------------------------------------------------- This behavior adds a convenient method to load relations models attributes in the same way that the load() method does. diff --git a/src/SaveRelationsBehavior.php b/src/SaveRelationsBehavior.php index 0b51042..fc22106 100644 --- a/src/SaveRelationsBehavior.php +++ b/src/SaveRelationsBehavior.php @@ -10,10 +10,10 @@ use yii\base\ModelEvent; use yii\base\UnknownPropertyException; use yii\db\ActiveQueryInterface; use yii\db\BaseActiveRecord; +use Yii\db\Exception as DbException; use yii\db\Transaction; use yii\helpers\ArrayHelper; use yii\helpers\Inflector; -use yii\helpers\VarDumper; /** * This Active Record Behavior allows to validate and save the Model relations when the save() method is invoked. @@ -179,7 +179,7 @@ class SaveRelationsBehavior extends Behavior $fks = []; // search PK - foreach($modelClass::primaryKey() as $modelAttribute) { + foreach ($modelClass::primaryKey() as $modelAttribute) { if (array_key_exists($modelAttribute, $data) && !empty($data[$modelAttribute])) { $fks[$modelAttribute] = $data[$modelAttribute]; } @@ -287,10 +287,7 @@ class SaveRelationsBehavior extends Behavior } } catch (Exception $e) { Yii::warning(get_class($e) . " was thrown during the saving of related records : " . $e->getMessage(), __METHOD__); - if (($this->_transaction instanceof Transaction) && $this->_transaction->isActive) { - $this->_transaction->rollBack(); // If anything goes wrong, transaction will be rolled back - Yii::info("Rolling back", __METHOD__); - } + $this->_rollback(); $event->isValid = false; // Stop saving, something went wrong return false; } @@ -330,12 +327,7 @@ class SaveRelationsBehavior extends Behavior } Yii::trace("Validating {$pettyRelationName} relation model using " . $relationModel->scenario . " scenario", __METHOD__); if (!$relationModel->validate()) { - foreach ($relationModel->errors as $attributeErrors) { - foreach ($attributeErrors as $error) { - $model->addError($relationName, "{$pettyRelationName}: {$error}"); - } - $event->isValid = false; - } + $this->_addError($relationModel, $model, $relationName, $pettyRelationName); } } } @@ -352,6 +344,8 @@ class SaveRelationsBehavior extends Behavior $model = $this->owner; $this->_relationsSaveStarted = true; try { + + foreach ($this->_relations as $relationName) { if (array_key_exists($relationName, $this->_oldRelationValue)) { // Relation was not set, do nothing... Yii::trace("Linking {$relationName} relation", __METHOD__); @@ -361,11 +355,15 @@ class SaveRelationsBehavior extends Behavior $existingRecords = []; /** @var BaseActiveRecord $relationModel */ - foreach ($model->{$relationName} as $relationModel) { + foreach ($model->{$relationName} as $i => $relationModel) { if ($relationModel->isNewRecord) { if ($relation->via !== null) { - if (!$relationModel->save()) { - throw new Exception('Related model ' . $relationName . ' could not be saved (' . VarDumper::dumpAsString($relationModel->getErrors()) . ')'); + if ($relationModel->validate()) { + $relationModel->save(); + } else { + $pettyRelationName = Inflector::camel2words($relationName, true) . " #{$i}"; + $this->_addError($relationModel, $model, $relationName, $pettyRelationName); + throw new DbException("Related record {$pettyRelationName} could not be saved."); } } $model->link($relationName, $relationModel); @@ -373,8 +371,12 @@ class SaveRelationsBehavior extends Behavior $existingRecords[] = $relationModel; } if (count($relationModel->dirtyAttributes)) { - if (!$relationModel->save()) { - throw new Exception('Related model ' . $relationName . ' could not be saved (' . VarDumper::dumpAsString($relationModel->getErrors()) . ')'); + if ($relationModel->validate()) { + $relationModel->save(); + } else { + $pettyRelationName = Inflector::camel2words($relationName, true); + $this->_addError($relationModel, $model, $relationName, $pettyRelationName); + throw new DbException("Related record {$pettyRelationName} could not be saved."); } } } @@ -409,11 +411,11 @@ class SaveRelationsBehavior extends Behavior } } } catch (Exception $e) { - Yii::warning(get_class($e) . " was thrown during the saving of related records : " . $e->getMessage(), __METHOD__); - if (($this->_transaction instanceof Transaction) && $this->_transaction->isActive) { - $this->_transaction->rollBack(); // If anything goes wrong, transaction will be rolled back - Yii::info("Rolling back", __METHOD__); - } + $this->_rollback(); + /*** + * Sadly mandatory because the error occurred during afterSave event + * and we don't want the user/developper not to be aware of the issue. + ***/ throw $e; } $model->refresh(); @@ -425,6 +427,26 @@ class SaveRelationsBehavior extends Behavior } /** + * Populates relations with input data + * @param array $data + */ + public function loadRelations($data) + { + /** @var BaseActiveRecord $model */ + $model = $this->owner; + foreach ($this->_relations as $relationName) { + $relation = $model->getRelation($relationName); + $modelClass = $relation->modelClass; + /** @var BaseActiveRecord $relationalModel */ + $relationalModel = new $modelClass; + $formName = $relationalModel->formName(); + if (array_key_exists($formName, $data)) { + $model->{$relationName} = $data[$formName]; + } + } + } + + /** * Compute the difference between two set of records using primary keys "tokens" * @param BaseActiveRecord[] $initialRelations * @param BaseActiveRecord[] $updatedRelations @@ -446,22 +468,27 @@ class SaveRelationsBehavior extends Behavior } /** - * Populates relations with input data - * @param array $data + * Attach errors to owner relational attributes + * @param $relationModel + * @param $owner + * @param $relationName + * @param $pettyRelationName + * @return array */ - public function loadRelations($data) + private function _addError($relationModel, $owner, $relationName, $pettyRelationName) { - /** @var BaseActiveRecord $model */ - $model = $this->owner; - foreach ($this->_relations as $relationName) { - $relation = $model->getRelation($relationName); - $modelClass = $relation->modelClass; - /** @var BaseActiveRecord $relationalModel */ - $relationalModel = new $modelClass; - $formName = $relationalModel->formName(); - if (array_key_exists($formName, $data)) { - $model->{$relationName} = $data[$formName]; + foreach ($relationModel->errors as $attributeErrors) { + foreach ($attributeErrors as $error) { + $owner->addError($relationName, "{$pettyRelationName}: {$error}"); } } } + + private function _rollback() + { + if (($this->_transaction instanceof Transaction) && $this->_transaction->isActive) { + $this->_transaction->rollBack(); // If anything goes wrong, transaction will be rolled back + Yii::info("Rolling back", __METHOD__); + } + } } diff --git a/tests/SaveRelationsBehaviorTest.php b/tests/SaveRelationsBehaviorTest.php index e5cf342..5a1e7b7 100644 --- a/tests/SaveRelationsBehaviorTest.php +++ b/tests/SaveRelationsBehaviorTest.php @@ -462,7 +462,7 @@ class SaveRelationsBehaviorTest extends \PHPUnit_Framework_TestCase } /** - * @expectedException \Exception + * @expectedException yii\db\Exception */ public function testSavingRelationWithSameUniqueKeyShouldFail() { @@ -486,7 +486,25 @@ class SaveRelationsBehaviorTest extends \PHPUnit_Framework_TestCase ] ]; $project->loadRelations($data); - $this->assertFalse($project->save(), 'Project should not be saved'); + /*** + * This test throw an yii\base\Exception due to key conflict for related records. + * That kind of issue is hard to address because no validation process could prevent that. + * The exception is raised during the afterSave event of the owner model. + * In that case, the behavior takes care to rollback any database modifications + * and add an error to the related relational record. + * Anyway, the exception should be catched to address the correct workflow. + ***/ + try { + $project->save(); + } catch (\Exception $e) { + $this->assertArrayHasKey( + 'links', + $project->getErrors(), + 'Links #1: The combination \"en\"-\"newsoft\" of Language and Name has already been taken.' + ); + throw $e; + } + } public function testUpdatingAnExistingRelationShouldSucceed() @@ -549,9 +567,6 @@ class SaveRelationsBehaviorTest extends \PHPUnit_Framework_TestCase $project->loadRelations($data); $this->assertFalse($project->save(), 'Project could be saved'); $data = [ - 'Company' => [ - 'name' => 'YiiSoft' - ], 'Link' => [ [ 'language' => 'en', diff --git a/tests/models/Project.php b/tests/models/Project.php index 5450fce..10287cd 100644 --- a/tests/models/Project.php +++ b/tests/models/Project.php @@ -69,7 +69,9 @@ class Project extends \yii\db\ActiveRecord */ public function getUsers() { - return $this->hasMany(User::className(), ['id' => 'user_id'])->via('projectUsers'); + return $this->hasMany(User::className(), ['id' => 'user_id'])->via('projectUsers', function ($query) { + return $query; + }); } /**