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

[Validator] Standardize constraint validators#28645

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 merge1 commit intosymfony:masterfromro0NL:constraint-tests

Closed

[Validator] Standardize constraint validators#28645

ro0NL wants to merge1 commit intosymfony:masterfromro0NL:constraint-tests

Conversation

Copy link

Contributor

ro0NL commentedSep 30, 2018•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no-ish
Deprecations?	no
Tests pass?	yes
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

there are subtle inconsistencies between constraint validators, esp. related to string values. I think we should tackle it, by taking away the boring stuff.

constraint type isnt always checked
casting objects to string doesnt always happen
checking empty string after casting doesnt always happen
the order of checks isnt always the same

This would help custom constraints, as well as core to e.g. deprecate empty string handling in a unified way (#28611).

carsonbot added Status: Needs Review Validator Feature labels

Sep 30, 2018

[Validator] Standardize constraint validators

7ec59ad

Copy link

Contributor

ostrolucky commentedSep 30, 2018

Regarding changing how exceptions are being thrown, tread lightly. Current design how invalid types are handled is faulty because it causes 500s even when there is@Type constraint, so spreading this design to all constraints does not make sense.

See
#12312
#26463

Copy link

ContributorAuthor

ro0NL commentedSep 30, 2018

that should be solved by#27917 right, here we just move code around.

Copy link

Contributor

ostrolucky commentedSep 30, 2018

If it gets merged, yeah

Copy link

Contributor

gmponos commentedOct 1, 2018

Don't get me wrong, but I believe that the method names that you have are not descriptive on what the methods are actually doing.

testConstraint gives me the feeling that the constraint will be executed and tested. Personally I would prefer something likeassertConstraintClass
toString gives the feeling that the Constraint class it's self will be converted to string. Something likeconvertValueToString would be better for me.

nicolas-grekas added this to the4.2 milestone

Oct 1, 2018

carsonbot removed the Status: Needs Review label

Oct 1, 2018

OskarStark requested changes

Oct 1, 2018

View reviewed changes

src/Symfony/Component/Validator/Constraints/BicValidator.php

		* file that was distributed with this source code.
		*/

		useSymfony\Component\Validator\Constraints\Bic;

Copy link

Contributor

OskarStarkOct 1, 2018

Choose a reason for hiding this comment

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

The use statement should be below the namespace

carsonbot added the Status: Needs Work label

Oct 1, 2018

Copy link

ContributorAuthor

ro0NL commentedOct 2, 2018•
edited
Loading

@gmponos thanks :) i did it a bit quick to propose the idea... we can talk about naming any time, but i think your suggestion is better yes.

i likeassertConstraint() + getStringValue() too :) i havent looked into all constraint yes, but we might addgetNumberValue() as well (what's needed i'd say).

nicolas-grekas reviewed

Oct 3, 2018

View reviewed changes

src/Symfony/Component/Validator/ConstraintValidator.php

		*
		* @throws UnexpectedTypeException
		*/
		finalprotectedstaticfunctiontestConstraint(Constraint$constraint,string$expectedClass)

Copy link

Member

nicolas-grekasOct 3, 2018•
edited
Loading

Choose a reason for hiding this comment

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

do we really need this? I feel like this removes some info for static analysis + increases the stack traces of exception for little practical benefit, don't you think?

Copy link

Contributor

ostroluckyOct 3, 2018

Choose a reason for hiding this comment

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

I ran a little investigation:

PHPStorm doesn't understand this
Psalm doeshttps://getpsalm.org/r/acd72ab0ab cc@muglug
PHPStan doesn'thttps://phpstan.org/r/be6b68a728fc1d24fb27069640f46701 cc@ondrejmirtes

Anyway Validator is full of this piece of code, so I think it's worth it. SA tools which don't support it is problem of the tool itself, not the code. And we can help them by specifying@param alongside@inheritDoc (works for PHPStorm and Psalm, PHPStan is stubborn like usual).

Extraction of common pieces of code is important for keeping consistency. Also, if this was done in the first place, there would be no need to manually touch so many places in#27917

But, this should be renamed.testConstraint is odd one.assertInstanceOf?

Copy link

Member

nicolas-grekasOct 3, 2018

Choose a reason for hiding this comment

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

That's not really a common piece of logic - that's an "if"...

Copy link

Contributor

ostroluckyOct 3, 2018

Choose a reason for hiding this comment

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

I didn't mention "logic", I mentioned "piece of code". Anyway "if" contains some logic. This "if" is in almost all validators. Devs keep producing highly repetitive code instead of extracting it to function. This is like no. 1 rule of programming. Programs are good at repeating, not humans.

And here is another advantage of having such thing there in the first place - if it was already there, all custom validators using such function would be automatically compatible with#27917. Since it's not there (because such function is too simple right?), every custom constraint will now have to change this piece of code within it. But it's too late for#27917. Maybe another time there will be need to change the message or exception type.

Copy link

Member

nicolas-grekasOct 3, 2018

Choose a reason for hiding this comment

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

I get that, but I don't agree: designing the code to make such changes easy is not a target at all to me. Software architecture should be built around things that change, not those that don't (#27917 is not a change that should be centralized to me.)

Copy link

Contributor

gmponos commentedOct 3, 2018

Not sure how much my word counts on this, but personally I don't like this. It's the mentality behind of it that I don't like.

I believe that it encourages developers to add more special cases inside these functions. What I mean is that I already imagine developers preparing their special cases with extraifs to be added inside thetoString function or developers developing atoXml function etc etc.

ThetestConstraint I can not imaging how a developer can come up with a special case but does it really worth it adding the function call overhead over there? And as@nicolas-grekas said it is just anif

Never the less if this gets approved I believe that the results returned bytoString method is inconsistent. If the value is'' it will returnnull and this can cause some trouble to developers who might misused the function.

ro0NL mentioned this pull request

Oct 4, 2018

[Validator] catch any UnexpectedValueException on validation#27917

Merged

Copy link

ContributorAuthor

ro0NL commentedOct 4, 2018•
edited
Loading

@gmponos i see you're point, im not too happy with the current approach myself as well :)

If the value is '' it will return null

That's intended, i.e. the current behavior. But i agree we might not want to standardize it like this, as not every constraint has to rely on it. FYI in core i'd like to get rid of this rule eventually, see#28611

I'll give this some more thought for now, and see how#27917 goes first.