diff --git a/src/ORM/Association/HasMany.php b/src/ORM/Association/HasMany.php index bb15c4e6b31..b72301975e5 100644 --- a/src/ORM/Association/HasMany.php +++ b/src/ORM/Association/HasMany.php @@ -164,63 +164,94 @@ public function saveStrategy($strategy = null) public function saveAssociated(EntityInterface $entity, array $options = []) { $targetEntities = $entity->get($this->getProperty()); - if (empty($targetEntities) && $this->_saveStrategy !== self::SAVE_REPLACE) { - return $entity; + + $isEmpty = in_array($targetEntities, [null, [], '', false], true); + if ($isEmpty) { + if ($entity->isNew() || + $this->getSaveStrategy() !== self::SAVE_REPLACE + ) { + return $entity; + } + + $targetEntities = []; } - if (!is_array($targetEntities) && !($targetEntities instanceof Traversable)) { + if (!is_array($targetEntities) && + !($targetEntities instanceof Traversable) + ) { $name = $this->getProperty(); $message = sprintf('Could not save %s, it cannot be traversed', $name); throw new InvalidArgumentException($message); } - $foreignKey = (array)$this->getForeignKey(); - $properties = array_combine( - $foreignKey, + $foreignKeyReference = array_combine( + (array)$this->getForeignKey(), $entity->extract((array)$this->getBindingKey()) ); - $target = $this->getTarget(); - $original = $targetEntities; + $options['_sourceTable'] = $this->getSource(); - $unlinkSuccessful = null; - if ($this->_saveStrategy === self::SAVE_REPLACE) { - $unlinkSuccessful = $this->_unlinkAssociated($properties, $entity, $target, $targetEntities, $options); + if ($this->_saveStrategy === self::SAVE_REPLACE && + !$this->_unlinkAssociated($foreignKeyReference, $entity, $this->getTarget(), $targetEntities, $options) + ) { + return false; } - if ($unlinkSuccessful === false) { + if (!$this->_saveTarget($foreignKeyReference, $entity, $targetEntities, $options)) { return false; } - foreach ($targetEntities as $k => $targetEntity) { - if (!($targetEntity instanceof EntityInterface)) { + return $entity; + } + + /** + * Persists each of the entities into the target table and creates links between + * the parent entity and each one of the saved target entities. + * + * @param array $foreignKeyReference The foreign key reference defining the link between the + * target entity, and the parent entity. + * @param \Cake\Datasource\EntityInterface $parentEntity The source entity containing the target + * entities to be saved. + * @param array|\Traversable $entities list of entities to persist in target table and to + * link to the parent entity + * @param array $options list of options accepted by `Table::save()`. + * @return bool `true` on success, `false` otherwise. + */ + protected function _saveTarget(array $foreignKeyReference, EntityInterface $parentEntity, $entities, array $options) + { + $foreignKey = array_keys($foreignKeyReference); + $table = $this->getTarget(); + $original = $entities; + + foreach ($entities as $k => $entity) { + if (!($entity instanceof EntityInterface)) { break; } if (!empty($options['atomic'])) { - $targetEntity = clone $targetEntity; + $entity = clone $entity; } - if ($properties !== $targetEntity->extract($foreignKey)) { - $targetEntity->set($properties, ['guard' => false]); + if ($foreignKeyReference !== $entity->extract($foreignKey)) { + $entity->set($foreignKeyReference, ['guard' => false]); } - if ($target->save($targetEntity, $options)) { - $targetEntities[$k] = $targetEntity; + if ($table->save($entity, $options)) { + $entities[$k] = $entity; continue; } if (!empty($options['atomic'])) { - $original[$k]->errors($targetEntity->errors()); + $original[$k]->errors($entity->errors()); $entity->set($this->getProperty(), $original); return false; } } - $entity->set($this->getProperty(), $targetEntities); + $parentEntity->set($this->getProperty(), $entities); - return $entity; + return true; } /** @@ -427,14 +458,15 @@ public function replace(EntityInterface $sourceEntity, array $targetEntities, ar * Deletes/sets null the related objects according to the dependency between source and targets and foreign key nullability * Skips deleting records present in $remainingEntities * - * @param array $properties array of foreignKey properties + * @param array $foreignKeyReference The foreign key reference defining the link between the + * target entity, and the parent entity. * @param \Cake\Datasource\EntityInterface $entity the entity which should have its associated entities unassigned * @param \Cake\ORM\Table $target The associated table * @param array $remainingEntities Entities that should not be deleted * @param array $options list of options accepted by `Table::delete()` * @return bool success */ - protected function _unlinkAssociated(array $properties, EntityInterface $entity, Table $target, array $remainingEntities = [], array $options = []) + protected function _unlinkAssociated(array $foreignKeyReference, EntityInterface $entity, Table $target, array $remainingEntities = [], array $options = []) { $primaryKey = (array)$target->getPrimaryKey(); $exclusions = new Collection($remainingEntities); @@ -450,18 +482,18 @@ function ($v) { ) ->toArray(); - $conditions = $properties; + $conditions = $foreignKeyReference; if (count($exclusions) > 0) { $conditions = [ 'NOT' => [ 'OR' => $exclusions ], - $properties + $foreignKeyReference ]; } - return $this->_unlink(array_keys($properties), $target, $conditions, $options); + return $this->_unlink(array_keys($foreignKeyReference), $target, $conditions, $options); } /** diff --git a/tests/TestCase/ORM/Association/HasManyTest.php b/tests/TestCase/ORM/Association/HasManyTest.php index 23e699b1cc6..257e75a5039 100644 --- a/tests/TestCase/ORM/Association/HasManyTest.php +++ b/tests/TestCase/ORM/Association/HasManyTest.php @@ -740,4 +740,216 @@ public function testLinkUsesSingleTransaction() $new = $this->author->get(2, ['contain' => 'Articles']); $this->assertCount(3, $new->articles); } + + /** + * Test that saveAssociated() fails on non-empty, non-iterable value + * + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage Could not save comments, it cannot be traversed + * @return void + */ + public function testSaveAssociatedNotEmptyNotIterable() + { + $articles = TableRegistry::get('Articles'); + $assoc = $articles->hasMany('Comments', [ + 'saveStrategy' => HasMany::SAVE_APPEND + ]); + + $entity = new Entity( + [ + 'id' => 1, + 'comments' => 'oh noes', + ], + ['markNew' => true] + ); + + $assoc->saveAssociated($entity); + } + + /** + * Provider for empty values + * + * @return array + */ + public function emptyProvider() + { + return [ + [''], + [false], + [null], + [[]] + ]; + } + + /** + * Test that saving an empty set with the `append` strategy on create works. + * + * @dataProvider emptyProvider + * @return void + */ + public function testSaveAssociatedAppendEmptySetCreateSuccess($value) + { + /* @var $table \Cake\ORM\Table|\PHPUnit_Framework_MockObject_MockObject */ + $table = $this + ->getMockBuilder('Cake\ORM\Table') + ->setMethods(['table']) + ->getMock(); + $table->setSchema([]); + + /* @var $assoc \Cake\ORM\Association\HasMany|\PHPUnit_Framework_MockObject_MockObject */ + $assoc = $this + ->getMockBuilder('\Cake\ORM\Association\HasMany') + ->setMethods(['_unlinkAssociated', '_saveTarget']) + ->setConstructorArgs(['comments', ['sourceTable' => $table]]) + ->getMock(); + $assoc->setSaveStrategy(HasMany::SAVE_APPEND); + + $assoc + ->expects($this->never()) + ->method('_unlinkAssociated'); + $assoc + ->expects($this->never()) + ->method('_saveTarget'); + + $entity = new Entity( + [ + 'id' => 1, + 'comments' => $value, + ], + ['markNew' => true] + ); + + $this->assertSame($entity, $assoc->saveAssociated($entity)); + } + + /** + * Test that saving an empty set with the `append` strategy on update works. + * + * @dataProvider emptyProvider + * @return void + */ + public function testSaveAssociatedAppendEmptySetUpdateSuccess($value) + { + /* @var $table \Cake\ORM\Table|\PHPUnit_Framework_MockObject_MockObject */ + $table = $this + ->getMockBuilder('Cake\ORM\Table') + ->setMethods(['table']) + ->getMock(); + $table->setSchema([]); + + /* @var $assoc \Cake\ORM\Association\HasMany|\PHPUnit_Framework_MockObject_MockObject */ + $assoc = $this + ->getMockBuilder('\Cake\ORM\Association\HasMany') + ->setMethods(['_unlinkAssociated', '_saveTarget']) + ->setConstructorArgs(['comments', ['sourceTable' => $table]]) + ->getMock(); + $assoc->setSaveStrategy(HasMany::SAVE_APPEND); + + $assoc + ->expects($this->never()) + ->method('_unlinkAssociated'); + $assoc + ->expects($this->never()) + ->method('_saveTarget'); + + $entity = new Entity( + [ + 'id' => 1, + 'comments' => $value, + ], + ['markNew' => false] + ); + + $this->assertSame($entity, $assoc->saveAssociated($entity)); + } + + /** + * Test that saving an empty set with the `replace` strategy on create works. + * + * @dataProvider emptyProvider + * @return void + */ + public function testSaveAssociatedReplaceEmptySetCreateSuccess($value) + { + /* @var $table \Cake\ORM\Table|\PHPUnit_Framework_MockObject_MockObject */ + $table = $this + ->getMockBuilder('Cake\ORM\Table') + ->setMethods(['table']) + ->getMock(); + $table->setSchema([]); + + /* @var $assoc \Cake\ORM\Association\HasMany|\PHPUnit_Framework_MockObject_MockObject */ + $assoc = $this->getMockBuilder('\Cake\ORM\Association\HasMany') + ->setMethods(['_unlinkAssociated', '_saveTarget']) + ->setConstructorArgs(['comments', ['sourceTable' => $table]]) + ->getMock(); + $assoc->setSaveStrategy(HasMany::SAVE_REPLACE); + + $assoc + ->expects($this->never()) + ->method('_unlinkAssociated'); + $assoc + ->expects($this->never()) + ->method('_saveTarget'); + + $entity = new Entity( + [ + 'id' => 1, + 'comments' => $value, + ], + ['markNew' => true] + ); + + $this->assertSame($entity, $assoc->saveAssociated($entity)); + } + + /** + * Test that saving an empty set with the `replace` strategy on update works. + * + * @dataProvider emptyProvider + * @return void + */ + public function testSaveAssociatedReplaceEmptySetUpdateSuccess($value) + { + /* @var $table \Cake\ORM\Table|\PHPUnit_Framework_MockObject_MockObject */ + $table = $this + ->getMockBuilder('Cake\ORM\Table') + ->setMethods(['table']) + ->getMock(); + $table->setSchema([]); + + /* @var $assoc \Cake\ORM\Association\HasMany|\PHPUnit_Framework_MockObject_MockObject */ + $assoc = $this + ->getMockBuilder('\Cake\ORM\Association\HasMany') + ->setMethods(['_unlinkAssociated', '_saveTarget']) + ->setConstructorArgs(['comments', [ + 'sourceTable' => $table, + 'foreignKey' => 'article_id', + 'bindingKey' => 'id' + ]]) + ->getMock(); + $assoc->setSaveStrategy(HasMany::SAVE_REPLACE); + + $entity = new Entity( + [ + 'id' => 1, + 'comments' => $value, + ], + ['markNew' => false] + ); + + $assoc + ->expects($this->once()) + ->method('_unlinkAssociated') + ->with(['article_id' => 1], $entity, $assoc->getTarget(), [], ['_sourceTable' => $table]) + ->will($this->returnValue(true)); + + $assoc + ->expects($this->once()) + ->method('_saveTarget') + ->with(['article_id' => 1], $entity, [], ['_sourceTable' => $table]) + ->will($this->returnValue(true)); + + $this->assertSame($entity, $assoc->saveAssociated($entity)); + } }