Merge pull request #3523 from cakephp/issue-3485

Fix issues saving belongsToMany associations

src/ORM/Association/BelongsToMany.php

@@ -385,10 +385,14 @@ class BelongsToMany extends Association {
 		$targetEntity = $entity->get($this->property());
 		$strategy = $this->saveStrategy();
 
-		if (!$targetEntity) {
+		if ($targetEntity === null) {
 			return false;
 		}
 
+		if ($targetEntity === [] && $entity->isNew()) {
+			return $entity;
+		}
+
 		if ($strategy === self::SAVE_APPEND) {
 			return $this->_saveTarget($entity, $targetEntity, $options);
 		}
@@ -516,7 +520,7 @@ class BelongsToMany extends Association {
  *
  * This method does not check link uniqueness.
  *
- * ###Example:
+ * ### Example:
  *
  * {{{
  * $newTags = $tags->find('relevant')->execute();
@@ -682,12 +686,17 @@ class BelongsToMany extends Association {
 					return false;
 				}
 
+				$inserted = [];
 				$property = $this->property();
-				$inserted = array_combine(
-					array_keys($inserts),
-					(array)$sourceEntity->get($property)
-				);
-				$targetEntities = $inserted + $targetEntities;
+
+				if (count($inserts)) {
+					$inserted = array_combine(
+						array_keys($inserts),
+						(array)$sourceEntity->get($property)
+					);
+					$targetEntities = $inserted + $targetEntities;
+				}
+
 				ksort($targetEntities);
 				$sourceEntity->set($property, array_values($targetEntities));
 				$sourceEntity->dirty($property, false);

src/ORM/Marshaller.php

@@ -170,7 +170,7 @@ class Marshaller {
  * @return array An array of built entities.
  */
 	protected function _belongsToMany(Association $assoc, array $data, $include = []) {
-		$hasIds = isset($data['_ids']);
+		$hasIds = array_key_exists('_ids', $data);
 		if ($hasIds && is_array($data['_ids'])) {
 			return $this->_loadBelongsToMany($assoc, $data['_ids']);
 		}
@@ -361,9 +361,13 @@ class Marshaller {
  * @return mixed
  */
 	protected function _mergeBelongsToMany($original, $assoc, $value, $include) {
-		if (isset($value['_ids']) && is_array($value['_ids'])) {
+		$hasIds = array_key_exists('_ids', $value);
+		if ($hasIds && is_array($value['_ids'])) {
 			return $this->_loadBelongsToMany($assoc, $value['_ids']);
 		}
+		if ($hasIds) {
+			return [];
+		}
 
 		if (!in_array('_joinData', $include) && !isset($include['_joinData'])) {
 			return $this->mergeMany($original, $value, $include);

tests/TestCase/ORM/Association/BelongsToManyTest.php

@@ -1240,7 +1240,83 @@ class BelongsToManyTest extends TestCase {
 	}
 
 /**
- * Tests that replaceLinks will delete entities not present in the passed
+ * Test that replaceLinks() can save an empty set, removing all rows.
+ *
+ * @return void
+ */
+	public function testReplaceLinksUpdateToEmptySet() {
+		$connection = ConnectionManager::get('test');
+		$joint = $this->getMock(
+			'\Cake\ORM\Table',
+			['delete', 'find'],
+			[['alias' => 'ArticlesTags', 'connection' => $connection]]
+		);
+		$config = [
+			'sourceTable' => $this->article,
+			'targetTable' => $this->tag,
+			'through' => $joint,
+			'joinTable' => 'tags_articles'
+		];
+		$assoc = $this->getMock(
+			'\Cake\ORM\Association\BelongsToMany',
+			['_collectJointEntities', '_saveTarget'],
+			['tags', $config]
+		);
+		$assoc->junction();
+
+		$this->article
+			->association('ArticlesTags')
+			->conditions(['foo' => 1]);
+
+		$query1 = $this->getMock(
+			'\Cake\ORM\Query',
+			['where', 'andWhere', 'addDefaultTypes'],
+			[$connection, $joint]
+		);
+
+		$joint->expects($this->at(0))->method('find')
+			->with('all')
+			->will($this->returnValue($query1));
+
+		$query1->expects($this->at(0))
+			->method('where')
+			->with(['foo' => 1])
+			->will($this->returnSelf());
+		$query1->expects($this->at(1))
+			->method('where')
+			->with(['article_id' => 1])
+			->will($this->returnSelf());
+
+		$existing = [
+			new Entity(['article_id' => 1, 'tag_id' => 2]),
+			new Entity(['article_id' => 1, 'tag_id' => 4]),
+		];
+		$query1->setResult(new \ArrayIterator($existing));
+
+		$tags = [];
+		$entity = new Entity(['id' => 1, 'test' => $tags]);
+
+		$assoc->expects($this->once())->method('_collectJointEntities')
+			->with($entity, $tags)
+			->will($this->returnValue([]));
+
+		$joint->expects($this->at(1))
+			->method('delete')
+			->with($existing[0]);
+		$joint->expects($this->at(2))
+			->method('delete')
+			->with($existing[1]);
+
+		$assoc->expects($this->never())
+			->method('_saveTarget');
+
+		$assoc->replaceLinks($entity, $tags);
+		$this->assertSame([], $entity->tags);
+		$this->assertFalse($entity->dirty('tags'));
+	}
+
+/**
+ * Tests that replaceLinks will delete entities not present in the passed,
  * array, maintain those are already persisted and were passed and also
  * insert the rest.
  *
@@ -1335,6 +1411,30 @@ class BelongsToManyTest extends TestCase {
 	}
 
 /**
+ * Test that saving an empty set on create works.
+ *
+ * @return void
+ */
+	public function testSaveEmptySetSuccess() {
+		$assoc = $this->getMock(
+			'\Cake\ORM\Association\BelongsToMany',
+			['_saveTarget', 'replaceLinks'],
+			['tags']
+		);
+		$entity = new Entity([
+			'id' => 1,
+			'tags' => []
+		], ['markNew' => true]);
+
+		$assoc->saveStrategy(BelongsToMany::SAVE_REPLACE);
+		$assoc->expects($this->never())
+			->method('replaceLinks');
+		$assoc->expects($this->never())
+			->method('_saveTarget');
+		$this->assertSame($entity, $assoc->save($entity));
+	}
+
+/**
  * Tests saving with replace strategy returning true
  *
  * @return void

tests/TestCase/ORM/MarshallerTest.php

@@ -477,7 +477,22 @@ class MarshallerTest extends TestCase {
 		];
 		$marshall = new Marshaller($this->articles);
 		$result = $marshall->one($data, ['Tags']);
+		$this->assertCount(0, $result->tags);
 
+		$data = [
+			'title' => 'Haz tags',
+			'body' => 'Some content here',
+			'tags' => ['_ids' => false]
+		];
+		$result = $marshall->one($data, ['Tags']);
+		$this->assertCount(0, $result->tags);
+
+		$data = [
+			'title' => 'Haz tags',
+			'body' => 'Some content here',
+			'tags' => ['_ids' => null]
+		];
+		$result = $marshall->one($data, ['Tags']);
 		$this->assertCount(0, $result->tags);
 
 		$data = [
@@ -726,6 +741,46 @@ class MarshallerTest extends TestCase {
 	}
 
 /**
+ * Tests that merging data to an entity containing belongsToMany and _ids
+ * will ignore empty values.
+ *
+ * @return void
+ */
+	public function testMergeBelongsToManyEntitiesFromIdsEmptyValue() {
+		$entity = new Entity([
+			'title' => 'Haz tags',
+			'body' => 'Some content here',
+			'tags' => [
+				new Entity(['id' => 1, 'name' => 'Cake']),
+				new Entity(['id' => 2, 'name' => 'PHP'])
+			]
+		]);
+
+		$data = [
+			'title' => 'Haz moar tags',
+			'tags' => ['_ids' => '']
+		];
+		$entity->accessible('*', true);
+		$marshall = new Marshaller($this->articles);
+		$result = $marshall->merge($entity, $data, ['Tags']);
+		$this->assertCount(0, $result->tags);
+
+		$data = [
+			'title' => 'Haz moar tags',
+			'tags' => ['_ids' => false]
+		];
+		$result = $marshall->merge($entity, $data, ['Tags']);
+		$this->assertCount(0, $result->tags);
+
+		$data = [
+			'title' => 'Haz moar tags',
+			'tags' => ['_ids' => null]
+		];
+		$result = $marshall->merge($entity, $data, ['Tags']);
+		$this->assertCount(0, $result->tags);
+	}
+
+/**
  * Test merging the _joinData entity for belongstomany associations.
  *
  * @return void

tests/TestCase/ORM/TableTest.php

@@ -1503,7 +1503,7 @@ class TableTest extends \Cake\TestSuite\TestCase {
  * @group save
  * @return void
  */
-	public function testsASavedEntityIsClean() {
+	public function testASavedEntityIsClean() {
 		$entity = new \Cake\ORM\Entity([
 			'username' => 'superuser',
 			'password' => 'root',
@@ -1524,7 +1524,7 @@ class TableTest extends \Cake\TestSuite\TestCase {
  * @group save
  * @return void
  */
-	public function testsASavedEntityIsNotNew() {
+	public function testASavedEntityIsNotNew() {
 		$entity = new \Cake\ORM\Entity([
 			'username' => 'superuser',
 			'password' => 'root',
@@ -2594,6 +2594,59 @@ class TableTest extends \Cake\TestSuite\TestCase {
 	}
 
 /**
+ * Tests saving belongsToMany records can delete all links.
+ *
+ * @group save
+ * @return void
+ */
+	public function testSaveBelongsToManyDeleteAllLinks() {
+		$table = TableRegistry::get('articles');
+		$table->belongsToMany('tags', [
+			'saveStrategy' => 'replace',
+		]);
+
+		$entity = $table->get(1, ['contain' => 'Tags']);
+		$this->assertCount(2, $entity->tags, 'Fixture data did not change.');
+
+		$entity->tags = [];
+		$result = $table->save($entity);
+		$this->assertSame($result, $entity);
+		$this->assertSame([], $entity->tags, 'No tags on the entity.');
+
+		$entity = $table->get(1, ['contain' => 'Tags']);
+		$this->assertSame([], $entity->tags, 'No tags in the db either.');
+	}
+
+/**
+ * Tests saving belongsToMany records can delete some links.
+ *
+ * @group save
+ * @return void
+ */
+	public function testSaveBelongsToManyDeleteSomeLinks() {
+		$table = TableRegistry::get('articles');
+		$table->belongsToMany('tags', [
+			'saveStrategy' => 'replace',
+		]);
+
+		$entity = $table->get(1, ['contain' => 'Tags']);
+		$this->assertCount(2, $entity->tags, 'Fixture data did not change.');
+
+		$tag = new \Cake\ORM\Entity([
+			'id' => 2,
+		]);
+		$entity->tags = [$tag];
+		$result = $table->save($entity);
+		$this->assertSame($result, $entity);
+		$this->assertCount(1, $entity->tags, 'Only one tag left.');
+		$this->assertEquals($tag, $entity->tags[0]);
+
+		$entity = $table->get(1, ['contain' => 'Tags']);
+		$this->assertCount(1, $entity->tags, 'Only one tag in the db.');
+		$this->assertEquals($tag->id, $entity->tags[0]->id);
+	}
+
+/**
  * Tests saving belongsToMany records with a validation error and atomic set
  * to true
  *