Fix Table::save() not pre-validating all entities.

This change fixes #3600, and dramatically changes how save() works. Instead of validating and saving entities in sequence, all entities are pre-validated. If all validation succeeds then entities will be saved. This makes the 'validate' option act like validate=first in 2.x. The 'atomic' is almost entirely orthogonal controlling whether or not the save is run in a transaction or not. The associated parents do not need to be revalidated so we can disable validation for them. However, child association do need further validation as the save process for belongsToMany materializes new objects that need validation applied.
cakephp · Sep 18, 2014 · e7637f6 · e7637f6
1 parent 36fe51e
commit e7637f6
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 31 deletions.
diff --git a/src/ORM/EntityValidator.php b/src/ORM/EntityValidator.php
@@ -80,6 +80,7 @@ protected function _buildPropertyMap($include) {
  */
 	public function one(Entity $entity, $options = []) {
 		$valid = true;
+		$types = [Association::ONE_TO_ONE, Association::MANY_TO_ONE];
 		$propertyMap = $this->_buildPropertyMap($options);
 
 		foreach ($propertyMap as $key => $assoc) {
@@ -89,10 +90,14 @@ public function one(Entity $entity, $options = []) {
 			if (!$value) {
 				continue;
 			}
+			$isOne = in_array($association->type(), $types);
+			if ($isOne && !($value instanceof Entity)) {
+				$valid = false;
+				continue;
+			}
 
 			$validator = $association->target()->entityValidator();
-			$types = [Association::ONE_TO_ONE, Association::MANY_TO_ONE];
-			if (in_array($association->type(), $types)) {
+			if ($isOne) {
 				$valid = $validator->one($value, $assoc['options']) && $valid;
 			} else {
 				$valid = $validator->many($value, $assoc['options']) && $valid;
@@ -102,7 +107,6 @@ public function one(Entity $entity, $options = []) {
 		if (!isset($options['validate'])) {
 			$options['validate'] = true;
 		}
-
 		return $this->_processValidation($entity, $options) && $valid;
 	}
 

diff --git a/src/ORM/Table.php b/src/ORM/Table.php
@@ -1170,15 +1170,12 @@ protected function _processSave($entity, $options) {
 			$entity->isNew(!$this->exists($conditions));
 		}
 
-		$associated = $options['associated'];
-		$options['associated'] = [];
+		$options['associated'] = $this->_associations->normalizeKeys($options['associated']);
 		$validate = $options['validate'];
 
 		if ($validate && !$this->validate($entity, $options)) {
 			return false;
 		}
-
-		$options['associated'] = $this->_associations->normalizeKeys($associated);
 		$event = $this->dispatchEvent('Model.beforeSave', compact('entity', 'options'));
 
 		if ($event->isStopped()) {
@@ -1189,7 +1186,7 @@ protected function _processSave($entity, $options) {
 			$this,
 			$entity,
 			$options['associated'],
-			['validate' => (bool)$validate] + $options->getArrayCopy()
+			['validate' => false] + $options->getArrayCopy()
 		);
 
 		if (!$saved && $options['atomic']) {

diff --git a/tests/TestCase/ORM/TableTest.php b/tests/TestCase/ORM/TableTest.php
@@ -2499,8 +2499,8 @@ public function testSaveOnlySaveAssociatedEntities() {
 		$table = TableRegistry::get('authors');
 		$table->hasOne('articles');
 
-		$this->assertSame($entity, $table->save($entity));
-		$this->assertFalse($entity->isNew());
+		$this->assertFalse($table->save($entity));
+		$this->assertTrue($entity->isNew());
 		$this->assertInternalType('array', $entity->article);
 	}
 
@@ -2637,14 +2637,15 @@ public function testSaveHasManyWithErrorsNonAtomic() {
 			->validator()
 			->add('title', 'num', ['rule' => 'numeric']);
 
-		$this->assertSame($entity, $table->save($entity, ['atomic' => false]));
-		$this->assertFalse($entity->isNew());
+		$result = $table->save($entity, ['atomic' => false]);
+		$this->assertFalse($result, 'Validation failed, no save.');
+		$this->assertTrue($entity->isNew());
 		$this->assertTrue($entity->articles[0]->isNew());
-		$this->assertFalse($entity->articles[1]->isNew());
-		$this->assertEquals(4, $entity->articles[1]->id);
+		$this->assertTrue($entity->articles[1]->isNew());
+		$this->assertNull($entity->articles[1]->id);
 		$this->assertNull($entity->articles[0]->id);
-		$this->assertEquals(5, $entity->articles[0]->author_id);
-		$this->assertEquals(5, $entity->articles[1]->author_id);
+		$this->assertNull($entity->articles[0]->author_id);
+		$this->assertNull($entity->articles[1]->author_id);
 	}
 
 /**
@@ -2670,8 +2671,9 @@ public function testSaveHasOneWithValidationErrorNonAtomic() {
 			->validator()
 			->add('title', 'num', ['rule' => 'numeric']);
 
-		$this->assertSame($entity, $table->save($entity, ['atomic' => false]));
-		$this->assertFalse($entity->isNew());
+		$result = $table->save($entity, ['atomic' => false]);
+		$this->assertFalse($result, 'Validation failed, no save.');
+		$this->assertTrue($entity->isNew());
 		$this->assertTrue($entity->article->isNew());
 		$this->assertNull($entity->article->id);
 		$this->assertNull($entity->article->get('author_id'));
@@ -2702,8 +2704,9 @@ public function testSaveBelongsToWithValidationErrorNotAtomic() {
 			->validator()
 			->add('name', 'num', ['rule' => 'numeric']);
 
-		$this->assertSame($entity, $table->save($entity, ['atomic' => false]));
-		$this->assertFalse($entity->isNew());
+		$result = $table->save($entity, ['atomic' => false]);
+		$this->assertFalse($result, 'Validation failed, no save');
+		$this->assertTrue($entity->isNew());
 		$this->assertTrue($entity->author->isNew());
 		$this->assertNull($entity->get('author_id'));
 		$this->assertNotEmpty($entity->author->errors('name'));
@@ -2802,7 +2805,7 @@ public function testSaveBelongsToManyDeleteSomeLinks() {
  * @group save
  * @return void
  */
-	public function testSaveBelongsToWithValidationErrorAtomic() {
+	public function testSaveBelongsToManyWithValidationErrorAtomic() {
 		$entity = new \Cake\ORM\Entity([
 			'title' => 'A Title',
 			'body' => 'A body'
@@ -2841,7 +2844,7 @@ public function testSaveBelongsToWithValidationErrorAtomic() {
  * @group save
  * @return void
  */
-	public function testSaveBelongsToWithValidationErrorNonAtomic() {
+	public function testSaveBelongsToManyWithValidationErrorNonAtomic() {
 		$entity = new \Cake\ORM\Entity([
 			'title' => 'A Title',
 			'body' => 'A body'
@@ -2861,15 +2864,16 @@ public function testSaveBelongsToWithValidationErrorNonAtomic() {
 			->validator()
 			->add('name', 'num', ['rule' => 'numeric']);
 
-		$this->assertSame($entity, $table->save($entity, ['atomic' => false]));
-		$this->assertFalse($entity->isNew());
+		$result = $table->save($entity, ['atomic' => false]);
+
+		$this->assertFalse($result, 'HABTM validation failed, save aborted');
+		$this->assertTrue($entity->isNew());
 		$this->assertTrue($entity->tags[0]->isNew());
-		$this->assertFalse($entity->tags[1]->isNew());
+		$this->assertTrue($entity->tags[1]->isNew());
 		$this->assertNull($entity->tags[0]->id);
-		$this->assertEquals(4, $entity->tags[1]->id);
+		$this->assertNull($entity->tags[1]->id);
 		$this->assertNull($entity->tags[0]->_joinData);
-		$this->assertEquals(4, $entity->tags[1]->_joinData->article_id);
-		$this->assertEquals(4, $entity->tags[1]->_joinData->tag_id);
+		$this->assertNull($entity->tags[1]->_joinData);
 	}
 
 /**
@@ -2878,7 +2882,7 @@ public function testSaveBelongsToWithValidationErrorNonAtomic() {
  * @group save
  * @return void
  */
-	public function testSaveBelongsToWithValidationErrorInJointEntity() {
+	public function testSaveBelongsToManyWithValidationErrorInJointEntity() {
 		$entity = new \Cake\ORM\Entity([
 			'title' => 'A Title',
 			'body' => 'A body'
@@ -2915,7 +2919,7 @@ public function testSaveBelongsToWithValidationErrorInJointEntity() {
  * @group save
  * @return void
  */
-	public function testSaveBelongsToWithValidationErrorInJointEntityNonAtomic() {
+	public function testSaveBelongsToManyWithValidationErrorInJointEntityNonAtomic() {
 		$entity = new \Cake\ORM\Entity([
 			'title' => 'A Title',
 			'body' => 'A body'
@@ -3068,7 +3072,7 @@ public function testSaveDeepAssociationOptions() {
 		$this->assertSame($entity, $articles->save($entity, [
 			'associated' => [
 				'authors' => [
-					'validate' => 'special'
+					'validate' => true
 				],
 				'authors.supervisors' => [
 					'atomic' => false,