Skip to content
This repository

1.3 saveAll() with validation option "only" or "first" works well with associated data. #210

Merged
merged 2 commits into from over 2 years ago

2 participants

Yosuke Basuke Suzuki ADmad
Yosuke Basuke Suzuki

saveAll() did set null foreign key when it just validates.

For example, I've assigned numeric validation on the hasMany side model and validation did fail with this behavior.

$data = array(
    'Post' => array(
        'title' => 'Hello world',
    ), 
    'Keyword' => array(
        array(
            'word' => 'foo',
        ),
        array(
            'word' => 'bar',
        ),
        ...
    ),
);

I've changed this not to set foreign key when it just validation.

Yosuke Basuke Suzuki Bug fixed. saveAll with validation option "only" or "first" works wll.
saveAll() did set null foreign key when it just validates. I've
assigned numeric validation on the hasMany side model and validation
did fail with this behavior. I've changed this not to set foreign key
when it just validation.
eef87ee
ADmad
Collaborator

Can you please add test cases for the bug you are describing.

Yosuke Basuke Suzuki

Sure. But how can I add new commit to this pull request? Do I have to send another pull request?

ADmad
Collaborator

Please check the github docs if its possible to add commit to existing pull request, I am not sure about it either. If not you can just close this request and make a new pull request.

Yosuke Basuke Suzuki

Thanks.

Yosuke Basuke Suzuki

ADmad, adding new commit to my branch automatically update this pull request! Great.

Yosuke Basuke Suzuki

This behavior was described just below the comment. #orz

 You can add more commits to this pull request by pushing to the 1.3 branch on basuke/cakephp
ADmad ADmad merged commit 9220837 into from
ADmad ADmad closed this
ADmad
Collaborator

Thank you for the patches. Can you please add the test cases for 2.0 branch too? Since saveAll was re-factored for 2.0 its unlikely the same bug exists there too, the test cases would confirm that.

Yosuke Basuke Suzuki

Sure I do. 2.0's saveAll is much readable. Thanks for refactoring ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

Sep 23, 2011
Yosuke Basuke Suzuki Bug fixed. saveAll with validation option "only" or "first" works wll.
saveAll() did set null foreign key when it just validates. I've
assigned numeric validation on the hasMany side model and validation
did fail with this behavior. I've changed this not to set foreign key
when it just validation.
eef87ee
Yosuke Basuke Suzuki Added unittest for eef87ee. d9c48b0
This page is out of date. Refresh to see the latest.
12  cake/libs/model/model.php
@@ -1687,7 +1687,10 @@ function saveAll($data = null, $options = array()) {
1687 1687
 					$type = $associations[$association];
1688 1688
 					switch ($type) {
1689 1689
 						case 'hasOne':
1690  
-							$values[$this->{$type}[$association]['foreignKey']] = $this->id;
  1690
+							if (!$validating) {
  1691
+								$values[$this->{$type}[$association]['foreignKey']] = $this->id;
  1692
+							}
  1693
+
1691 1694
 							if (!$this->{$association}->__save($values, $options)) {
1692 1695
 								$validationErrors[$association] = $this->{$association}->validationErrors;
1693 1696
 								$validates = false;
@@ -1697,9 +1700,12 @@ function saveAll($data = null, $options = array()) {
1697 1700
 							}
1698 1701
 						break;
1699 1702
 						case 'hasMany':
1700  
-							foreach ($values as $i => $value) {
1701  
-								$values[$i][$this->{$type}[$association]['foreignKey']] =  $this->id;
  1703
+							if (!$validating) {
  1704
+								foreach ($values as $i => $value) {
  1705
+									$values[$i][$this->{$type}[$association]['foreignKey']] =  $this->id;
  1706
+								}
1702 1707
 							}
  1708
+
1703 1709
 							$_options = array_merge($options, array('atomic' => false));
1704 1710
 
1705 1711
 							if ($_options['validate'] === 'first') {
42  cake/tests/cases/libs/model/model_validation.test.php
@@ -644,6 +644,48 @@ function testValidatesWithModelsAndSaveAll() {
644 644
 	}
645 645
 
646 646
 /**
  647
+ * test that saveAll and with models at initial insert (no id has set yet)
  648
+ * with validation interact well
  649
+ *
  650
+ * @return void
  651
+ */
  652
+	function testValidatesWithModelsAndSaveAllWithoutId() {
  653
+		$data = array(
  654
+			'Article' => array(
  655
+				'title' => 'Extra Fields',
  656
+				'body' => 'Extra Fields Body',
  657
+				'published' => '1'
  658
+			),
  659
+			'Comment' => array(
  660
+				array('word' => 'Hello'), 
  661
+				array('word' => 'World'), 
  662
+			)
  663
+		);
  664
+		$Article =& new Article();
  665
+		$Comment =& $Article->Comment;
  666
+
  667
+		$Comment->validate = array('article_id' => array('rule' => 'numeric'));
  668
+
  669
+		$Article->create();
  670
+		$result = $Article->saveAll($data, array('validate' => 'only'));
  671
+		$this->assertTrue($result);
  672
+
  673
+		$Article->create();
  674
+		$result = $Article->saveAll($data, array('validate' => 'first'));
  675
+		$this->assertTrue($result);
  676
+		$this->assertFalse(is_null($Article->id));
  677
+		
  678
+		$id = $Article->id;
  679
+		$count = $Article->find('count', array('conditions' => array('Article.id' => $id)));
  680
+		$this->assertIdentical($count, 1);
  681
+
  682
+		$count = $Comment->find('count', array(
  683
+			'conditions' => array('Comment.article_id' => $id)
  684
+		));
  685
+		$this->assertEqual($count, count($data['Comment']));
  686
+	}
  687
+
  688
+/**
647 689
  * Test that missing validation methods trigger errors in development mode.
648 690
  * Helps to make developement easier.
649 691
  *
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.