[Validator] OneOf constraint vs Choice #6403

Closed
fabpot opened this Issue Dec 18, 2012 · 14 comments

Comments

Projects
None yet
7 participants
Owner

fabpot commented Dec 18, 2012

In PR #6360, the Choice constraint was renamed to OneOf. This PR fixes ticket #6324.

But as mentioned by @vicb and @Tobion, it might not be such a good idea for several reasons:

  • We are again breaking BC without a good reason. Choice was not a horrible name
  • The new name is probably worse than the previous one as the OneOf constraint has a multiple option. When set to true, the name is not accurate anymore

So, I propose that we revert this change.

ping @bschussek

Contributor

webmozart commented Dec 18, 2012

What about splitting it further into OneOf and SomeOf?

I really think that Choice is not self-explanatory enough. Remember your talk at Symfony Live in Berlin - we should do everything to make things easier for newcomers. Now consider a newcomer reading:

/** @Assert\Choice({"ONE", "TWO", "THREE"}) */
private $variant;

I think this is hard to understand (not for us, because we know about the "choice" form type).

On the other hand, consider:

/** @Assert\OneOf({"ONE", "TWO", "THREE"}) */
private $variant;

/** @Assert\SomeOf({"ONE", "TWO", "THREE"}) */
private $variants;

That relates to an english sentence: "the variant is one of ONE, TWO, THREE" and so is much more self-explanatory.

Contributor

vicb commented Dec 18, 2012

@bschussek very good point, but -1 still.

What about Type, Email, Max, Collection, ... should they be changed to be more english ?

IMO Choice is good enough, not worth breaking BC.

Contributor

webmozart commented Dec 18, 2012

What about Type, Email, Max, Collection, ... should they be changed to be more english ?

I think the other constraints are fine.

"assert that x is an email"
"assert that x has type y"

etc.

I noticed the bad naming of Choice when revising our constraints lately. The only other constraint that I found to be not self-explanatory is All, which should probably be merged into Collection, but that's a different topic.

Contributor

maoueh commented Dec 18, 2012

In my opinion, @Assert\OneOf is a better name than @Assert\Choice. For me, it is more clear what the assertion tries to check using @Assert\OneOf.

So, I vote to keep the new name.

Contributor

vicb commented Dec 18, 2012

Could you please explain (with an english sentence) how @Assert\OneOf(min = 2) is clear ?

Member

stof commented Dec 18, 2012

@vicb First, this is invalid as you are missing a required option (either choices or callback must be defined). And this is also why @bschussek suggested above to split it into OneOf and SomeOf

Contributor

vicb commented Dec 18, 2012

I won't comment the first part of your comment. About the second, that would be an un-necessary BC break (IMO) as I stated in my first comment.

Contributor

webmozart commented Dec 18, 2012

@Assert\SomeOf({"1", "2", "3"}, min = 2) makes perfect sense to me:

"Assert that x contains some of "1", "2", "3" and at least (a minimum of) 2"

The "min" option can probably be deprecated in favor of the new @Assert\Count(min = 2).

Contributor

ricardclau commented Dec 18, 2012

I like the SomeOf / OneOf approach... the only thing is that keeping BC is a bit more difficult (not a big deal, though) and this way, I gues "multiple" argument can be removed

If you decide to go on with the splitting into 2 constraints I can also help in the new PR

I'll keep an eye on this thread!

Member

weaverryan commented Dec 26, 2012

-1 from me, I don't think the name change is worth breaking BC and having old versions of docs and blog posts be Choice and new ones be OneOf.

It's impossible to put myself in the shoes of a beginner, but when I see the Choice example above, it makes good sense to me. Perhaps the OneOf is slight better, but not at the cost of BC. To me, it's one of those "I wish we had done XX differently, but no big deal, oh well" kind of things.

Thanks!

Owner

fabpot commented Dec 27, 2012

For the record, I'm also for reverting this change.

Owner

fabpot commented Dec 28, 2012

@bschussek I'm going to revert PR #6360 this week-end.

Contributor

webmozart commented Dec 29, 2012

Ok.

Owner

fabpot commented Dec 29, 2012

I've just reverted PR #6360

fabpot closed this Dec 29, 2012

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