Skip to content

[Validator] Only handle numeric values in DivisibleBy #33435

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 3, 2019
Merged

[Validator] Only handle numeric values in DivisibleBy #33435

merged 1 commit into from
Sep 3, 2019

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Sep 2, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Currently it probably breaks because abs throws a notice on objects.

@yceruto yceruto added this to the 4.3 milestone Sep 2, 2019
@fancyweb fancyweb mentioned this pull request Sep 2, 2019
Closed
4 tasks
@fancyweb fancyweb force-pushed the validator-divisible-by-non-numeric-values branch 2 times, most recently from 6afa33e to 13e8b44 Compare September 3, 2019 07:41
@fancyweb
Copy link
Contributor Author

fancyweb commented Sep 3, 2019

Now the only problem with it is that the values can be transformed in the AbstractComparisonValidator class, meaning that the message can be incoherent with what is actually passed by the developer to the constraint. I wonder if we should not add a protected function supportsValue($value): bool in the abstract class to throw from here with the real value?

@ro0NL
Copy link
Contributor

ro0NL commented Sep 3, 2019

@fancyweb not sure i follow, the passed message still applies to numeric values. We get a different message/violation for type errors, like any other constraint does.

@fancyweb
Copy link
Contributor Author

fancyweb commented Sep 3, 2019

What I mean is that if my constraint is @DivisibleBy("+10 days") on a \DateTime value (totally not logical but still possible), the error message will be Expected argument of type "numeric", "\DateTime" given while I never provided a \DateTime instance in the first place. It was converted in the AbstractComparisonValidator class. The goal would be to have a better message with the initial passed value. Might not worth it though.

@ro0NL
Copy link
Contributor

ro0NL commented Sep 3, 2019

i see, IMHO that's a side effect we can live with yes.

@fancyweb fancyweb force-pushed the validator-divisible-by-non-numeric-values branch from 13e8b44 to f974add Compare September 3, 2019 09:18
@fabpot
Copy link
Member

fabpot commented Sep 3, 2019

Thank you @fancyweb.

fabpot added a commit that referenced this pull request Sep 3, 2019
@fabpot
bug #33435 [Validator] Only handle numeric values in DivisibleBy (fan…
c0f4037 
…cyweb)

This PR was merged into the 4.3 branch.

Discussion
----------

[Validator] Only handle numeric values in DivisibleBy

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Currently it probably breaks because `abs` throws a notice on objects.

Commits
-------

f974add [Validator] Only handle numeric values in DivisibleBy
@fabpot fabpot merged commit f974add into symfony:4.3 Sep 3, 2019
@fancyweb fancyweb deleted the validator-divisible-by-non-numeric-values branch September 3, 2019 17:35
@fabpot fabpot mentioned this pull request Oct 7, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@ro0NL ro0NL ro0NL approved these changes

@yceruto yceruto yceruto approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed Validator
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
5 participants
@fancyweb @ro0NL @fabpot @yceruto @carsonbot