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

Add allowBlank option to queryparam #886

Merged
merged 1 commit into from Dec 10, 2014
Merged

Conversation

mosch
Copy link
Contributor

@mosch mosch commented Oct 15, 2014

fixes issue #814

@mosch
Copy link
Contributor Author

mosch commented Oct 15, 2014

I'm not sure how the nullable option should work. i don't really get the difference. maybe this is redundant but the nullable option doesn't work?

@lsmith77
Copy link
Member

@ClementGautier can you have a look here?

@lsmith77 lsmith77 added this to the 1.5.0 milestone Nov 5, 2014
@lsmith77
Copy link
Member

ping

@ClementGautier
Copy link
Contributor

Sorry for the delay.

👍 because we already have a nullable option. (But, for me, as we already can do it using a Constraint we could instead drop nullable as well :P))

@mosch
Copy link
Contributor Author

mosch commented Nov 20, 2014

if (null === $value || '' === $value) {
  return;
}

Since the RegexValidator#L37 ignores everything that is null or an empty string, I think even if you're using a constraint with a regex that doesn't allow an empty string, the validation will pass.

So 👍 is a go for a merge?

@ClementGautier
Copy link
Contributor

@mosch You can combine NotNull and Regex with a All constraint, I think it could work. But yes, as I said I'm ok with allowBlank option.

@mosch
Copy link
Contributor Author

mosch commented Nov 20, 2014

great, i think i should go and extend the test and then we can go for the merge. anything else missing?

@mosch
Copy link
Contributor Author

mosch commented Nov 20, 2014

i think it could should also be added in the testExceptionOnValidatesFailure() test, but the class is confusing to me a litte. any advice?

@ClementGautier
Copy link
Contributor

@mosch I dont think its necessary. We only test strict cases on this test and, in this PR we only add "white cases" possibilities (which are covered by testValidatesConfiguredParam)

@lsmith77
Copy link
Member

lsmith77 commented Dec 2, 2014

I guess this is ready to merge?

@lsmith77
Copy link
Member

lsmith77 commented Dec 9, 2014

needs a rebase

@mosch
Copy link
Contributor Author

mosch commented Dec 10, 2014

did the rebase, everything should be fine now.

lsmith77 added a commit that referenced this pull request Dec 10, 2014
Add allowBlank option to queryparam
@lsmith77 lsmith77 merged commit 7650034 into FriendsOfSymfony:master Dec 10, 2014
@mosch
Copy link
Contributor Author

mosch commented Dec 10, 2014

👍 thanks

@lsmith77
Copy link
Member

@mosch can you have a look at #992 ?

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

Successfully merging this pull request may close these issues.

None yet

3 participants