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

[Validator] Deprecated "checkDNS" option in Url constraint #25516

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@ro0NL
Contributor

ro0NL commented Dec 15, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #23538
License MIT
Doc PR symfony/symfony-docs#8921
@@ -90,6 +90,8 @@ public function validate($value, Constraint $constraint)
throw new InvalidOptionsException(sprintf('Invalid value for option "checkDNS" in constraint %s', get_class($constraint)), array('checkDNS'));
}
@trigger_error(sprintf('The "checkDNS" option in "%s" is deprecated since 4.1 and will be removed in 5.0.', Url::class), E_USER_DEPRECATED);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 18, 2017

Member

since version 4.1
I'm wondering: should we add a few words why? like eg
The "checkDNS" option in "%s" is deprecated since 4.1 and will be removed in 5.0. It's too fragile to be relied upon.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 18, 2017

Member

the notice should be before the above "if", isn't it?

This comment has been minimized.

@fabpot

fabpot Dec 18, 2017

Member

Not sure if this is too fragile, but the false positive rate is so high that it's not reliable. I think reliable is perhaps better.

@@ -73,6 +73,8 @@ public function validate($value, Constraint $constraint)
}
if ($constraint->checkDNS) {
@trigger_error(sprintf('The "checkDNS" option in "%s" is deprecated since version 4.1 and will be removed in 5.0. Its false positive rate is too high to be relied upon.', Url::class), E_USER_DEPRECATED);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 20, 2017

Member

I would just suggest a dash: false-positive

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 20, 2017

@@ -30,6 +30,7 @@ Validator
* The `Email::__construct()` 'strict' property is deprecated and will be removed in 5.0. Use 'mode'=>"strict" instead.
* Calling `EmailValidator::__construct()` method with a boolean parameter is deprecated and will be removed in 5.0, use `EmailValidator("strict")` instead.
* Deprecated `checkDNS` option in `Url` constraint. It will be removed in 5.0.

This comment has been minimized.

@xabbuh

xabbuh Dec 22, 2017

Member

Deprecated the checkDNS option in the Url [...]

@@ -29,7 +29,7 @@ Validator
* The `Email::__construct()` 'strict' property has been removed. Use 'mode'=>"strict" instead.
* Calling `EmailValidator::__construct()` method with a boolean parameter has been removed, use `EmailValidator("strict")` instead.
* Removed `checkDNS` option in `Url` constraint.

This comment has been minimized.

@xabbuh

xabbuh Dec 22, 2017

Member

Removed the checkDNS option from the Url constraint.

4.1.0
-----
* Deprecated `checkDNS` option in `Url` constraint. It will be removed in 5.0.

This comment has been minimized.

@xabbuh

xabbuh Dec 22, 2017

Member

Deprecated the checkDNS option in the Url [...]

@@ -21,18 +21,57 @@
*/
class Url extends Constraint
{
/**
* @deprecated since version 4.1, to be removed in 5.0

This comment has been minimized.

@xabbuh

xabbuh Dec 22, 2017

Member

since Symfony 4.1 (see #25565)

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 22, 2017

Forgot to deprecate $dnsMessage option. Now updated. Should it be mentioned explicitly? In general it's only enabled thru $checkDNS so fine to me as is.

@xabbuh

This comment has been minimized.

Member

xabbuh commented Dec 22, 2017

As people may forgot to not use the dnsMessage option when dropping checkDNS it might be good to deprecate it explicitly too.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Dec 23, 2017

Deprecation now moved to constraint constructor.

@@ -30,6 +30,7 @@ Validator
* The `Email::__construct()` 'strict' property is deprecated and will be removed in 5.0. Use 'mode'=>"strict" instead.
* Calling `EmailValidator::__construct()` method with a boolean parameter is deprecated and will be removed in 5.0, use `EmailValidator("strict")` instead.
* Deprecated the `checkDNS` and `dnsMessage` options in the `Url` constraint. They will be removed in 5.0.

This comment has been minimized.

@xabbuh

xabbuh Dec 26, 2017

Member

[...] options of the [...]

@@ -29,7 +29,7 @@ Validator
* The `Email::__construct()` 'strict' property has been removed. Use 'mode'=>"strict" instead.
* Calling `EmailValidator::__construct()` method with a boolean parameter has been removed, use `EmailValidator("strict")` instead.
* Removed the `checkDNS` and `dnsMessage` options in the `Url` constraint.

This comment has been minimized.

@xabbuh

xabbuh Dec 26, 2017

Member

[...] options from the [...]

4.1.0
-----
* Deprecated the `checkDNS` and `dnsMessage` options in the `Url` constraint. They will be removed in 5.0.

This comment has been minimized.

@xabbuh

xabbuh Dec 26, 2017

Member

[...] options of the [...]

ro0NL added some commits Dec 27, 2017

@fabpot

fabpot approved these changes Dec 31, 2017

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 31, 2017

Thank you @ro0NL.

@fabpot fabpot closed this in 38b946e Dec 31, 2017

@ro0NL ro0NL deleted the ro0NL:checkdnsrr branch Dec 31, 2017

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@iisisrael

This comment has been minimized.

Contributor

iisisrael commented Aug 17, 2018

I'm actually sad to see this go. Adding the functionality back into our project as a customized extension was not difficult, but I think it odd that there was so little discussion around the deprecation. If the underlying functionality is deemed inconclusive because of false positives, that's something that should be understood by the consuming application. We know that, and use this functionality as just one piece of a greater validation strategy. Having it as an option was just that, an option.

@iisisrael

This comment has been minimized.

Contributor

iisisrael commented Aug 17, 2018

Also, the same functionality continues to exist undeprecated as the checkHost and checkMX options in the EmailValidator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment