Skip to content

[Validator] Fix CardSchemeValidator double violation #6603

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

Closed
wants to merge 1 commit into from

Conversation

stloyd
Copy link
Contributor

@stloyd stloyd commented Jan 7, 2013

Squashed version of #6450 with CS fixed.

…non-numeric.

Making scheme option accept strings in addition to arrays.
@ricardclau
Copy link
Contributor

Yesterday I did a pull request regarding CardSchemeValidator with some fixes that you can see at #6583

Unless @fabpot or any other core members have further comments, the only thing that is missing is to add further information inside comments about the regexps, so I expect to have it done tonight.

Do you think it makes sense to join both PR in one?

@fabpot
Copy link
Member

fabpot commented Jan 7, 2013

Yes, merging the 2 PRs would be good indeed.

@ricardclau
Copy link
Contributor

If @stloyd and @Sgoettschkes are ok with it I can join both PR.

I had plans to finish mine tonight with the documentation so if it is ok to everyone I can have both things done

@fabpot
Copy link
Member

fabpot commented Jan 7, 2013

Closing in favor of #6583

@fabpot fabpot closed this Jan 7, 2013
fabpot added a commit that referenced this pull request Jan 7, 2013
This PR was merged into the master branch.

Commits
-------

5be0042 better regexp, more test cases, added comments about each credit card
cc278af [Validator] Fix `CardSchemeValidator` double violation when value is non-numeric. Making scheme option accept strings in addition to arrays.

Discussion
----------

[Validator] Improve regexp for Credit Cards and some more tests

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets:
Todo: Ensure these regexps are proper (credit card validation is always a pain)
License of the code: MIT
Documentation PR:

Regarding Cases excluded from new Regular Expressions:

- Credit card lengths should be respected, these regexp cover lengths in http://en.wikipedia.org/wiki/Bank_card_number
- Visa length can only be 16 and 13 (older ones)
- Diners Cards starting by 5 come from a joint venture between Diners Club and MasterCard, and should be processed like a MasterCard (according to http://www.regular-expressions.info/creditcard.html).
- There seems to be JCB cards starting by 2131 and 1800, I could find them is some places, also found these numbers being tested in Credit Card generators, but some people don't cover them. I don't know their story either

Any comments will be much appreciated!

---------------------------------------------------------------------------

by fabpot at 2013-01-06T19:33:27Z

Thanks for working on this. It would be very valuable if you can add information about these regexes as comments (with links to relevant sources -- like what you've done in the PR description). Thanks.

---------------------------------------------------------------------------

by ricardclau at 2013-01-06T21:01:52Z

Always glad to be able to contribute a little bit

@fabpot you mean @link / @see PHPDoc inside CardSchemeValidator.php? Or further comments in this discussion before adding them?

---------------------------------------------------------------------------

by fabpot at 2013-01-06T21:16:48Z

The more information we can add in the class, the better it is.

---------------------------------------------------------------------------

by ricardclau at 2013-01-07T20:56:05Z

I've added comments and included code from #6603 as I've said there. If you need something else, please let me know, once this is merged, #6603 can also be closed

---------------------------------------------------------------------------

by fabpot at 2013-01-07T21:41:40Z

Can you keep the commit from #6603 to keep ownership?

---------------------------------------------------------------------------

by ricardclau at 2013-01-07T21:44:16Z

I actually have thought about that... let me try my git skills :)

---------------------------------------------------------------------------

by ricardclau at 2013-01-07T21:59:16Z

There you go!
@stloyd stloyd deleted the bugfix/card_scheme branch January 7, 2013 23:29
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.

4 participants