-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.7] Add uuid validation rule to validator #26135
Conversation
The |
@X-Coder264 Didn't think of that, I removed the dependency. Now it uses regex directly |
return false; | ||
} | ||
|
||
if ($value === '00000000-0000-0000-0000-000000000000') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following ramsey/uuid
validation, this is a nil uuid, which is considered valid. We can choose to accept nil uuids or not. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he means that you just hardcoded a case. Make your regex include that case.
Edit: I just saw your regex, it already validate the nil UUID, did you tested this regex or did you just made a copypasta from StackOverflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha!
I followed the ramsey/uuid
validation workflow. They check it agains 00000000-0000-0000-0000-000000000000
before the regex so I did the same.
return false; | ||
} | ||
|
||
return preg_match('/^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{12}$/D', $value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the check is case insensitive it makes sense to use the i
option.
Also you can use \d
for digits.
return preg_match('/^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$/iD', $value);
return false; | ||
} | ||
|
||
return preg_match('/^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$/iD', $value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preg_match
doesn't return bool
, it returns int
:
return preg_match('/^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$/iD', $value); | |
return preg_match('/^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$/iD', $value) === 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed validateUrl condition
return preg_match($pattern, $value) > 0; |
Does this regex match the regex used by Ramsey UUID? |
@taylorotwell Functionally it's the same, but the regex this PR uses |
Often there is the need to extend the Validator to add a uuid validation rule like this below.
Since Laravel already depends on
ramsey/uuid
, I think this would be a nice little addition to Laravel's validation ruleset.Let me know what you think