Added support for simplified required validation on 'create' or 'update'. Fixes #230. #467

Merged
merged 2 commits into from Feb 22, 2012

Projects

None yet

4 participants

@tPl0ch
tPl0ch commented Feb 7, 2012

Added testcases for new 'required' attributes and conflicts with the 'on' key.

Thomas Ploch Added support for simplified required validation on 'create' or 'upda…
…te'. Added testcases for new 'required' attributes and conflicts with the 'on' key. Fixes #230.
4f3b266
@markstory
Member

I think the changes seem good. :) Ideally if someone makes conflicting on/required settings we should trigger a warning but I don't know if that is a must have.

@tPl0ch
tPl0ch commented Feb 7, 2012

@markstory I don't think that the warning is nessecary since the 'on' key will just skip the rule as intended, and if the 'required' is set to a conflicting value, it will just be ignored as intended by the 'on' key.

And I mean this just looks wrong on first glance:

$Model->validate = array(
    'title' => array(
        'required' => 'update',
        'on' => 'create'    
    )
);
@lorenzo
Member
lorenzo commented Feb 7, 2012

Changes looks good to me, I don't think an error is necessary in this case.

@lorenzo lorenzo commented on the diff Feb 7, 2012
lib/Cake/Model/Model.php
if ($exists === null) {
$exists = $this->exists();
}
- if (($validator['on'] == 'create' && $exists) || ($validator['on'] == 'update' && !$exists)) {
+ if ($validator['on'] == 'create' && $exists || $validator['on'] == 'update' && !$exists) {
@lorenzo
lorenzo Feb 7, 2012 CakePHP member

I think you need to do strict comparison when checking the 'required' value, as it may take a boolean (default value is true in historic apps). All those comparisons will evaluate to true, unfortunately.

@tPl0ch
tPl0ch Feb 7, 2012

@lorenzo Well, yeah, as well as the in_array() check needs to be set to strict checking then. Gonna commit it in a second. But you commented on the wrong line since this is the comparison for the 'on' key :-)

@markstory
Member

I think this changeset looks good, any objections to merging it into 2.1?

@predominant
Member

Do want.

@lorenzo
Member
lorenzo commented Feb 22, 2012

Looks good to me

On Feb 21, 2012, at 22:07, Graham Weldon
reply@reply.github.com
wrote:

Do want.


Reply to this email directly or view it on GitHub:
#467 (comment)

@markstory
Member

I've already updated the docs :)

@markstory markstory merged commit b730285 into cakephp:2.1 Feb 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment