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][Choice] Make strict the default option for choice validation #19257

Closed
wants to merge 5 commits into from
Closed

[Validator][Choice] Make strict the default option for choice validation #19257

wants to merge 5 commits into from

Conversation

peterrehm
Copy link
Contributor

@peterrehm peterrehm commented Jul 1, 2016

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

This is just the WIP as there are two options.

  1. Just change default which would only possible to introduce in 4.x or in 3.2 if this BC break is considered as acceptable
  2. Add a new option e.g. strictComparison which defaults to true in 4.x and deprecate the usage of the strict option for 3.2.
  3. Just deprecate strict = false and remove the option but I would be against that as we remove flexibility which might be wanted.

As per discussion I went ahead with option 3. We can then still decide if we want to remove the option entirely or eventually reenable setting strict to false in a later release.

@stof
Copy link
Member

stof commented Jul 1, 2016

Such BC break cannot be done in 3.x

@peterrehm
Copy link
Contributor Author

@stof I was unsure about that, as for me this is a serious bug in the current behaviour.

If that is not possible the only way would be option 2 and 3 where I would prefer 2.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 1, 2016

Could this target 4.x? Not sure this is common.. but master could trigger a deprecation error about changing behavior in 4.x right?

@peterrehm
Copy link
Contributor Author

master could introduce the deprecation, so option 2 or 3 should be possible.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 1, 2016

Yeah, but option 1 would be ideal right? I.e. option 1 & 2 are practically the same, it just proposes to rename the option.. which isnt necessarily needed imo.

@peterrehm
Copy link
Contributor Author

Yes, it would be great to fix it without the rename, lets see.

@nicolas-grekas
Copy link
Member

Options 2 or 3 are the only viable solutions, to preserve BC and enable a continuous upgrade path.

@xabbuh
Copy link
Member

xabbuh commented Jul 10, 2016

If noone has a good use case for being able to switch the behaviour of the constraint, I would deprecate setting the strict option to false and change the default value of it in 4.0.

@peterrehm
Copy link
Contributor Author

Updated accordingly. The open decision is if we want to deprecate the entire option or just the default value and as later still allow setting it to false.

@peterrehm
Copy link
Contributor Author

Should be good to go from my end now.

@peterrehm
Copy link
Contributor Author

Any further feedback?

@xabbuh
Copy link
Member

xabbuh commented Aug 8, 2016

Can you update the upgrade file for 4.0 too please? Besides that the changes look good to me.

@peterrehm
Copy link
Contributor Author

Updated. Besides that the BC Break can be removed as the current solution does not contain any BC break.

@fabpot
Copy link
Member

fabpot commented Sep 14, 2016

Thank you @peterrehm.

@fabpot fabpot closed this Sep 14, 2016
fabpot added a commit that referenced this pull request Sep 14, 2016
… choice validation (peterrehm)

This PR was squashed before being merged into the 3.2-dev branch (closes #19257).

Discussion
----------

[Validator][Choice] Make strict the default option for choice validation

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

This is just the WIP as there are two options.

1. Just change default which would only possible to introduce in 4.x or in 3.2 if this BC break is considered as acceptable

2. Add a new option e.g. `strictComparison` which defaults to true in 4.x and deprecate the usage of the strict option for 3.2.

3. Just deprecate strict = false and remove the option but I would be against that as we remove flexibility which might be wanted.

As per discussion I went ahead with option 3. We can then still decide if we want to remove the option entirely or eventually reenable setting strict to false in a later release.

Commits
-------

177c513 [Validator][Choice] Make strict the default option for choice validation
@fabpot fabpot mentioned this pull request Oct 27, 2016
@peterrehm peterrehm deleted the choice-validator branch January 10, 2017 19:04
@Strate
Copy link
Contributor

Strate commented Jan 22, 2018

How can I avoid deprecation warning without adding strict=true to each usage of @Assert\Choice?

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

Successfully merging this pull request may close these issues.

None yet

8 participants