-
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
[11.x] Add fluent Email
validation rule
#54067
base: 11.x
Are you sure you want to change the base?
Conversation
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
@@ -86,7 +86,7 @@ class File implements Rule, DataAwareRule, ValidatorAwareRule | |||
* If no arguments are passed, the default file rule configuration will be returned. | |||
* | |||
* @param static|callable|null $callback | |||
* @return static|null | |||
* @return static|void |
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.
never returns null
, but does return void
use Illuminate\Validation\Validator; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class ValidationEmailRuleTest extends TestCase |
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.
Test class inspired by ValidationFileRuleTest
use Stringable; | ||
use function Illuminate\Support\enum_value; | ||
|
||
class Email implements Rule, DataAwareRule, ValidatorAwareRule |
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.
Heavily inspired by src/Illuminate/Validation/Rules/File.php
@@ -124,7 +124,7 @@ public function __construct($min) | |||
* If no arguments are passed, the default password rule configuration will be returned. | |||
* | |||
* @param static|callable|null $callback | |||
* @return static|null | |||
* @return static|void |
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.
This might be better suited for a separate PR as this is not strongly related to the rest of this PR
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.
It's an incorrect docblock in the two classes that are similar to this PR. This PR uses the correct return type and fixes it for the other usages. I think it's probably too small for a standalone PR, but will take it out and make a separate PR if the Laravel team prefers that
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 usually tiptoe around Taylor and something as small as this or a fly on his office wall can make him reject a PR, so I better don't take any chances.
@@ -86,7 +86,7 @@ class File implements Rule, DataAwareRule, ValidatorAwareRule | |||
* If no arguments are passed, the default file rule configuration will be returned. | |||
* | |||
* @param static|callable|null $callback | |||
* @return static|null | |||
* @return static|void |
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.
This might be better suited for a separate PR as this is not strongly related to the rest of this PR
While I like terse methods, I wonder if we should give these methods a bit more descriptive names? It's not super clear was |
@taylorotwell Thank you for reviewing the pull request and providing your feedback. I’ve updated the method names to make them more descriptive and aligned with Laravel’s validation conventions. New names:
I attempted to pick names aiming to balance clarity and brevity, ensuring developers can intuitively understand the intent of each rule when chaining them together. For example: Rule::email()
->rfcCompliant(true)
->domainExists()
->preventEmailSpoofing(); Perhaps they are not perfect yet, but it might inspire you to make the final naming tweaks! |
@taylorotwell If we’re moving away from string-based names, we could also consider @shaedrich’s suggestion of merging the This approach would reduce the number of methods from 6 to 4, making the API easier to use and more intuitive while maintaining clarity. Let me know your thoughts! |
I would actually flip that negation around: |
This PR aims to provide a similar experience as the Password validation rule and File validation rule (#43271) for emails. It does so by providing a fluent, extendable Email rule object.
Basic usage
Available methods
The following methods are available on the rule:
::default
: equivalent to thePassword::default()
rule::strictSecurity
: equivalent toRule::email()->strict()->dns()->spoof()
strict
: equivalent to thestrict
ruledns
: equivalent to thedns
rulespoof
: equivalent to thespoof
rulefilter
: equivalent to thefilter
rulefilterUnicode
: equivalent to thefilter_unicode
rulerfc
: equivalent to therfc
ruleExtending for custom types
Whilst the
File
rule only ships with the::image
method as a custom type, the rule isMacroable
, allowing each application to specify more granular file permissions as required.Open for discussion
I've added
strictSecurity
method as an option to easily create a solid email validation check, since the differences between the available rules are rather complex. Not including this method would be a valid choice as well. However, I hope this method will help Laravel developers better secure the email inputs, for example example against spoofing. The composition of this methods (strict + DNS + spoof) is a result of running applications in production and handling all kinds of edge cases with invalid emails that passed validation before using this combination.Thanks to @lukeraymonddowning for providing #43271. I've used his PR as inspiration for the body text of this PR.