Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix errors when validateUnique is used with scope.
Fix validateUnique() causing errors when used as a validator with
additional scope.

Fixes #5652
  • Loading branch information
markstory committed Jan 13, 2015
1 parent c636777 commit 28462ca
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 2 deletions.
8 changes: 6 additions & 2 deletions src/ORM/Table.php
Expand Up @@ -1961,11 +1961,15 @@ public function patchEntities($entities, array $data, array $options = [])
* the data to be validated.
*
* @param mixed $value The value of column to be checked for uniqueness
* @param array $options The options array, optionally containing the 'scope' key
* @param array $context Either the options or validation context.
* @param array|null $options The options array, optionally containing the 'scope' key
* @return bool true if the value is unique
*/
public function validateUnique($value, array $options)
public function validateUnique($value, array $context, array $options = null)
{
if ($options === null) {
$options = $context;

This comment has been minimized.

Copy link
@dereuromark

dereuromark Jan 26, 2015

Member

Not sure if that joggling is good per se.
Maybe we should always attach the context as last argument to be safe?

validateUnique($value, array $options, array $context)

Other validation methods could follow the same pattern:

validateSomething($value, array $foo, array $bar, array $context)
}
$entity = new Entity(
$options['data'],
['useSetters' => false, 'markNew' => $options['newRecord']]
Expand Down
21 changes: 21 additions & 0 deletions tests/TestCase/ORM/TableTest.php
Expand Up @@ -3338,4 +3338,25 @@ public function testValidateUnique()
$data = ['username' => 'larry'];
$this->assertNotEmpty($validator->errors($data, false));
}

/**
* Tests the validateUnique method with scope
*
* @return void
*/
public function testValidateUniqueScope()
{
$table = TableRegistry::get('Users');
$validator = new Validator;
$validator->add('username', 'unique', [
'rule' => ['validateUnique', ['scope' => 'id']],
'provider' => 'table'
]);
$validator->provider('table', $table);
$data = ['username' => 'larry'];
$this->assertNotEmpty($validator->errors($data));

$data = ['username' => 'jose'];
$this->assertEmpty($validator->errors($data));
}
}

10 comments on commit 28462ca

@dereuromark
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You switched the arguments, but the docs are still wrong (doc block).

I tried using scope but now it doesn't work anymore. scope is in $context['scope'] not $options['scope'].

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test case shows scope working as a validator though.

@dereuromark
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text case might work because the strict check applies for null being passed, If it is an empty array or alike it might be different. At least in my real life scenario the order was wrong.

I hotfixed it merging the params context and option for now, but we should probably into it.

@jonlachmann
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used RC1 and got the error here. Then I updated to RC2, now everything seems to work at first, but it does not consider the scope I assign the rule. It marks my email as invalid even though it is unique (to that scope, but not globally in the table).

Below is the code I have added to my table model. Am I stupid and doing something wrong? Or is this a bug? Best regards Jon

->add('email', 'unique', [
    'rule' => [
        'validateUnique', [
            'scope' => 'survey_id']],
    'provider' => 'table'])

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like it should work based on the tests cases. What queries is the validation rule emitting?

@jonlachmann
Copy link

@jonlachmann jonlachmann commented on 28462ca Feb 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the query that is only present when I have the rule activated, no notion of survey_id at all.

SELECT 1 AS `existing` FROM participants Participants WHERE email = '***@***.**' LIMIT 1

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I will take another look. The tests could be wrong.

@jonlachmann
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you able to replicate the problem? Or do you need more input?

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and fix has been merged into 3.0.

@jonlachmann
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful! Thanks for the quick remedy of this problem, and for CakePHP in all. Its really a great framework! =D

Please sign in to comment.