Fixes #1955: Some validators used to cause warnings or errors in case non-scalar array typed values being checked. #2186

Merged
merged 4 commits into from Mar 10, 2013

Conversation

Projects
None yet
5 participants
@resurtm
Contributor

resurtm commented Mar 9, 2013

Fixes #1955: Some validators used to cause warnings or errors in case non-scalar array typed values being checked.

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Mar 9, 2013

Owner

Looks good to me. The only thing I think may be wrong are messages.

Owner

samdark commented Mar 9, 2013

Looks good to me. The only thing I think may be wrong are messages.

@ghost ghost assigned samdark Mar 9, 2013

@resurtm

This comment has been minimized.

Show comment Hide comment
@resurtm

resurtm Mar 10, 2013

Contributor

The only thing I think may be wrong are messages.

What's wrong with them? Do you mean they should be customizable from validator configuration?
(But generally they work fine.)

Contributor

resurtm commented Mar 10, 2013

The only thing I think may be wrong are messages.

What's wrong with them? Do you mean they should be customizable from validator configuration?
(But generally they work fine.)

@mdomba

This comment has been minimized.

Show comment Hide comment
@mdomba

mdomba Mar 10, 2013

Member

The error mesage of a validator should always the "[custom] validator message" (like "username is not right") not the one you put "array not allowed"

Member

mdomba commented Mar 10, 2013

The error mesage of a validator should always the "[custom] validator message" (like "username is not right") not the one you put "array not allowed"

@resurtm

This comment has been minimized.

Show comment Hide comment
@resurtm

resurtm Mar 10, 2013

Contributor

I changed error messages (they became human friendly), but let me explain each validator separately:

CExistValidator:
Default error message string: '{attribute} "{value}" is invalid.'
My error message string: '{attribute} is invalid.'
Default error message string won't fit, so i decided to introduce new message.

CNumberValidator:
Default error message string: '{attribute} must be a number.'
This message is acceptable so i used it for array value case.

CRegularExpressionValidator:
Default error message string: '{attribute} is invalid.'
I left it unchanged too.

CStringValidator:
Default error message string: '{attribute} is of the wrong length (should be {length} characters).'
My error message string: '{attribute} is invalid.'
Default message won't fit too.

CUniqueValidator:
Default error messge string: '{attribute} "{value}" has already been taken.'
My error message string: '{attribute} is invalid.'
Default message won't fit too.

Contributor

resurtm commented Mar 10, 2013

I changed error messages (they became human friendly), but let me explain each validator separately:

CExistValidator:
Default error message string: '{attribute} "{value}" is invalid.'
My error message string: '{attribute} is invalid.'
Default error message string won't fit, so i decided to introduce new message.

CNumberValidator:
Default error message string: '{attribute} must be a number.'
This message is acceptable so i used it for array value case.

CRegularExpressionValidator:
Default error message string: '{attribute} is invalid.'
I left it unchanged too.

CStringValidator:
Default error message string: '{attribute} is of the wrong length (should be {length} characters).'
My error message string: '{attribute} is invalid.'
Default message won't fit too.

CUniqueValidator:
Default error messge string: '{attribute} "{value}" has already been taken.'
My error message string: '{attribute} is invalid.'
Default message won't fit too.

samdark added a commit that referenced this pull request Mar 10, 2013

Merge pull request #2186 from resurtm/fixes-1955
Fixes #1955: Some validators used to cause warnings or errors in case non-scalar array typed values being checked.

@samdark samdark merged commit 1e3a178 into yiisoft:master Mar 10, 2013

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Mar 10, 2013

Owner

Merged. Thanks for working on this one.

Owner

samdark commented Mar 10, 2013

Merged. Thanks for working on this one.

@resurtm

This comment has been minimized.

Show comment Hide comment
@resurtm

resurtm Mar 10, 2013

Contributor

Though i'll test it more before 1.1.14 release.

Contributor

resurtm commented Mar 10, 2013

Though i'll test it more before 1.1.14 release.

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Mar 10, 2013

Owner

We plan doing RC like last time. As I can see, unit tests are covering the cases I'd test very well.

Owner

samdark commented Mar 10, 2013

We plan doing RC like last time. As I can see, unit tests are covering the cases I'd test very well.

@mdomba

This comment has been minimized.

Show comment Hide comment
@mdomba

mdomba Mar 10, 2013

Member

@samdark you are too fast with the merge :D

Problem is that here we are now forcing a message that is not customized. If a custom validator message is set it would be expected to get that message and not a different now.

As it's pointed before, this "array error" can happen only when someone mess with the URL... I think that in that case we should throw an exception and not set a fixed message.

Member

mdomba commented Mar 10, 2013

@samdark you are too fast with the merge :D

Problem is that here we are now forcing a message that is not customized. If a custom validator message is set it would be expected to get that message and not a different now.

As it's pointed before, this "array error" can happen only when someone mess with the URL... I think that in that case we should throw an exception and not set a fixed message.

@resurtm

This comment has been minimized.

Show comment Hide comment
@resurtm

resurtm Mar 10, 2013

Contributor

I think that in that case we should throw an exception and not set a fixed message.

I thought about exceptions in the beginning, but purely subjectively stopped on usual error messages (i was wondering what you will think about it). But yes, i share your opinion on throwing exceptions, since it's no doubt far more an exceptional situation rather than usual (some validators are not intended to be used with arrays).

@samdark, do you agree?

(However i'm preparing fixing PR.)

Contributor

resurtm commented Mar 10, 2013

I think that in that case we should throw an exception and not set a fixed message.

I thought about exceptions in the beginning, but purely subjectively stopped on usual error messages (i was wondering what you will think about it). But yes, i share your opinion on throwing exceptions, since it's no doubt far more an exceptional situation rather than usual (some validators are not intended to be used with arrays).

@samdark, do you agree?

(However i'm preparing fixing PR.)

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Mar 10, 2013

Owner

I'm against throwing exceptions in validators. Any value passed should either generate an error or be considered as valid. As for messages, I think array case is special and there's no point in customizing this message that only "too smart" people will see.

Owner

samdark commented Mar 10, 2013

I'm against throwing exceptions in validators. Any value passed should either generate an error or be considered as valid. As for messages, I think array case is special and there's no point in customizing this message that only "too smart" people will see.

@mdomba

This comment has been minimized.

Show comment Hide comment
@mdomba

mdomba Mar 10, 2013

Member

@samdark I will stress this again... this "error" happens only if someone mess with the URL, so I don't see your point why would we just return the error message and continue the application as nothing was wrong. Note that this could later give some other nasty problems in the user app.

Because of all that and because this is not an expected value, exception is more than needed IMO.

Also note that we are already throwing different exceptions in the validators like for example the invalid operator in the CCompareValidator so why would be different for invalid value?

Member

mdomba commented Mar 10, 2013

@samdark I will stress this again... this "error" happens only if someone mess with the URL, so I don't see your point why would we just return the error message and continue the application as nothing was wrong. Note that this could later give some other nasty problems in the user app.

Because of all that and because this is not an expected value, exception is more than needed IMO.

Also note that we are already throwing different exceptions in the validators like for example the invalid operator in the CCompareValidator so why would be different for invalid value?

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Mar 10, 2013

Owner

Which nasty problems exactly? Value will not be valid.

We're throwing exceptions for not valid programmer input, not for not valid user import.

Owner

samdark commented Mar 10, 2013

Which nasty problems exactly? Value will not be valid.

We're throwing exceptions for not valid programmer input, not for not valid user import.

@mdomba

This comment has been minimized.

Show comment Hide comment
@mdomba

mdomba Mar 10, 2013

Member

I wrote "this could" not "this will"... it all depends on what the developer does with the parameters in his code.

Member

mdomba commented Mar 10, 2013

I wrote "this could" not "this will"... it all depends on what the developer does with the parameters in his code.

@samdark

This comment has been minimized.

Show comment Hide comment
@samdark

samdark Mar 10, 2013

Owner

Validators aren't generally filtering values so if developer uses not valid or not validated values he'll get into trouble anyway.

Exception is probably OK as well since it's a special case not achievable by regular users. Personally either exception or validator error is OK for me.

Owner

samdark commented Mar 10, 2013

Validators aren't generally filtering values so if developer uses not valid or not validated values he'll get into trouble anyway.

Exception is probably OK as well since it's a special case not achievable by regular users. Personally either exception or validator error is OK for me.

@cebe

This comment has been minimized.

Show comment Hide comment
@cebe

cebe Mar 10, 2013

Owner

I agree that validator should not throw exception for an input value as he is ment to be used to detect any value for beeing valid or not.
But I also see the point of @mdomba that the validator message should not complain about array type but simply say it is invalid as this is what a users sees and he does not care what is going on in the background.
Haven't looked into the code, but I think messages that are ment for end users should not contain technical terms even if this is a case which someone might only get by manipulating urls...

Owner

cebe commented Mar 10, 2013

I agree that validator should not throw exception for an input value as he is ment to be used to detect any value for beeing valid or not.
But I also see the point of @mdomba that the validator message should not complain about array type but simply say it is invalid as this is what a users sees and he does not care what is going on in the background.
Haven't looked into the code, but I think messages that are ment for end users should not contain technical terms even if this is a case which someone might only get by manipulating urls...

+ // https://github.com/yiisoft/yii/issues/1955
+ if(is_array($text))
+ $text='';
+

This comment has been minimized.

Show comment Hide comment
@qiangxue

qiangxue Mar 11, 2013

Owner

There is no need to check array here. The developer is responsible to make sure the type is proper. By doing so, it would mean we should do similar for nearly everywhere, which is simply too much for the framework.

@qiangxue

qiangxue Mar 11, 2013

Owner

There is no need to check array here. The developer is responsible to make sure the type is proper. By doing so, it would mean we should do similar for nearly everywhere, which is simply too much for the framework.

@qiangxue

This comment has been minimized.

Show comment Hide comment
@qiangxue

qiangxue Mar 11, 2013

Owner

I prefer to error messages here because it does fail the validation.

@mdomba made a point by saying that "this could later give some other nasty problems in the user app." However, it is not the responsibility of a single validator to stop the execution of the application because a user inputs an invalid value. A validator is responsible to check if an input value is valid or not and give back appropriate error messages. It should not stop the execution. What if there is some code after the validator who wants to detect such user attempts? (just an artificial example). The situation here is different from throwing exception in CCompareValidator because of invalid operator. In that case, the error is caused by developers, and the exception is used to warn the developer.

Owner

qiangxue commented Mar 11, 2013

I prefer to error messages here because it does fail the validation.

@mdomba made a point by saying that "this could later give some other nasty problems in the user app." However, it is not the responsibility of a single validator to stop the execution of the application because a user inputs an invalid value. A validator is responsible to check if an input value is valid or not and give back appropriate error messages. It should not stop the execution. What if there is some code after the validator who wants to detect such user attempts? (just an artificial example). The situation here is different from throwing exception in CCompareValidator because of invalid operator. In that case, the error is caused by developers, and the exception is used to warn the developer.

@mdomba

This comment has been minimized.

Show comment Hide comment
@mdomba

mdomba Mar 11, 2013

Member

Yes I understood that exceptions should be thrown only for developers error, I guess @samdark was thinking the same but I did not "get it"...

So, message it is, but currently there is a new fixed message used, shouldn't we check for the custom message even here?

Member

mdomba commented Mar 11, 2013

Yes I understood that exceptions should be thrown only for developers error, I guess @samdark was thinking the same but I did not "get it"...

So, message it is, but currently there is a new fixed message used, shouldn't we check for the custom message even here?

@qiangxue

This comment has been minimized.

Show comment Hide comment
@qiangxue

qiangxue Mar 11, 2013

Owner

Since this error normally does not appear to normal users, I think it is fine we don't offer customization.
In Yii 2, I am planning to introduce a property named allowArray in the base validator class and perform check there, which would avoid the duplicated array check in every validator.

Owner

qiangxue commented Mar 11, 2013

Since this error normally does not appear to normal users, I think it is fine we don't offer customization.
In Yii 2, I am planning to introduce a property named allowArray in the base validator class and perform check there, which would avoid the duplicated array check in every validator.

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