Speed up is_{,in}valid

[iazel]

While utilizing this lib, I have encountered an error with validation of password, because I hashed the confirmation one directly in the validate method.
The user is something like:

class User extends ... {
private $password_confirm;
public function set_password($pass) {
    $this->assign_attribute('password', static::encrypt($pass));
}
public function validate() {
    $this->password_confirm = static::encrypt(password_confirm);
    if($this->password !== $this->password_confirm)
       $this->errors->add('password', 'Password mismatch');
}

And with this kind of code:

$user = User::create($params);
if( $user->is_invalid() )
    exit_with_error($user);

The if statement will be ALWAYS true. Why? It was not so clear at first blush.
The create method call _validate, but so do is_invalid, rehashing again password_confirm!
For my point of view, is_valid should only be a check and not a validate disguised by check. And so do this commit.

[Rican7]

is_{in}valid() method calls would have no idea that the model was valid or not without first "validating" the model and checking for any validation failures.

The problem here, seems to be more of an implementation problem than anything else. If you're trying to validate whether a user entered the same information twice in one form/call (like a "password" and "confirm password" form), you should check if the two strings are "identical" (as opposed to just equal) before setting the password in the model. Especially since doing two hash/encrypt calls would just unnecessarily slow things down.

Does this make sense? Let me know if you need any more clarification. :)

[iazel]

You are right, but if you see the change, the is_valid method now check if model already validated, if not then call _validate. Validation can also be forced with is_valid(true).
The use of case is just an example to show an eventually problem. Plus, checking password (or any other kind of information that requires a double check) when you set it, for me, is not the right way. And yes, I've used identical :)
I think business logic must reside in Model and that check if data is correct or not is a task of Validation.

However, this fix just speed up is_valid and help avoid that kind of bugs :)

[al-the-x]

I don't disagree with either @Rican7 or @iazel here. The is_valid() method should not call validate() again if validate() has already been called... Of course, when do you reset that is_validated flag? Explicitly, when the proposed clear_state() method (suggested rename: reset()?) is called. Implicitly, when a data attribute is modified, which might change its validation state.

@iazel, can you add some tests to describe this behavior (and passing $force_revalidate, too) and cover the implicit case -- property modification -- as well?

On an unrelated note, double underscored identifiers are generally taboo in PHP as they conflict with the "magic names" convention employed by the language, although I see they're used regularly in PHP-AR, due to the attempt to directly translate from the Ruby implementation, no doubt. I'll open a separate discussion ticket for that.

[al-the-x]

Well, I assume that means that @iazel isn't interested in pursuing this... I'll take swing at writing tests for his patch when I have some time.

[iazel]

No, don't worry, I've already done the tests, but have some issues running it ^^" I've closed this because it have been merged with the other pull request (regarding has many through). When tests run correctly, I will require another pull for both :)

I've also slightly changed the code and got your advice regarding the double underscored attributes. You can see the actual version on my fork (master branch).