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

[5.5] Let In validation parameters contain commas #21012

Merged
merged 3 commits into from
Sep 6, 2017
Merged

[5.5] Let In validation parameters contain commas #21012

merged 3 commits into from
Sep 6, 2017

Conversation

balping
Copy link
Contributor

@balping balping commented Sep 6, 2017

Prevoiusly

Rule::in([
    'alpha,beta',
    'gamma,delta'
]);

was parsed to in:alpha,beta,gamma,delta meaning that 4 values would pass this rule instead of the quite explicitly stated 2 possibilities.

This PR fixes the above bug by enclosing the listed values by double quotes. This is understood by the function str_getcsv which is used later on.

Because of introducing extra double quotes, it is necessary to escape existing double quotes (with a double quote, resulting double-double quotes, silly CSV standard...) , this is also implemented here.

@tillkruss tillkruss changed the title [5.5][bugfix] Let In validation parameters contain commas [5.5] Let In validation parameters contain commas Sep 6, 2017
@devcircus
Copy link
Contributor

Shouldn't the following work?
Rule::in([
'"alpha,beta",
"gamma,delta"'
]);

@balping
Copy link
Contributor Author

balping commented Sep 6, 2017

The validator should match the exact strings listed in that array. A developer should be not required to have knowledge about the behind-the-scenes usage of str_getcsv or about how escaping in that function works. It took me ~30 minutes to find out both and I'd like to save that time for others.

I dont know what you mean by 'work', but in the example provided by you, the only value passing that rule is the string

'"alpha,beta",
"gamma,delta"'

If your concern are new line characters, they are handeled correctly, I added tests to prove that.

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.

3 participants