Fix saveAll failed to save array that last entry have validation error #1058

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@xcompass

xcompass commented Jan 6, 2013

See the added test case for details

@lorenzo

This comment has been minimized.

Show comment Hide comment
@lorenzo

lorenzo Jan 6, 2013

Member

You need to use 'validate' => true instead of first

Member

lorenzo commented Jan 6, 2013

You need to use 'validate' => true instead of first

@lorenzo lorenzo closed this Jan 6, 2013

@xcompass

This comment has been minimized.

Show comment Hide comment
@xcompass

xcompass Jan 6, 2013

But the behaviour of:
$TestModel->validate = array('title' => 'notEmpty');
$result = $TestModel->saveAll(
array(
array('title' => 'title 1'),
array('title' => 'title 2'),
array('title' => ''),
),
array('validate'=>'first', 'atomic' => false)
);

and
$TestModel->validate = array('title' => 'notEmpty');
$result = $TestModel->saveAll(
array(
array('title' => 'title 1'),
array('title' => ''),
array('title' => 'title 2'),
),
array('validate'=>'first', 'atomic' => false)
);

are completely different!. The first piece of the code saves nothing and
the second one saves title 1 and title 2.

Cheers,
Pan

On Sat, Jan 5, 2013 at 5:00 PM, José Lorenzo Rodríguez <
notifications@github.com> wrote:

You need to use 'validate' => true instead of first


Reply to this email directly or view it on GitHubhttps://github.com/cakephp/cakephp/pull/1058#issuecomment-11922552.

xcompass commented Jan 6, 2013

But the behaviour of:
$TestModel->validate = array('title' => 'notEmpty');
$result = $TestModel->saveAll(
array(
array('title' => 'title 1'),
array('title' => 'title 2'),
array('title' => ''),
),
array('validate'=>'first', 'atomic' => false)
);

and
$TestModel->validate = array('title' => 'notEmpty');
$result = $TestModel->saveAll(
array(
array('title' => 'title 1'),
array('title' => ''),
array('title' => 'title 2'),
),
array('validate'=>'first', 'atomic' => false)
);

are completely different!. The first piece of the code saves nothing and
the second one saves title 1 and title 2.

Cheers,
Pan

On Sat, Jan 5, 2013 at 5:00 PM, José Lorenzo Rodríguez <
notifications@github.com> wrote:

You need to use 'validate' => true instead of first


Reply to this email directly or view it on GitHubhttps://github.com/cakephp/cakephp/pull/1058#issuecomment-11922552.

@dereuromark

This comment has been minimized.

Show comment Hide comment
@dereuromark

dereuromark Jan 6, 2013

Member

well, with validate=>first it should not save either one of them, of course, in 1.3. At least from what I remember about "first".

Member

dereuromark commented Jan 6, 2013

well, with validate=>first it should not save either one of them, of course, in 1.3. At least from what I remember about "first".

@xcompass

This comment has been minimized.

Show comment Hide comment
@xcompass

xcompass Jan 6, 2013

Even with atomic => false?

Cheers,
Pan

On Sun, Jan 6, 2013 at 3:29 AM, Mark notifications@github.com wrote:

well, with validate=>first it should not save either one of them, of
course.


Reply to this email directly or view it on GitHubhttps://github.com/cakephp/cakephp/pull/1058#issuecomment-11927206.

xcompass commented Jan 6, 2013

Even with atomic => false?

Cheers,
Pan

On Sun, Jan 6, 2013 at 3:29 AM, Mark notifications@github.com wrote:

well, with validate=>first it should not save either one of them, of
course.


Reply to this email directly or view it on GitHubhttps://github.com/cakephp/cakephp/pull/1058#issuecomment-11927206.

@ADmad

This comment has been minimized.

Show comment Hide comment
@ADmad

ADmad Jan 6, 2013

Member

'validate' => 'first' with 'atomic' => false is kinda pointless. If you want only those records passing validation to be inserted/updated and get errors back for records failing validation just use 'validate' => true with 'atomic' => false.

Member

ADmad commented Jan 6, 2013

'validate' => 'first' with 'atomic' => false is kinda pointless. If you want only those records passing validation to be inserted/updated and get errors back for records failing validation just use 'validate' => true with 'atomic' => false.

@xcompass

This comment has been minimized.

Show comment Hide comment
@xcompass

xcompass Jan 8, 2013

I understand 'validate' => 'first' with 'atomic' => false is basically the
same as 'validate' => 'true' with 'atomic' => false. However, the default
value for validate is set to 'first'. So when people use saveAll($data,
array('atomic' => false')) will be saveAll($data, array('validate' =>
'first', 'atomic' => false')), hence trigger the bug when the last entry is
invalid, nothing get saved.

I think the 'validate' => 'first' and 'atomic' => false should behave the
same as 'validate' => 'true' and 'atomic' => false. so the people
only specify atomic => false in saveAll will not get tripped by this bug.

Cheers,
Pan

On Sun, Jan 6, 2013 at 10:32 AM, ADmad notifications@github.com wrote:

'validate' => 'first' with 'atomic' => false is kinda pointless. If you
want only those records passing validation to be inserted/updated and get
errors back for records failing validation just use 'validate' => true with
'atomic' => false.


Reply to this email directly or view it on GitHubhttps://github.com/cakephp/cakephp/pull/1058#issuecomment-11932086.

xcompass commented Jan 8, 2013

I understand 'validate' => 'first' with 'atomic' => false is basically the
same as 'validate' => 'true' with 'atomic' => false. However, the default
value for validate is set to 'first'. So when people use saveAll($data,
array('atomic' => false')) will be saveAll($data, array('validate' =>
'first', 'atomic' => false')), hence trigger the bug when the last entry is
invalid, nothing get saved.

I think the 'validate' => 'first' and 'atomic' => false should behave the
same as 'validate' => 'true' and 'atomic' => false. so the people
only specify atomic => false in saveAll will not get tripped by this bug.

Cheers,
Pan

On Sun, Jan 6, 2013 at 10:32 AM, ADmad notifications@github.com wrote:

'validate' => 'first' with 'atomic' => false is kinda pointless. If you
want only those records passing validation to be inserted/updated and get
errors back for records failing validation just use 'validate' => true with
'atomic' => false.


Reply to this email directly or view it on GitHubhttps://github.com/cakephp/cakephp/pull/1058#issuecomment-11932086.

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