Skip to content
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] Choices constraint improvement #29658

Merged
merged 1 commit into from Jan 2, 2019

Conversation

Projects
None yet
8 participants
@nikophil
Copy link
Contributor

commented Dec 20, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

Hi,

here is a little improvement for the choice constraint: expose a new choices wildcard for the messages, in order to provide a way to display easily the valid choices to the user.

thanks.

@nikophil nikophil force-pushed the nikophil:validator-choice-constant branch from 507709c to 05b1626 Dec 20, 2018

@HeahDude

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

This can be closed as this feature is supported by the annotation reader already, just remove the quotes :).

 /**
     * @var string
     *
     * @Assert\Choice(choices=App\Constants::AVAILABLE_STATES)
     */
    private $state;
@yceruto

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

@HeahDude Do you meant @Assert\Choice(choices=App\Constants::AVAILABLE_STATES)?

@HeahDude

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

yes x).

@@ -100,6 +116,7 @@ public function validate($value, Constraint $constraint)
} elseif (!\in_array($value, $choices, true)) {
$this->context->buildViolation($constraint->message)
->setParameter('{{ value }}', $this->formatValue($value))
->setParameter('{{ choices }}', implode(', ', $choices))

This comment has been minimized.

Copy link
@HeahDude

HeahDude Dec 21, 2018

Member

Ah this line is still a nice addition indeed!

@nikophil

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

yeah indeed, you're right, and i 've learnt something new :)
will we keep the new wildcard choices i've added, or should i close the PR ?

@HeahDude

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

I'm 👍 for supporting a default choices implode, as long as we don't change the default message for both BC and let it opt-in performance wise.

@nikophil nikophil force-pushed the nikophil:validator-choice-constant branch from 05b1626 to 1ebdef3 Dec 21, 2018

@nikophil

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2018

i've updated the PR

@@ -100,6 +100,7 @@ public function validate($value, Constraint $constraint)
} elseif (!\in_array($value, $choices, true)) {
$this->context->buildViolation($constraint->message)
->setParameter('{{ value }}', $this->formatValue($value))
->setParameter('{{ choices }}', implode(', ', $choices))

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 21, 2018

Contributor

should also be added to the multiple=true case

choices can be any value, i think we need to map all choices using formatValue() also.

This comment has been minimized.

Copy link
@nikophil

nikophil Dec 27, 2018

Author Contributor

yep, you're totally right - i've added to the multiple=true case and formatted the value

@nicolas-grekas nicolas-grekas added this to the next milestone Dec 24, 2018

@nikophil nikophil force-pushed the nikophil:validator-choice-constant branch from 1ebdef3 to 849d765 Dec 27, 2018

@nikophil nikophil force-pushed the nikophil:validator-choice-constant branch from 849d765 to 71dfa35 Dec 27, 2018

@xabbuh

This comment has been minimized.

Copy link
Member

commented Dec 27, 2018

@nikophil Can you please update the PR title and description to match the recent changes (they will be part of the git history)?

@nikophil

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

done 👍

@xabbuh

xabbuh approved these changes Dec 27, 2018

@fabpot

fabpot approved these changes Jan 2, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

Thank you @nikophil.

@fabpot fabpot merged commit 71dfa35 into symfony:master Jan 2, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Jan 2, 2019

feature #29658 [Validator] Choices constraint improvement (nikophil)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Validator] Choices constraint improvement

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

Hi,

here is a little improvement for the choice constraint:  expose a new `choices` wildcard for the messages, in order to provide a way to display easily the valid choices to the user.

thanks.

Commits
-------

71dfa35 add new `choices` wildcard in message

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jan 3, 2019

minor #10836 document "choices" wildcard in Choice validation constra…
…int (nikophil)

This PR was submitted for the 3.4 branch but it was merged into the master branch instead (closes #10836).

Discussion
----------

document "choices" wildcard in Choice validation constraint

Hello,

here is a little doc update in order to document the `choices` wildcard introduced by symfony/symfony#29658

Commits
-------

7764519 document "choices" wildcard in Choice validation constraint

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 7, 2019

minor #10810 [Validator] Explained how to use array constant in annot…
…ation (nikophil)

This PR was squashed before being merged into the 3.4 branch (closes #10810).

Discussion
----------

[Validator] Explained how to use array constant in annotation

Hi,

few days ago i tried to add a new feature to the `choice` validator constraint, by adding a new `choicesFromConstant` option.
symfony/symfony#29658
But then, i realized this was already supported.

I think this should be documented, as i think a lot of people don't know this feature.

thanks.

Commits
-------

7774aab [Validator] Explained how to use array constant in annotation

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.