-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Deprecated email validation mode loose
#43505
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
Hey! To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done? Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review. Cheers! Carsonbot |
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.
a similar notice is needed in validate()
, isn't it?
Should I commit the actual removal of "loose" to the 6.0 branch already, or when is the "usual" time to do this? |
loose
loose
loose
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.
the constraint should also trigger a deprecation when you instantiate it with a config putting it in loose mode
Not sure this is required to me. Not doing that would prevent triggering two very similar deprecations instead, thus reducing confusion.
You need to update these tests to use the |
you need to use the |
At least I managed to get rid of the 4 coming from
Don't even know what |
@@ -21,6 +22,8 @@ | |||
*/ | |||
class EmailValidatorTest extends ConstraintValidatorTestCase | |||
{ | |||
use ExpectDeprecationTrait; | |||
|
|||
protected function createValidator() | |||
{ | |||
return new EmailValidator(Email::VALIDATION_MODE_LOOSE); |
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.
The createValidator()
method is called in setUp()
in the base ConstraintValidatorTestCase
class. That call will lead to the deprecation being triggered here.
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.
So I need to add @group legacy
and $this->expectDeprecation
at ConstraintValidatorTestCase::setUp?
Cause I'm sure I already tried it here at EmailValidatorTest::createValidator()
and it didn't work...
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.
No, you need to update this method to not pass loose
but html5
as the mode instead. The legacy loose
mode needs to be tested explicitly in legacy test methods 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.
I think since we changed this, the deprecation warning is gone:
1) Symfony\Component\Validator\Tests\Constraints\EmailValidatorTest::testValidEmailsLoose with data set #0 ('fabien@symfony.com')
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
@expectedDeprecation:
-%A Since symfony/validator 5.4: The "loose" email validation mode is deprecated, use "html5" instead
+
So do I need to trigger_deprecation()
in createValidator()
too? But wouldn't this result in what @nicolas-grekas said he doesn't want? #43505 (review)
Not sure this is required to me. Not doing that would prevent triggering two very similar deprecations instead, thus reducing confusion.
src/Symfony/Component/Validator/Tests/Constraints/EmailValidatorTest.php
Outdated
Show resolved
Hide resolved
The purpose of EmailValidatorTest::testValidNormalizedEmails() seems to be to test the normalizer. So should I just delete all "html5-failing" email addresses from the data provider? |
Closing as it has been finished in #46518 |
Closes #41855
Don't know what else needs to be done... ;-)