From 4e0ec16f6d45d52a33464a47857cec761e3119c3 Mon Sep 17 00:00:00 2001 From: Alban Jubert Date: Mon, 9 Oct 2017 18:11:10 +0200 Subject: [PATCH] Enh #3: Added the abilty to specify the validation scenario for related records Exception is now thrown (with a transaction rollback when relevant) in case a related record could not be saved during afterSave event --- .gitignore | 1 + CHANGELOG.md | 13 +++- README.md | 16 ++++- src/SaveRelationsBehavior.php | 126 +++++++++++++++++++++--------------- tests/SaveRelationsBehaviorTest.php | 112 ++++++++++++++++++++++++++++++-- tests/bootstrap.php | 11 +++- tests/models/Link.php | 3 +- tests/models/ProjectLink.php | 4 +- 8 files changed, 224 insertions(+), 62 deletions(-) diff --git a/.gitignore b/.gitignore index 8bbe89d..028dc1f 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ /vendor/ /tests/report/ /phpunit.phar +/tests/runtime/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 6eca5c4..4f37a5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,19 @@ # Yii2 Active Record Save Relations Behavior Change Log -## [1.2.1] +## [1.3.0] - Unreleased version +### Added +- Ability to define validation scenario for related records +- More test cases + +### Fixed +- False positive testLoadRelationsShouldSucceed test case + +### Changed +- afterSave throw exception if a related record fail to be saved +- related record are now correctly updated based on there primary key (Thanks to @DD174) + ## [1.2.0] ### Changed - Use of `ActiveQueryInterface` and `BaseActiveRecord` to ensure broader DB driver compatibility (Thx @bookin) diff --git a/README.md b/README.md index 5a527d7..a13c8e2 100644 --- a/README.md +++ b/README.md @@ -135,7 +135,21 @@ Every declared related models will be validated prior to be saved. If any valida For `hasMany()` relations, the index of the related model will be used to identify the associated error message. - +It is possible to specify the validation scenario for each relation by declaring an associative array in which the `scenario` key must contain the needed scenario value. +For instance, in the following configuration, the `links ` related records will be validated using the `Link::SOME_SCENARIO` scenario: +```php +... + public function behaviors() + { + return [ + 'saveRelations' => [ + 'class' => SaveRelationsBehavior::className(), + 'relations' => ['company', 'users', 'links' => ['scenario' => Link::SOME_SCENARIO]] + ], + ]; + } +... +``` > **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. diff --git a/src/SaveRelationsBehavior.php b/src/SaveRelationsBehavior.php index deca23e..0b51042 100644 --- a/src/SaveRelationsBehavior.php +++ b/src/SaveRelationsBehavior.php @@ -13,6 +13,7 @@ use yii\db\BaseActiveRecord; 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. @@ -29,12 +30,13 @@ class SaveRelationsBehavior extends Behavior private $_transaction; private $_relationsScenario = []; - private $_relationsCascadeDelete = []; + + //private $_relationsCascadeDelete = []; //TODO public function init() { parent::init(); - $allowedProperties = ['scenario', 'cascadeDelete']; + $allowedProperties = ['scenario']; foreach ($this->relations as $key => $value) { if (is_int($key)) { $this->_relations[] = $value; @@ -102,7 +104,15 @@ class SaveRelationsBehavior extends Behavior if (in_array($name, $this->_relations)) { Yii::trace("Setting {$name} relation value", __METHOD__); if (!isset($this->_oldRelationValue[$name])) { - $this->_oldRelationValue[$name] = $this->owner->{$name}; + if ($this->owner->isNewRecord) { + if ($this->owner->getRelation($name)->multiple === true) { + $this->_oldRelationValue[$name] = []; + } else { + $this->_oldRelationValue[$name] = null; + } + } else { + $this->_oldRelationValue[$name] = $this->owner->{$name}; + } } if ($this->owner->getRelation($name)->multiple === true) { $this->setMultipleRelation($name, $value); @@ -167,15 +177,14 @@ class SaveRelationsBehavior extends Behavior // Get the related model foreign keys if (is_array($data)) { $fks = []; - + // search PK foreach($modelClass::primaryKey() as $modelAttribute) { if (array_key_exists($modelAttribute, $data) && !empty($data[$modelAttribute])) { $fks[$modelAttribute] = $data[$modelAttribute]; } } - - if (!$fks) { + if (empty($fks)) { // Get the right link definition if ($relation->via instanceof BaseActiveRecord) { $viaQuery = $relation->via; @@ -186,7 +195,6 @@ class SaveRelationsBehavior extends Behavior } else { $link = $relation->link; } - foreach ($link as $relatedAttribute => $modelAttribute) { if (array_key_exists($modelAttribute, $data) && !empty($data[$modelAttribute])) { $fks[$modelAttribute] = $data[$modelAttribute]; @@ -317,9 +325,9 @@ class SaveRelationsBehavior extends Behavior /** @var BaseActiveRecord $model */ $model = $this->owner; if (!is_null($relationModel) && ($relationModel->isNewRecord || count($relationModel->getDirtyAttributes()))) { -// if (key_exists($relationModel, $this->_relationsScenario)) { -// $relationModel->setScenario($this->_relationsScenario[$relationModel]); -// } + if (key_exists($relationName, $this->_relationsScenario)) { + $relationModel->setScenario($this->_relationsScenario[$relationName]); + } Yii::trace("Validating {$pettyRelationName} relation model using " . $relationModel->scenario . " scenario", __METHOD__); if (!$relationModel->validate()) { foreach ($relationModel->errors as $attributeErrors) { @@ -343,56 +351,70 @@ class SaveRelationsBehavior extends Behavior /** @var BaseActiveRecord $model */ $model = $this->owner; $this->_relationsSaveStarted = true; - foreach ($this->_relations as $relationName) { - if (array_key_exists($relationName, $this->_oldRelationValue)) { // Relation was not set, do nothing... - Yii::trace("Linking {$relationName} relation", __METHOD__); - $relation = $model->getRelation($relationName); - if ($relation->multiple === true) { // Has many relation - // Process new relations - $existingRecords = []; - /** @var BaseActiveRecord $relationModel */ - foreach ($model->{$relationName} as $relationModel) { - if ($relationModel->isNewRecord) { - if ($relation->via !== null) { - $relationModel->save(false); + 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__); + $relation = $model->getRelation($relationName); + if ($relation->multiple === true) { // Has many relation + // Process new relations + $existingRecords = []; + + /** @var BaseActiveRecord $relationModel */ + foreach ($model->{$relationName} as $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()) . ')'); + } + } + $model->link($relationName, $relationModel); + } else { + $existingRecords[] = $relationModel; + } + if (count($relationModel->dirtyAttributes)) { + if (!$relationModel->save()) { + throw new Exception('Related model ' . $relationName . ' could not be saved (' . VarDumper::dumpAsString($relationModel->getErrors()) . ')'); + } } - $model->link($relationName, $relationModel); - } else { - $existingRecords[] = $relationModel; } - if (count($relationModel->dirtyAttributes)) { - $relationModel->save(false); + // Process existing added and deleted relations + list($addedPks, $deletedPks) = $this->_computePkDiff($this->_oldRelationValue[$relationName], $existingRecords); + // Deleted relations + $initialModels = ArrayHelper::index($this->_oldRelationValue[$relationName], function (BaseActiveRecord $model) { + return implode("-", $model->getPrimaryKey(true)); + }); + foreach ($deletedPks as $key) { + $model->unlink($relationName, $initialModels[$key], true); } - } - // Process existing added and deleted relations - list($addedPks, $deletedPks) = $this->_computePkDiff($this->_oldRelationValue[$relationName], $existingRecords); - // Deleted relations - $initialModels = ArrayHelper::index($this->_oldRelationValue[$relationName], function (BaseActiveRecord $model) { - return implode("-", $model->getPrimaryKey(true)); - }); - foreach ($deletedPks as $key) { - $model->unlink($relationName, $initialModels[$key], true); - } - // Added relations - $actualModels = ArrayHelper::index($model->{$relationName}, function (BaseActiveRecord $model) { - return implode("-", $model->getPrimaryKey(true)); - }); - foreach ($addedPks as $key) { - $model->link($relationName, $actualModels[$key]); - } - } else { // Has one relation - if ($this->_oldRelationValue[$relationName] !== $model->{$relationName}) { - if ($model->{$relationName} instanceof BaseActiveRecord) { - $model->link($relationName, $model->{$relationName}); - } else { - if ($this->_oldRelationValue[$relationName] instanceof BaseActiveRecord) { - $model->unlink($relationName, $this->_oldRelationValue[$relationName]); + // Added relations + $actualModels = ArrayHelper::index($model->{$relationName}, function (BaseActiveRecord $model) { + return implode("-", $model->getPrimaryKey(true)); + }); + foreach ($addedPks as $key) { + $model->link($relationName, $actualModels[$key]); + } + } else { // Has one relation + if ($this->_oldRelationValue[$relationName] !== $model->{$relationName}) { + if ($model->{$relationName} instanceof BaseActiveRecord) { + $model->link($relationName, $model->{$relationName}); + } else { + if ($this->_oldRelationValue[$relationName] instanceof BaseActiveRecord) { + $model->unlink($relationName, $this->_oldRelationValue[$relationName]); + } } } } + unset($this->_oldRelationValue[$relationName]); } - unset($this->_oldRelationValue[$relationName]); } + } 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__); + } + throw $e; } $model->refresh(); $this->_relationsSaveStarted = false; diff --git a/tests/SaveRelationsBehaviorTest.php b/tests/SaveRelationsBehaviorTest.php index d0787b9..e5cf342 100644 --- a/tests/SaveRelationsBehaviorTest.php +++ b/tests/SaveRelationsBehaviorTest.php @@ -10,7 +10,6 @@ use tests\models\DummyModel; use tests\models\DummyModelParent; use tests\models\Link; use tests\models\Project; -use tests\models\ProjectLink; use tests\models\ProjectNoTransactions; use tests\models\User; use Yii; @@ -258,7 +257,7 @@ class SaveRelationsBehaviorTest extends \PHPUnit_Framework_TestCase $this->assertCount(2, $project->users, 'Project should have 2 users before save'); $project->users = array_merge($project->users, [$user]); // Add new user to the existing list $this->assertCount(3, $project->users, 'Project should have 3 users after assignment'); - $this->assertTrue($project->save(), 'Project could not be saved'.VarDumper::dumpAsString($project->errors)); + $this->assertTrue($project->save(), 'Project could not be saved' . VarDumper::dumpAsString($project->errors)); $this->assertCount(3, $project->users, 'Project should have 3 users after save'); } @@ -407,10 +406,10 @@ class SaveRelationsBehaviorTest extends \PHPUnit_Framework_TestCase { $project = Project::findOne(1); $data = [ - 'Company' => [ + 'Company' => [ 'name' => 'YiiSoft' ], - 'ProjectLink' => [ + 'Link' => [ [ 'language' => 'en', 'name' => 'yii', @@ -427,6 +426,8 @@ class SaveRelationsBehaviorTest extends \PHPUnit_Framework_TestCase $this->assertTrue($project->save(), 'Project could not be saved'); $this->assertEquals('YiiSoft', $project->company->name, "Company name should be YiiSoft"); $this->assertCount(2, $project->projectLinks, "Project should have 2 links"); + $this->assertEquals($project->links[0]->link, 'http://www.yiiframework.com'); + $this->assertEquals($project->links[1]->link, 'http://www.yiiframework.fr'); } public function testAssignSingleObjectToHasManyRelationShouldSucceed() @@ -460,4 +461,107 @@ class SaveRelationsBehaviorTest extends \PHPUnit_Framework_TestCase $this->assertTrue($dummy_a->save(), 'Dummy A could not be saved'); } + /** + * @expectedException \Exception + */ + public function testSavingRelationWithSameUniqueKeyShouldFail() + { + $project = new Project(); + $project->name = "Yii Framework"; + $data = [ + 'Company' => [ + 'name' => 'NewSoft' + ], + 'Link' => [ + [ + 'language' => 'en', + 'name' => 'newsoft', + 'link' => 'http://www.newsoft.com' + ], + [ + 'language' => 'en', + 'name' => 'newsoft', + 'link' => 'http://www.newsoft.co.uk' + ] + ] + ]; + $project->loadRelations($data); + $this->assertFalse($project->save(), 'Project should not be saved'); + } + + public function testUpdatingAnExistingRelationShouldSucceed() + { + $project = new Project(); + $project->name = "Yii Framework"; + $data = [ + 'Company' => [ + 'name' => 'YiiSoft' + ], + 'Link' => [ + [ + 'language' => 'en', + 'name' => 'yii', + 'link' => 'http://www.yiiframework.ru' + ] + ] + ]; + $project->loadRelations($data); + $this->assertTrue($project->save(), 'Project could not be saved'); + $this->assertEquals('YiiSoft', $project->company->name, "Company name should be YiiSoft"); + $this->assertCount(1, $project->projectLinks, "Project should have 1 link"); + $this->assertEquals($project->links[0]->link, 'http://www.yiiframework.ru'); + $data = [ + 'Link' => [ + [ + 'language' => 'en', + 'name' => 'yii', + 'link' => 'http://www.yiiframework.com' + ], + [ + 'language' => 'fr', + 'name' => 'yii', + 'link' => 'http://www.yiiframework.fr' + ] + ] + ]; + $project->loadRelations($data); + $this->assertTrue($project->save(), 'Project could not be saved'); + $this->assertEquals($project->links[0]->link, 'http://www.yiiframework.com'); + $this->assertEquals($project->links[1]->link, 'http://www.yiiframework.fr'); + } + + public function testPerScenarioAttributeValidationShouldSucceed() + { + $project = new Project(); + $project->name = "Yii Framework"; + $data = [ + 'Company' => [ + 'name' => 'YiiSoft' + ], + 'Link' => [ + [ + 'language' => 'en', + 'name' => 'yii', + 'link' => 'Invalid value', + ] + ] + ]; + $project->loadRelations($data); + $this->assertFalse($project->save(), 'Project could be saved'); + $data = [ + 'Company' => [ + 'name' => 'YiiSoft' + ], + 'Link' => [ + [ + 'language' => 'en', + 'name' => 'yii', + 'link' => 'http://www.yiiframework.com', + ] + ] + ]; + $project->loadRelations($data); + $this->assertTrue($project->save(), 'Project could not be saved'); + } + } diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 417eab4..3aea0f7 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -15,10 +15,19 @@ new \yii\console\Application([ 'id' => 'unit', 'basePath' => __DIR__, 'vendorPath' => dirname(__DIR__) . '/vendor', +// 'bootstrap' => ['log'], 'components' => [ 'db' => [ 'class' => 'yii\db\Connection', 'dsn' => 'sqlite::memory:', ], - ], +// 'log' => [ +// 'targets' => [ +// [ +// 'class' => 'yii\log\FileTarget', +// 'categories' => ['yii\db\*', 'lhs\Yii2SaveRelationsBehavior\*'] +// ], +// ] +// ], + ] ]); diff --git a/tests/models/Link.php b/tests/models/Link.php index 23ecb0e..da3a64a 100644 --- a/tests/models/Link.php +++ b/tests/models/Link.php @@ -6,9 +6,7 @@ use lhs\Yii2SaveRelationsBehavior\SaveRelationsBehavior; class Link extends \yii\db\ActiveRecord { - const SCENARIO_FIRST = 'first'; - const SCENARIO_SECOND = 'second'; /** * @inheritdoc @@ -39,6 +37,7 @@ class Link extends \yii\db\ActiveRecord return [ [['language', 'name', 'link'], 'required'], [['name'], 'unique', 'targetAttribute' => ['language', 'name']], + [['link'], 'url', 'on' => [self::SCENARIO_FIRST]], [['link_type_id'], 'safe'] ]; } diff --git a/tests/models/ProjectLink.php b/tests/models/ProjectLink.php index 3ca876e..e20f6db 100644 --- a/tests/models/ProjectLink.php +++ b/tests/models/ProjectLink.php @@ -4,6 +4,7 @@ namespace tests\models; class ProjectLink extends \yii\db\ActiveRecord { + /** * @inheritdoc */ @@ -18,7 +19,8 @@ class ProjectLink extends \yii\db\ActiveRecord public function rules() { return [ - [['language', 'name', 'project_id'], 'required'] + [['language', 'name', 'project_id'], 'required'], + [['language', 'name'], 'unique'] ]; }