8000 Replace sfValidatorEmail with php builtin filter_var function by nederdirk · Pull Request #52 · FriendsOfSymfony1/symfony1 · GitHub
[go: up one dir, main page]
Skip to content

Replace sfValidatorEmail with php builtin filter_var function #52

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Replace sfValidatorEmail with php builtin filter_var function #52

wants to merge 3 commits into from

Conversation

nederdirk
Copy link

As this fork of Symfony1 depends on PHP 5.3 or greater, it is a good idea to use the PHP builtin function. The original implementation is at least as good as the Symfony regular expression and improves over the PHP versions.

@j0k3r
Copy link
Contributor
j0k3r commented Mar 27, 2014

Of course, this can be a better solution to what symfony1 use.

But according to what was done in Symfony2 (here and here), they dropped filter_var in favor to egulias / EmailValidator.
Why not integrate it instead of filter_var ?

By the way, could you add some tests in sfValidatorEmailTest so that the regex validator will fail and not the one with filter_var ? You can get inspired by these tests.

@nederdirk
Copy link
Author

Thank you! I had not heard about equlias/EmailValidator before, I will surely look into it soon.
One caveat may be that a full-blown LL(1) parser is a bit code-heavy compared to PHP's builtin, maybe I'll do some benchmarks if I find the time.

@GromNaN
Copy link
Collaborator
GromNaN commented Mar 27, 2014

Hi, I don't want to break the backward compatibility on such feature. Creating a new validator is easy enough so that we don't need to alter the behavior of the framework.

@nederdirk
Copy link
Author

I don't think improving the email validation breaks backwards compatibility, as the behavior of sfValidatorEmail still validates email addresses. The original implementation, however, accepts invalid, unroutable addresses, e.g. .sample@example.com, as valid.
Filtering these input errors as soon as possible prevents email bounces and invalid information in data storage.
I think accepting less errors is not the same thing as unnecessarily making validation stricter. Therefor I think the solution @j0k3r suggested is the best one, as it is a well-tested email validation library, highly conforming to the formal specification of email addresses. On top of that it accepts international domain names, which are valid according to the RFCs but not accepted by PHP's filter_var at this time.

@miniche
Copy link
miniche commented May 31, 2016

Mmh @j0k3r or @GromNaN: What do you want to do with this PR?

@GromNaN
Copy link
Collaborator
GromNaN commented May 31, 2016

👎 For this reason.

@j0k3r j0k3r closed this Mar 9, 2018
@Ugoku Ugoku deleted the validatorEmail branch January 21, 2020 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nederdirk @j0k3r @GromNaN @miniche
0