NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

ro0NL wants to merge4 commits intosymfony:masterfromro0NL:checkdnsrr

Closed

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

ro0NL wants to merge4 commits intosymfony:masterfromro0NL:checkdnsrr

Conversation

Copy link

Contributor

ro0NL commentedDec 15, 2017•
edited
Loading

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

carsonbot added Status: Needs Review Validator Feature Deprecation labels

Dec 15, 2017

nicolas-grekas reviewed

Dec 18, 2017

View reviewed changes

src/Symfony/Component/Validator/Constraints/UrlValidator.php Outdated

		thrownewInvalidOptionsException(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);

Copy link

Member

nicolas-grekasDec 18, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

sinceversion 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.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

nicolas-grekas approved these changes

Dec 20, 2017

View reviewed changes

src/Symfony/Component/Validator/Constraints/UrlValidator.php Outdated

		}

		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);

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I would just suggest a dash: false-positive

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Dec 20, 2017

nicolas-grekas added this to the4.1 milestone

Dec 20, 2017

xabbuh reviewed

Dec 22, 2017

View reviewed changes

UPGRADE-4.1.md Outdated


		* 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.

Copy link

Member

xabbuhDec 22, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Deprecated thecheckDNS option in theUrl [...]

xabbuh reviewed

Dec 22, 2017

View reviewed changes

UPGRADE-5.0.md Outdated

		* 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.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Removed thecheckDNS option from theUrl constraint.

xabbuh reviewed

Dec 22, 2017

View reviewed changes

src/Symfony/Component/Validator/CHANGELOG.md Outdated

		4.1.0
		-----

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Deprecated thecheckDNS option in theUrl [...]

xabbuh reviewed

Dec 22, 2017

View reviewed changes

src/Symfony/Component/Validator/Constraints/Url.php Outdated

		class Urlextends Constraint
		{
		/**
		* @deprecated since version 4.1, to be removed in 5.0

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

since Symfony 4.1 (see#25565)

ro0NL mentioned this pull request

Dec 22, 2017

Removed checkDNS option in Url constraintsymfony/symfony-docs#8921

Closed

Copy link

ContributorAuthor

ro0NL commentedDec 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.

Copy link

Member

xabbuh commentedDec 22, 2017

As people may forgot to not use thednsMessage option when droppingcheckDNS it might be good to deprecate it explicitly too.

[Validator] Deprecated "checkDNS" option in Url constraint

63df13d

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

xabbuh reviewed

Dec 26, 2017

View reviewed changes

UPGRADE-5.0.md Outdated

		* 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.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

xabbuh reviewed

Dec 26, 2017

View reviewed changes

src/Symfony/Component/Validator/CHANGELOG.md Outdated

		4.1.0
		-----

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

ro0NL added3 commits

December 27, 2017 12:43

Update UPGRADE-4.1.md

17554c9

Update UPGRADE-5.0.md

b9f77eb

Update CHANGELOG.md

3a38b81

fabpot approved these changes

Dec 31, 2017

View reviewed changes

Copy link

Member

fabpot commentedDec 31, 2017

Thank you@ro0NL.

fabpot closed this in38b946e

Dec 31, 2017

ro0NL deleted the checkdnsrr branch

December 31, 2017 07:10

javiereguiluz mentioned this pull request

Jan 3, 2018

[Validator] Url and Email validator possible bug#25666

Closed

fabpot mentioned this pull request

May 7, 2018

Release v4.1.0-BETA1#27181

Merged

Copy link

iisisrael commentedAug 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.

Copy link

iisisrael commentedAug 17, 2018•
edited
Loading

Also, the same functionality continues to exist undeprecated as thecheckHost andcheckMX options in theEmailValidator.

ro0NL mentioned this pull request

Oct 1, 2018

[Validator] Email validation false-positive due to relative host#28665

Closed

ro0NL mentioned this pull request

May 29, 2019

[Validator] Remove checkDNS option in Url#31681

Merged

nicolas-grekas added a commit that referenced this pull request

May 29, 2019

minor#31681[Validator] Remove checkDNS option in Url (ro0NL)

52756a5

This PR was merged into the 5.0-dev branch.Discussion----------[Validator] Remove checkDNS option in Url| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->See#25516Commits-------885703f [Validator] Remove checkDNS option in Url

fabpot mentioned this pull request

Jul 20, 2022

[Validator] addcheckTldDns option toUrlValidator#46921

Closed