Skip to content

Compare error passwords in clientValidation patch #2026

Closed
wants to merge 11 commits into from

6 participants

@adminnu
adminnu commented Jan 22, 2013

dependentAttribute - attribute allows you to specify a field in which to validate the client along with the current.
I include this attribute for CCompareValidator, which gets rid of the errors in the absence of updates compare-fields.
#1910, #1925

Additionally removed the brackets from the conditions with only one operator is allowed to code style in js and reduces the file size.

@adminnu adminnu commented on an outdated diff Jan 22, 2013
framework/web/js/source/jquery.yiiactiveform.js
@@ -95,13 +88,13 @@
$.fn.yiiactiveform.validate($form, function (data) {
var hasError = false;
$.each(settings.attributes, function () {
- if (this.status === 2 || this.status === 3) {
+ if (this.dependentAttribute !== undefined && this.dependentAttribute == attribute.inputID)
@adminnu
adminnu added a note Jan 22, 2013

here include a check of the field-dependent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adminnu adminnu and 1 other commented on an outdated diff Jan 22, 2013
framework/web/widgets/CActiveForm.php
@@ -481,6 +480,8 @@ public function error($model,$attribute,$htmlOptions=array(),$enableAjaxValidati
{
if(($js=$validator->clientValidateAttribute($model,$attributeName))!='')
$validators[]=$js;
+ if(get_class($validator)=='CCompareValidator' && empty($option['dependentAttribute']) && !empty($validator->compareAttribute))
@adminnu
adminnu added a note Jan 22, 2013

then switched to attribute dependentAttribute CCompareValidator

@samdark
Yii Software LLC member
samdark added a note Jan 23, 2013

Why not to call it compareAttribute?

@adminnu
adminnu added a note Jan 23, 2013

It can be useful for other things when you have to simultaneously process multiple fields, not necessarily a comparison

@samdark
Yii Software LLC member
samdark added a note Jan 23, 2013

But it works with compare validator only, right?

@adminnu
adminnu added a note Jan 23, 2013

By default, it is included in CCompareValidator. But it can be turned on when necessary, such as the interval from-to, etc.

@samdark
Yii Software LLC member
samdark added a note Jan 23, 2013

OK. Makes sense.

@adminnu
adminnu added a note Jan 23, 2013

You can call it another way validateWich

@samdark
Yii Software LLC member
samdark added a note Jan 23, 2013

$validator instanceof CCompareValidator can be used instead of comparing strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mdomba mdomba was assigned Jan 23, 2013
@samdark samdark and 1 other commented on an outdated diff Jan 23, 2013
framework/web/js/source/jquery.yiiactiveform.js
@@ -17,9 +17,8 @@
var getAFValue = function (o) {
var type,
c = [];
- if (!o.length) {
@samdark
Yii Software LLC member
samdark added a note Jan 23, 2013

Brackets are needed in JS since when compressed in can go wrong if these are absent.

@adminnu
adminnu added a note Jan 23, 2013

Be right back braces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@samdark
Yii Software LLC member
samdark commented Jan 23, 2013

Something happened with JS diff. It's all deleted-all added. Impossible to tell what was changed.

@adminnu
adminnu commented Jan 23, 2013

is correct, added crlf instead of lf to transfer

@adminnu
adminnu commented Jan 23, 2013

repaired

@samdark samdark commented on an outdated diff Jan 23, 2013
framework/web/js/source/jquery.yiiactiveform.js
@@ -87,6 +87,9 @@
}
if (attribute.beforeValidateAttribute === undefined || attribute.beforeValidateAttribute($form, attribute)) {
$.each(settings.attributes, function () {
+ if (this.dependentAttribute !== undefined && this.dependentAttribute == attribute.inputID) {
@samdark
Yii Software LLC member
samdark added a note Jan 23, 2013

this.dependentAttribute !== undefined can be just this.dependentAttribute. Also I think you can safely use === for comparing two strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@samdark samdark commented on an outdated diff Jan 23, 2013
framework/web/widgets/CActiveForm.php
@@ -481,6 +480,8 @@ public function error($model,$attribute,$htmlOptions=array(),$enableAjaxValidati
{
if(($js=$validator->clientValidateAttribute($model,$attributeName))!='')
$validators[]=$js;
+ if(get_class($validator)=='CCompareValidator' && empty($option['dependentAttribute']) && !empty($validator->compareAttribute))
+ $option['dependentAttribute']=CHtml::activeId($model, $validator->compareAttribute);
@samdark
Yii Software LLC member
samdark added a note Jan 23, 2013

Extra space after ,.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adminnu
adminnu commented Jan 23, 2013

By the way, that with regards to passwords.
They should be written in the reverse order in the validator:
array ('cPassword', 'compare', 'compareAttribute' => 'password')),
if the fields are in order: First, the field password, then cPassword.
Then delete items that when we fill the first field, the second comes out unfilled error.

@samdark
Yii Software LLC member
samdark commented Jan 23, 2013

That's smart indeed.

@samdark
Yii Software LLC member
samdark commented Jan 23, 2013

Now pull-request looks good for me. @mdomba what do you think?

@adminnu
adminnu commented Jan 23, 2013

I think I came up with is if we have the first editable field dependent, we can turn off the launch of the second field test, as it has the status is undefined.
Second, now I will do.

@adminnu adminnu commented on the diff Jan 23, 2013
framework/web/js/source/jquery.yiiactiveform.js
@@ -87,6 +87,9 @@
}
if (attribute.beforeValidateAttribute === undefined || attribute.beforeValidateAttribute($form, attribute)) {
$.each(settings.attributes, function () {
+ if (this.dependentAttribute === attribute.inputID && this.status !== undefined) {
@adminnu
adminnu added a note Jan 23, 2013

Added && this.status !== undefined
It so happens when we change the dependent field with a beginning, there is no need to validate the fields on which depend if it is not filled, that is, when its status is undefined

In the opposite direction does not work, since it is already in the test ccomparevalidator or in others where it will be used.
For passwords just fit place compare in reverse order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mdomba
Yii Software LLC member
mdomba commented Jan 23, 2013

Problem is that you are adding a new property (dependentAttribute) and new code to the whole form processing so to solve one validator only... this way this property is added to all form items even if CCompareValidator is not used...

Also this validator can be used for different things adn teh operator can be changed - it's not used only for comparing if password_repeat is equal to password.

@adminnu
adminnu commented Jan 23, 2013

Problem is that you are adding a new property (dependentAttribute) and new code to the whole form processing so to solve one validator only... this way this property is added to all form items even if CCompareValidator is not used...

This property is added to the form ellement only if you specify it yourself or CCompareValidator.

Also this validator can be used for different things adn teh operator can be changed - it's not used only for comparing if password_repeat is equal to password.

Yes, of course. This allows you to update the validation for the dependent field.

To put this in context, only CCompareValidator, then the client validation for this validator at the moment is absolutely meaningless because it is not completed yet, as can be compared 2-fields, but the error may affect only one of the field.

@mdomba
Yii Software LLC member
mdomba commented Jan 23, 2013

I think you did not understand my points...

The property is added for every form item options on the client side.

The validator can be used for different things and different operator can be used (not only equal)... yet you added in your PR this code if (this.dependentAttribute === attribute.inputID ...

The validator as it is now, without making any change can be made working

@mdomba
Yii Software LLC member
mdomba commented Jan 23, 2013

OK let's get from the beginning. With current Yii code (not PR code) !

Let's use a new generated Yii webapp and the login form..

We add to models/LoginForm a new field public $password_repeat;

On the view file views/site/login.php we enable ajax and client validation with:

<?php $form=$this->beginWidget('CActiveForm', array(
    'id'=>'login-form',
    'enableAjaxValidation'=>true,
    'enableClientValidation'=>true,
)); ?>

and we add a new input for password repeat after the password input field, with

        <?php echo $form->labelEx($model,'password'); ?>
        <?php echo $form->passwordField($model,'password'); ?>
        <?php echo $form->error($model,'password'); ?>
        <?php echo $form->labelEx($model,'password_repeat'); ?>
        <?php echo $form->passwordField($model,'password_repeat'); ?>
        <?php echo $form->error($model,'password_repeat'); ?>

Now, we need to add a validation rule to models/LoginForm

If we add a classic rule: array('password', 'compare', 'skipOnError'=>true),

There is the first problem that when we leave the password field and go to the next input (password repeat) because of client/ajax validation we will instantly get an error for the password field that say Password must be repeated exactly. This is not good as the password repeat has not been entered at all, we just got to that field so we can enter it.
And the second problem is that whatever we now enter in the password_repeat field the above message stays there.

So the most elegant solution I can see here to fix both problems is to set the compare validation on the password_repeat field instead on the password field.

So by removing the previous validation rule and by adding the rule
array('password_repeat', 'compare', 'skipOnError'=>true, 'compareAttribute'=>'password'),

we solve the problem with the "Password must be repeated exactly" message.

And if you think about it... it makes more sense that this error message is shown on the "repeat" field and only when the password is entered for the second time.

....

Problem is that with ajax validation, and if you enter a wrong password you will get the message "password is invalid" even before the password_repeat" is entered, so again this is not good as we want to check for proper password only after the password is repeated.

To solve this we should change the order of those fields:

In the views/site/login.php

        <?php echo $form->labelEx($model,'password_repeat'); ?>
        <?php echo $form->passwordField($model,'password_repeat'); ?>
        <?php echo $form->error($model,'password_repeat'); ?>
        <?php echo $form->labelEx($model,'password'); ?>
        <?php echo $form->passwordField($model,'password'); ?>
        <?php echo $form->error($model,'password'); ?>

and in the models/LoginForm

array('password', 'compare', 'skipOnError'=>true),

With this all problems are solved. The only thing remaining to fix are the labels but that is done in a second.

So is this working for you ?

@adminnu
adminnu commented Jan 23, 2013

@mdomba

With this all problems are solved. The only thing remaining to fix are the labels but that is done in a second.

No. Not so. What you have written, I have described above #2026 (comment) , the change order flogged.
This solves one problem only if the first field filled and the other empty. Yes, it is solved by an inversion in the rules of the two fields.
I pull solves another problem, he solves the problem, if it turned out that two password filled in different ways and the man decided to make a change in the first field, and not in the second, then the error in the second field is not updated, here we consider only this case.
Therefore, this pull is for client validation, since Ajax validation is no such problem.

Approx. do not take the password, let it be two fields with the price and the first field must be greater than the second one, we have one, the error is, I have a second, there is no reaction, which is not correct.

@nsanden
nsanden commented Jan 23, 2013

@mdomba - that is almost exactly the method I have been using and it works for the most part but it isn't perfect like adminnu says. For example, what if I enter my password as test12 and test123. I wanted test123 but well they don't match, so I have to go back and fix the first one, but changing that one will not trigger validation again, so it will continue to say passwords do not match.

Maybe i'm expecting too much but if we can make it work without too much bloat or other complications (looks like 2 different people submitted a pull request for this) why not?

@fiesh
fiesh commented Feb 2, 2013

+1, this may be a "design behaviour" as it was put by Qiang, still the client validator should be triggered by the attribute it references too.

@cebe cebe modified the milestone: 1.1.16, 1.1.17 Dec 21, 2014
adminnu added some commits Jun 1, 2015
@adminnu adminnu summary
description
fe962fd
@adminnu adminnu Revert "summary"
This reverts commit fe962fd.
f695b7e
@samdark samdark closed this Jan 3, 2016
@cebe cebe removed this from the 1.1.17 milestone Jan 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.