For ease of use validate() should not throw an exception

[zsoerenm]

This is more a Feature Request: If I have to validate 2 Models in the same form, I will have to do the following:

try {
    $user->validate();
}
catch(Validate_Exception $e) {
    $error = true;
    Form::set_errors($e->array->errors('forms/user/register'));
}

try {
    $customer->validate();
}
catch(Validate_Exception $e) {
    $error = true;
    Form::set_errors($e->array->errors('forms/user/register'));
}

if(!isset($error)) {
    try {
        $user->save();
        $customer->set('user',$user->id());
        $customer->save();
        $this->request->redirect(Route::get('user')->uri());
    }
    catch(Validate_Exception $e) {
        $error = true;
        Form::set_errors($e->array->errors('forms/user/register'));
    }
}
else {
    Form::set_defaults($_POST);
}

This could be more simple when validate() is not throwing an exception:

if($user->validate() and $customer->validate()) {
    try {
        $user->save();
        $customer->set('user',$user->id());
        $customer->save();
        $this->request->redirect(Route::get('user')->uri());
    }
    catch(Validate_Exception $e) {
        Form::set_errors($e->array->errors('forms/user/register'));
    }
}
else {
    Form::set_errors($user->errors());
    Form::set_errors($customer->errors());
    Form::set_defaults($_POST);
}

[banks]

Possible options:

1. Change validate() to return boolean instead of throw exceptions and change save() to check this and throw the exception itself
2. Leave validate() as it is and add a method is_valid() which just returns a bool.

If anyone has an opinion, please let us know.

I'm still waiting to hear from Jon about where he is up to with the API changes etc.

[bmidget]

I happen to agree that validate() shouldn't throw exceptions. I personally don't think it's just an ease of use thing, but that there should be a difference between error messages and actual exceptions.

For instance, I think the idea that a form would throw an exception if it didn't pass validation rules is silly. It would return a boolean and create error messages. That's it.

Failed validation when saving should throw an exception, of course.

Thus, in my opinion option 1 is the best direction.

[zsoerenm]

100% agreed! Thanks bmidget

[loonies]

I'm currently testing this. If anyone is interested, here is the relevant commit: http://github.com/loonies/kohana-jelly/commit/0260bafeab29a23b121ba43ee38e017040c83512

[jonathangeiger]

Think I'm gonna go ahead with this.

[jonathangeiger]

Just making a note of a few places that I've noticed where issues may crop up. I can't really fix these right at this moment.

1. The :key context will not be properly set on the validator if you run validate() outside of save().
2. Empty data should return TRUE, not NULL.

[jonathangeiger]

I did merge your changes though loonies. Thank you very much for you work.

[loonies]

Hi

I'm playing with another approach in my experimental/validation branch: http://github.com/loonies/kohana-jelly/commits/experimental/validation/ It would be much better for validate to return model object instead of boolean.

This allows something like:

$user = Jelly::factory('User')
    ->set(...)
    ->validate(TRUE);

if ( ! $user->valid())
{
    // Do something
}

$user = Jelly::query('User', $id)
    ->select()
    ->set(...)
    ->validate();

if ( ! $user->valid())
{
    // Do something
}

Don't have time ATM, but I'll look at this further ASAP.

[banks]

I don't see the point of that - you've just added an extra method for no gain.

Why do you need to manually call validate(TRUE) if you are going to call a method called valid()? Just have the valid() method actually perform the validation...

[loonies]

Hi Banks.

For me it's more intuitive to work with objects like this :)

You are telling the model to run the validation on itself which will set the _valid flag on model, and then you query for the model's state with valid() method (just the way that loaded() or saved() works).

I can't remember the exact user case I had, but I do remember that I needed the model object as returning value from validate() method.

Anyway, I'm just experimenting and multi-model validation is still PITA :(

[jonathangeiger]

I agree with banks, loonies. You're welcome to open a separate issue to see if there's more support from the community for your ideas, but I'm closing this ticket for now.