-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[12.x] Add isEmail()
for Str
and Stringable
#57049
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
Conversation
isEmail()
for Str and Stringable
isEmail()
for Str and StringableisEmail()
for Str
and Stringable
@rodrigopedra So this is the 5th one. Yikes! I wanted this method to exist and use it. Seems like there is a demand for this, although the odds of getting this merged are slim. Anyway, I'll let it stay. |
src/Illuminate/Support/Str.php
Outdated
return false; | ||
} | ||
|
||
$validations = (new Collection($parameters)) |
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 wouldn't use parameters here like we do in validation. You can use named arguments for better readability.
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.
@taylorotwell Should I go for something like this?
public static function isEmail(
$value,
bool $strict = false,
bool $dns = false,
bool $spoof = false,
bool $filter = false,
bool $filterUnicode = false,
array $customValidations = [],
) {}
src/Illuminate/Support/Str.php
Outdated
*/ | ||
public static function isEmail($value, $parameters = []) | ||
{ | ||
if (! is_string($value) && ! (is_object($value) && method_exists($value, '__toString'))) { |
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.
Wouldn't testing if the object's class implements Stringable
be better?
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.
@shaedrich That sounds better. I'll fix it.
src/Illuminate/Support/Str.php
Outdated
* Determine if the given value is a valid email. | ||
* | ||
* @param mixed $value | ||
* @param array<int, int|string> $parameters |
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.
You could narrow this further down:
* @param array<int, int|string> $parameters | |
* @param array<int, int|'strict'|'dns'|'spoof'|'filter'|'filter_unicode'|class-string> $parameters |
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.
@shaedrich Sure, but @taylorotwell said:
I wouldn't use parameters here like we do in validation. You can use named arguments for better readability.
Should I go for this instead?
public static function isEmail(
$value,
bool $strict = false,
bool $dns = false,
bool $spoof = false,
bool $filter = false,
bool $filterUnicode = false,
array $customValidations = [],
) {}
Or I'd love a better suggestion than my proposal.
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.
Yeah, Taylor's feedback and your solution look good 👍🏻
You can incorporate my suggestion into this without a problem:
/**
* @param class-string[] $customValidations
*/
public static function isEmail(
$value,
bool $strict = false,
bool $dns = false,
bool $spoof = false,
bool $filter = false,
bool $filterUnicode = false,
array $customValidations = [],
) {}
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.
@shaedrich Great! I'll implement all the changes. BTW, may I add type declarations?
public static function isEmail(
\Stringable|string $value,
// other arguments...
Instead of the if check?
if (! is_string($value) && ! (is_object($value) && $value instanceof \Stringable)) {
return false;
}
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.
We tend to use native types over PHPDoc ones these days, so that would make sense 👍🏻
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.
@Rahul-Mac I can't approve or reject since I'm not a reviewer, but Taylor will
You need to move the email validator dependency from the For those who use these components outside a Laravel app.
|
…support's composer.json
Perhaps it should be considered to make it easier to validate a single value in a fluent way in the validator and return the string helper? After all, this is a string helper, not a string validator. |
@NickSdot |
I know. But they shouldn't be there (at least they don't have dependencies, though). Where does it end? Do we soon have the full validator in the string helper? |
I was advocating for reducing the bloat of the string helper, yet to no avail (#52979), so I have to assume, this is by design for a lack of effort to change that. |
I remember that PR. Here since Laravel 4. Things got way too overwhelming ever since. In average we added ~42,000 new lines per year since L5.4. And that's just Now we start to basically "proxy" the validator into the string helper? After this is merged, the rest will follow? How about We have so much things in here that people send PRs for methods that already exist. Because no one knows everything anymore. Not even the OGs and Taylor himself (as seen on Twitter). My personal feeling is that something has to happen, somehow. Maybe a feature freeze year under the theme "let's clean up and reorganise things" would do good. Modernise, type, reduce inheritance, reduce duplication, reduce call stacks, separate concerns, make global helpers namespaced, make IDEs understand Laravel without plugins that make it sweat, etc... Open a new namespace as a sibling of Illuminate or whatever; then people can opt-in in their own pace. Introduce official rector rules. This is not me bashing. It's subjective and constructive feedback. @taylorotwell, you mentioned on Twitter that you are very interested in any feedback. Here is some. 👆 ![]() |
@NickSdot Your argument is compelling. There are tons of classes that appear bloated but the thing is (correct me if I'm wrong here) that Laravel is more of a "making devs happy & rapidly building code" framework more than a "best practices" framework like Symfony. Let's face it: Laravel violates a few principles. Personally, if I wanted to follow "best practices", I'd code using Symfony. But Laravel helps me get my work done and provides a certain ease although I do prefer Contracts over Facades. This helper is something that I felt was needed. Let's see what happens. But I'll keep in mind not to come up with another such helper unless it truly solves a problem. |
Screenshot above shows that devs aren't as happy anymore. My point exactly. I don't even talk about principles here. It's about usability. The Laravel approach worked great for a long time. But now Laravel is a massive framework. It has many many more tools compared to L4. Throwing everything just somewhere like it's a kitchen sink will not age well. |
It's great feedback, thanks! 👍🏻 Yeah, that'd be great.
Wholeheartedly agree! 👍🏻 |
"ext-ctype": "*", | ||
"ext-filter": "*", | ||
"ext-mbstring": "*", | ||
"egulias/email-validator": "^3.2.5|^4.0", |
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.
Should just be a suggest:
framework/src/Illuminate/Support/composer.json
Lines 52 to 62 in 280b160
"suggest": { | |
"illuminate/filesystem": "Required to use the Composer class (^12.0).", | |
"laravel/serializable-closure": "Required to use the once function (^1.3|^2.0).", | |
"league/commonmark": "Required to use Str::markdown() and Stringable::markdown() (^2.7).", | |
"league/uri": "Required to use the Uri class (^7.5.1).", | |
"ramsey/uuid": "Required to use Str::uuid() (^4.7).", | |
"symfony/process": "Required to use the Composer class (^7.2).", | |
"symfony/uid": "Required to use Str::ulid() (^7.2).", | |
"symfony/var-dumper": "Required to use the dd function (^7.2).", | |
"vlucas/phpdotenv": "Required to use the Env class and env helper (^5.6.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.
Then Validation/composer.json
should keep it as required.
Thanks for your pull request to Laravel! Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include. If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions! |
This PR introduces a new method to determine if a value is a valid email. Currently, the only way to check this is using the
Validator
. This change transfers the logic ofValidatesAttributes::validateEmail()
intoStr::isEmail()
Signature
Usage
Basic
Validation Mode
Note
RFC validation will be used by default if all the flags are set to false and there aren't any custom validations added.
Stringable
class