New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.3, 2.4, isUnique with multiple fields is not working, or am I missing something? #4571

Closed
spooknick opened this Issue Sep 10, 2014 · 10 comments

Comments

Projects
None yet
5 participants
@spooknick

spooknick commented Sep 10, 2014

The isUnique rule is not working when defined as a validation rule but it works when used as method. I am missing something?

I tried the following but it only validates the field to which it is attached.

    public $validate = array(
        'field1' => array(
            'unique' => array(
                'rule' => array('isUnique', array('field1', 'field2'), false),
                'message' => 'This field1 & field2 combination has already been used.',
            ),
        ),
    );

On the other hand, using isUnique method in a custom validation rule works

    public $validate = array(
        'field1' => array(
            'unique' => array(
                'rule' => array('checkUnique', array('field1', 'field2'), false),
                'message' => 'This field1 & field2 combination has already been used.',
            ),
        ),
    );

    public function checkUnique($ignoredData, $fields, $or = true) {
            return $this->isUnique($fields, $or);
        }
@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Sep 10, 2014

Member

Note the documentation about how to validate dependent fields:
http://book.cakephp.org/2.0/en/models/data-validation.html#Model::Validation::isUnique

The field itself should not be contained in the $fields array.

Member

dereuromark commented Sep 10, 2014

Note the documentation about how to validate dependent fields:
http://book.cakephp.org/2.0/en/models/data-validation.html#Model::Validation::isUnique

The field itself should not be contained in the $fields array.

@spooknick

This comment has been minimized.

Show comment
Hide comment
@spooknick

spooknick Sep 10, 2014

While I'm at it, the $or param is missing in the documentation

Also, I tend to find it strange to have it default to true. I would think that uniqueness is defined by a sum of difference. If we take 2 persons, they are not similar if they share only their firstname, but they maybe identical is they share the firstname and the lastname and whatever.

spooknick commented Sep 10, 2014

While I'm at it, the $or param is missing in the documentation

Also, I tend to find it strange to have it default to true. I would think that uniqueness is defined by a sum of difference. If we take 2 persons, they are not similar if they share only their firstname, but they maybe identical is they share the firstname and the lastname and whatever.

@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Sep 10, 2014

Member

Also, I tend to find it strange to have it default to true.

You have a point, but since we need to maintain backwards compatibility we cannot change it in 2.x. The equivalent Table::validateUnique() in 3.0 doesn't use OR.

Member

ADmad commented Sep 10, 2014

Also, I tend to find it strange to have it default to true.

You have a point, but since we need to maintain backwards compatibility we cannot change it in 2.x. The equivalent Table::validateUnique() in 3.0 doesn't use OR.

@ravage84

This comment has been minimized.

Show comment
Hide comment
@ravage84
Member

ravage84 commented Sep 10, 2014

@spooknick may be my plugin https://github.com/ravage84/MultiColumnUniqueness can ease your pain.

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Sep 10, 2014

Member

I never used the 2.x core method for dependent fields, but an AppModel extension.
I disliked to have to additionally name the dependent field again in the fields array besides other mentioned things above.
But most of that will probably be fixed with 3.0 then.

As for the docs, maybe you can make a PR with your suggested changes.

Member

dereuromark commented Sep 10, 2014

I never used the 2.x core method for dependent fields, but an AppModel extension.
I disliked to have to additionally name the dependent field again in the fields array besides other mentioned things above.
But most of that will probably be fixed with 3.0 then.

As for the docs, maybe you can make a PR with your suggested changes.

@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Sep 14, 2014

Member

Given that the method 'works' albeit a bit oddly is there anything to be done here?

Member

markstory commented Sep 14, 2014

Given that the method 'works' albeit a bit oddly is there anything to be done here?

@spooknick

This comment has been minimized.

Show comment
Hide comment
@spooknick

spooknick Sep 14, 2014

The method works but not has a validation rule. So maybe it should be
removed from the manual or show how it can be used, because the example
doesn't work.

2014-09-14 15:52 GMT+02:00 Mark Story notifications@github.com:

Given that the method 'works' albeit a bit oddly is there anything to be
done here?


Reply to this email directly or view it on GitHub
#4571 (comment).

spooknick commented Sep 14, 2014

The method works but not has a validation rule. So maybe it should be
removed from the manual or show how it can be used, because the example
doesn't work.

2014-09-14 15:52 GMT+02:00 Mark Story notifications@github.com:

Given that the method 'works' albeit a bit oddly is there anything to be
done here?


Reply to this email directly or view it on GitHub
#4571 (comment).

@dereuromark

This comment has been minimized.

Show comment
Hide comment
@dereuromark

dereuromark Sep 15, 2014

Member

@spooknick So you are saying the example in the docs doesnt work?
Whats the produced SQL query?
Aren't there existing tests that proof that it works? Maybe that one is missing then.

Member

dereuromark commented Sep 15, 2014

@spooknick So you are saying the example in the docs doesnt work?
Whats the produced SQL query?
Aren't there existing tests that proof that it works? Maybe that one is missing then.

@spooknick

This comment has been minimized.

Show comment
Hide comment
@spooknick

spooknick Sep 15, 2014

@dereuromark Yes I do say that the example doesn't work.
I haven't found any tests for the multi field validation, but I am far from knowing CakePHP as you guys are, so I might have missed it. I guess the rule is not parsed well.

Regarding SQL query:

using isUnique as a validation rule as here below

    public $validate = array(
        'f1' => array(
            'unique' => array(
                'rule' => array('isUnique', array('f1', 'f2'), false),
                'message' => 'A company must be selected',
            ),
        ),
    );

Generates the following incorrect SQL

SELECT COUNT(*) AS `count` FROM `bimol`.`test_articles` AS `Article` 
WHERE `Article`.`f1` = 'field1'

Were as using isUnique as a method

    public $validate = array(
        'f1' => array(
            'unique' => array(
                'rule' => array('checkUnique', array('f1', 'f2'), false),
                'message' => 'A company must be selected',
            ),
        ),
    );

    public function checkUnique($ignoredData, $fields, $or = false) {
        return $this->isUnique($fields, $or);
    }

Generates the following correct SQL

SELECT COUNT(*) AS `count` FROM `bimol`.`test_articles` AS `Article` 
WHERE `Article`.`f1` = 'field1' AND `Article`.`f2` = 'field2'

spooknick commented Sep 15, 2014

@dereuromark Yes I do say that the example doesn't work.
I haven't found any tests for the multi field validation, but I am far from knowing CakePHP as you guys are, so I might have missed it. I guess the rule is not parsed well.

Regarding SQL query:

using isUnique as a validation rule as here below

    public $validate = array(
        'f1' => array(
            'unique' => array(
                'rule' => array('isUnique', array('f1', 'f2'), false),
                'message' => 'A company must be selected',
            ),
        ),
    );

Generates the following incorrect SQL

SELECT COUNT(*) AS `count` FROM `bimol`.`test_articles` AS `Article` 
WHERE `Article`.`f1` = 'field1'

Were as using isUnique as a method

    public $validate = array(
        'f1' => array(
            'unique' => array(
                'rule' => array('checkUnique', array('f1', 'f2'), false),
                'message' => 'A company must be selected',
            ),
        ),
    );

    public function checkUnique($ignoredData, $fields, $or = false) {
        return $this->isUnique($fields, $or);
    }

Generates the following correct SQL

SELECT COUNT(*) AS `count` FROM `bimol`.`test_articles` AS `Article` 
WHERE `Article`.`f1` = 'field1' AND `Article`.`f2` = 'field2'

@markstory markstory modified the milestones: 2.5.5, 2.5.6 Oct 5, 2014

markstory added a commit that referenced this issue Oct 10, 2014

Fix Model::isUnique() not working as a validator.
While it *did* work for single fields, isUnique could not be used to
validate the uniqueness across multiple fields as documented. Because
validation methods pass arguments in an order the validator did not
expect the validation method would not work correctly.

Fixes #4571
@markstory

This comment has been minimized.

Show comment
Hide comment
@markstory

markstory Oct 10, 2014

Member

Pull request up now.

Member

markstory commented Oct 10, 2014

Pull request up now.

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