-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(ocp): add email address validator #53834
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
lib/private/Mail/Mailer.php
Outdated
| /** | ||
| * @param string $email Email address to be validated | ||
| * @return bool True if the mail address is valid, false otherwise | ||
| * @deprecated 32.0.0 use IEmailValidator.isValid instead |
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 was already deprecated since 26 we undeprecated it for 32 but if you add a replacement, please readd the deprecation since 26.
susnux
left a comment
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.
Code looks good, my comment about the deprecation is still valid
I only change the interface 🙈 |
|
What do you think about the trait to obtain an instance for usage in tests? |
46206ff to
ef9312c
Compare
|
The tests are now using the preconfigured instance from the trait. I think it’s a plus that the validation is now done with the actual implementation and not a test double. The downside is that we’re losing the check that the validator was called. |
e92dd99 to
6be9132
Compare
b6d28ce to
93db1d6
Compare
Signed-off-by: Daniel Kesselberg <[email protected]>
93db1d6 to
336c6d2
Compare
|
Moved the refactoring commits, to use the validator, to another branch because it's external API freeze today, and checking the test needs more time. |
But that does not change anything here, no? We do not remove the old way and just refactor internal usage. |
Fair enough, technically we do not break anything and just add a new API. I'm running a bit out of time this week and thus wanted to make sure that the new API lands in 32. There was a conflicting merge meanwhile, and also one of the tests needs a closer look (if we are still testing the right thing). I hope to finish it next week and have everything migrated for 32. Okay for you to merge this one without the refactorings, or do you want to keep it together? |
|
Its fine. |
Summary
Makes it possible to validate an email address, without injecting the mailer.
Todo
Checklist