Add loose emptiness handling for HasMany associations.

Adds support for treating `null`, `false`, and empty strings as empty sets of associated data, similar to how `BelongsToMany` handles such values. Also introduces bailing out early on empty sets for not yet persisted entities when using the `replace` save strategy. Solves #9474
cakephp · Apr 4, 2017 · b6b46ff · b6b46ff
1 parent 0a5c9eb
commit b6b46ff
Show file tree

Hide file tree

Showing 2 changed files with 271 additions and 27 deletions.
diff --git 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
@@ -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));
+    }
 }