-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Mailer][Mime][TwigBridge][Validator] Allow egulias/email-validator 3.x #39685
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
derrabus
commented
Jan 2, 2021
Q | A |
---|---|
Branch? | 4.4 |
Bug fix? | yes |
New feature? | no |
Deprecations? | no |
Tickets | Fix #39656 |
License | MIT |
Doc PR | N/A |
I need a little help with that test: symfony/src/Symfony/Component/Mime/Tests/Header/IdentificationHeaderTest.php Lines 109 to 118 in 04c67e6
During the construction of @fabpot you are the author of that test: Do you recall what is tested here and have an idea how we can proceed with the changed behavior of |
composer.json
Outdated
@@ -118,7 +118,7 @@ | |||
"predis/predis": "~1.1", | |||
"psr/http-client": "^1.0", | |||
"psr/simple-cache": "^1.0", | |||
"egulias/email-validator": "~1.2,>=1.2.8|~2.0", | |||
"egulias/email-validator": "^1.2.8|^2|^3", |
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.
Shall I change bump to ^2.1.10|^3
here to be consistent with the constraint in the components? I don't think that we're testing with v1 anywhere at the moment.
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.
Let's drop 1.2, no need to support 3 major versions anyway.
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.
Do we really want to drop support for it in a patch release?
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 should drop it in 5.x only.
src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php
Show resolved
Hide resolved
But these characters are valid (see https://tools.ietf.org/html/rfc2822#section-3.2.4). If looks like a wrong change in email validator. |
Indeed, when validating strictly according to the RFC, these characters should be allowed if I read the RFC correctly. Friendly ping to @egulias. |
Hello there, |
@egulias Thanks for the comment. The main "issue" here is that we are validating a message id/reference/reply to and for those, I'm not sure we should use RFC 1035. At least, that would be a BC break (which we can assume for good reasons). Is there a way to keep validating according to the original spec? |
Hi @fabpot , To solve this two options come to my mind: a) is for Symfony to add a custom validator for it implementing the interface. b) is to make it the next feature to be added. I can work on my own here but it will take time; some help would be very much appreciated to speed it up. Happy to hear more possibilites. |
Hi @fabpot @derrabus , |
Ok. So did a research on how the validator is being used in Symfony & Swiftmailer. My conclusion is as follows: This was possible in version <3 of the EmailValidator because of the interpretation done of the RFC. The solution for this, then, is to create a new validation for "message-id" and then addapt if (!self::$validator->isValid($this->address, new MessageIDValidation())) { @fabpot @xabbuh & others, please let me know if this would work for you. DetailsThe exception bubbles from Address in the Mime component, used from the Now, here's the thing. id-right
To note that domain syntaxis
And at the ends, specifies:
STD13 is RFC1035 |
Hi there! if (!self::$validator->isValid($this->address, new MessageIDValidation())) { |
f6f5404
to
b37e598
Compare
@egulias That's great news, thank you. Give me a ping as soon as we can test the new validator. I'll update this PR then. |
PR ready @derrabus egulias/EmailValidator#290 , will give it a week and merge next one. |
b37e598
to
3ec6a8b
Compare
@egulias This PR now runs against your feature branch and the tests are green now. Apparently, your changes work for us. 🎉 |
3ec6a8b
to
3beeadb
Compare
Thank you, the PR is ready now. 🚀 |
Thank you @egulias! |
Thank you @derrabus. |
Thanks! I've merged the changes up to 5.x and resolved all the composer.json conflicts. |