Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] Allow the distinct validation rule to optionally ignore case #21757

Merged
merged 1 commit into from
Oct 25, 2017
Merged

[5.5] Allow the distinct validation rule to optionally ignore case #21757

merged 1 commit into from
Oct 25, 2017

Conversation

jfadich
Copy link
Contributor

@jfadich jfadich commented Oct 19, 2017

Added the 'ignore_case' parameter to the distinct validation rule to allow case insensitive check for distinct values.

@sisve
Copy link
Contributor

sisve commented Oct 19, 2017

Which language rules are used to calculate case sensitivity? Should preg be called with /u to support unicode? How can I specify which language rules should be used when doing case insensitive comparisons?

It would make sense to write some tests that are using non-ascii characters. I'll use my favorite characters for this; dotted and dotless i from Turkish.

Upper case Lower case
I= Latin Capital Letter I (U+0049) ı = Latin Small Letter Dotless I (U+0131)
İ= Latin Capital Letter I with Dot Above (U+0130) i = Latin Small Letter I (U+0069)

@tillkruss tillkruss changed the title Allow the distinct validation rule to optionally ignore case [5.5] Allow the distinct validation rule to optionally ignore case Oct 19, 2017
@jfadich
Copy link
Contributor Author

jfadich commented Oct 19, 2017

@sisve I've added the Unicode flag to the preg call. Not sure the best way to specify the language rules to use.

@taylorotwell
Copy link
Member

I'm not sure I have enough experience in this area to make a good decision here. @sisve do you have any further input after the latest changes?

@sisve
Copy link
Contributor

sisve commented Oct 20, 2017

It is not clear how preg/pcre handles locales.

The documentation of mb_strotolower says that it does not use locales, but looks at unicode character data. This is exactly why the turkish characters fails; Unicode does not have a "turkish lowercase dotted i" (that maps to "turkish uppercase dotted İ"), but just a "latin small letter i". There are known special rules for turkish (see https://www.unicode.org/Public/UCD/latest/ucd/SpecialCasing.txt) but these are not used in this case.

The IntlChar::tolower also fails.

The Collator class has no way to compare case-insensitive.

I believe the solution is to write an library for handling languages, locales, encodings and everything. It is always out-of-scope for whatever I was actually looking into when I came to this conclusion.

The turkish characters are always watching, silently waiting for their time. I see no reasonable solution for these at the moment.

With this said, the code is not escaping the $value it uses to generate the pattern, which will cause issues if the value contains slashes.

conform to style guide

add unicode support

escape slashes in regular expression
@jfadich
Copy link
Contributor Author

jfadich commented Oct 20, 2017

I updated the code to escape the $value used in the preg_grep pattern.

@taylorotwell
Copy link
Member

I'm kind of confused by the wording. Wouldn't it be better to call it "case_sensitive". It seems like adding the parameter makes the check fail if the cases don't match?

@sisve
Copy link
Contributor

sisve commented Oct 23, 2017

The rule is currently case sensitive, and ['a', 'A'] would validate successfully since they are different distinct values. Adding the ignore_case flag will consider the two values equal, and thus fail the distinctness test.

The flag affects the comparison; and a match should fail the validation. The logic error here is that the tests aren't checking the fails() method (which is part of the Validator interface), but the passes() method (which is not part of the Validator interface). So the tests looks to check for the inverted value of what we (or at least I) mentally expect to see.

@taylorotwell taylorotwell merged commit 1f6e787 into laravel:5.5 Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants