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

[Breaking Change] Refactor validation rules: alpha, alpha_dash, alpha_num #45767

Closed

Conversation

fuwasegu
Copy link
Contributor

@fuwasegu fuwasegu commented Jan 23, 2023

This PR is a redo of #45609

Reason for re-submitting PR

The reason why I refactored this again is discussed at #45660 . Please see.
In the previous PR, we received a comment that custom rules should be defined individually by the developer.
However, as described below, there is a problem that the definition of "alphabet" is implemented differently from what is generally known, which is not good for a Web framework widely used in the world.
Also, since we received a lot of support in the above Discussion, we thought it would be better to address this issue as a framework rather than as individual developers.

What is this?

The validation rules: alpha, alpha_dash, alpha_num have a problem.
The common definition of "alphabet" or "alphabetic characters" is the 26 types of characters [a-zA-Z] (52 types of uppercase and lowercase characters combined).
However, Laravel's validation rules for verifying whether an input character is an alphabetic character actually allowed characters that went well beyond the definition of an alphabetic character.

This is the actual "alpha" rule code

public function validateAlpha($attribute, $value)
{
    return is_string($value) && preg_match('/^[\pL\pM]+$/u', $value) > 0;
}

The following is the meaning of the properties included in this regular expression (from https://www.php.net/manual/en/regexp.reference.unicode.php)
スクリーンショット 2023-01-23 13 37 25

That's why, I believe the following code is the more correct one

public function validateAlpha($attribute, $value)
{
    return is_string($value) && preg_match('/^[a-zA-Z]+$/u', $value) > 0;
}

This overly broad definition is contrary to the developers' intentions in a language area such as Japan, where multibyte characters are used on a daily basis.
I shared this opinion in the discussion above and got over 30 Upvodes. This is the second most Upvodes of any discussion topic in the Laravel project.

Changes

  • Refactored existing rule logic (alpha, alpha_num, alpha_dash) 4441ec3
    • Changed the rule to not allow multibyte characters according to the original definition of "alpha".
  • Renamed existing definitions 59c39e6
    • The rule originally defined as "alpha" has been renamed to indicate that it "contains multibytes".
  • Wrote the test. fa7c923

What you get from this change

  • Correct validation rules based on the original definition
  • Clear separation of validation rules when multibyte is allowed and when it is not
  • Preventing the use of validation rules contrary to the developer's intentions

@taylorotwell
Copy link
Member

taylorotwell commented Jan 23, 2023

Hi there - sorry to close this again but I would actually leave the current rules as they are and instead introduce ascii_alpha variants. It's too breaking to change these existing rules which have existed in this way for many years. Similar to your original PR but with all variants.

@fuwasegu
Copy link
Contributor Author

fuwasegu commented Jan 23, 2023

@taylorotwell
Thanks for the response!
I see...
I understood that this PR was a rather disruptive change, since the alpha and other variation rules have been implemented for quite some time, and I had to understand that since Laravel is a widely used and long-established web framework.
Can I implement your proposed ascii_alpha implementation from now on?
And if I implement it, may I create a PR to include in 9.x?

@fuwasegu
Copy link
Contributor Author

I tried to impl ascii validation rules to 9.x
#45769

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.

2 participants