[Validator] new email validation option to match with w3c official specification #47872
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
b62971f to
7cb0c02
Compare
|
@guillemfondin could you please add some unit tests for the change. |
|
@maxbeckers Of course :) |
There was a problem hiding this comment.
This approach makes sense to me. Accepting emails with no TLD by default would likely make more bad than good in practice. We'd better allow this in an opt-in way. The alternative could be to just do nothing BTW. The motivation for changes in Symfony should be based on real-world use cases.
7cb0c02 to
0a40e62
Compare
|
Weird having discussion in two seperate places, thanks @guillemfondin for notifying me :) Personally I feel a bit uncomfortable with this change. Not because I need the ability to allow emails without a tld, actually I don't have an use-case for this feature. I have filed the issue only because during some tests I got to the point, where the browser validator said the email is fine, while the backend validator said it's invalid and that made me curious. Having a validator, which claims it does the same check as the browser and is also named to represent this, but actually having a difference is weird. It get's even more weird, when the html5 mode (named after the html5 validator), does something different than the actual html5 validator and the "real" html5 validator has a name not even related to html5. I'd say the naming should move to something like "html5" and "html5-force-tld". The only issue is the BC implication. |
src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php
Outdated
Show resolved
Hide resolved
d0da11d to
23047d9
Compare
|
Thank you @guillemfondin. |
…ndin) This PR was squashed before being merged into the 6.2 branch. Discussion ---------- [Validator] Email - new `allow-no-tld` mode See [Related feature PR](symfony/symfony#47872) As suggested [here](symfony/symfony#47712 (comment)) ; the actual description of `html5` mode isn't consistent with the one provided by browsers. Browsers allow no tld, `html5` mode not. I proprose to move the actual description of `html5` mode to the new `allow-no-tld` mode description, and clarify the subtlety (force .tld) of the actual `html5` mode. Commits ------- 485d57e [Validator] Email - new `allow-no-tld` mode
…ion modes can be configured (xabbuh) This PR was merged into the 6.4 branch. Discussion ---------- [FrameworkBundle] ensure that all supported e-mail validation modes can be configured | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #60352 | License | MIT this update was forgotten when adding the `html5-allow-no-tld` e-mail validation mode in #47872 Commits ------- b8b3c37 ensure that all supported e-mail validation modes can be configured
WHAT
Email validator no matches with w3c official specication (that allow no tld) see #47712 for details.
WHY
In general, for public common usage, developers may not need to give users this liberty.
Except for specific use case, (internal email for eg) in companies...
HOW
Add an option for accept tld (already exist
Email::VALIDATION_MODE_HTML5) or not (official specEmail::VALIDATION_MODE_HTML5_ALLOW_NO_TLD)