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

CreditCardTest failing on windows #75

Closed
enumag opened this issue Oct 3, 2015 · 8 comments
Closed

CreditCardTest failing on windows #75

enumag opened this issue Oct 3, 2015 · 8 comments

Comments

@enumag
Copy link
Contributor

enumag commented Oct 3, 2015

Test for this card number fails on windows because a number this long can't be stored as an integer. Instead it becomes a float and when converted into string it is 6.3041E+15 instead of 6304100000000008.

In my opinion CreditCard::validate should only allow string arguments or at least throw an exception if a float is passed as parameter to prevent this error.

@soullivaneuh
Copy link
Contributor

@soullivaneuh
Copy link
Contributor

@ronanguilloux I can help to configure appveyor CI if you want. Just tell me! ;-)

@ronanguilloux
Copy link
Owner

64-bit platforms usually have a maximum value of about 9E18, except on Windows prior to PHP 7, where it was always 32 bit.
No final decision here, and no troll either (really), but IMHO, it is not the IsoCodes project responsibility to handle Microsoft Windows limitations on implementing of the integer type.
Quick-win: use Vagrant/Docker/WhateverVirtualMachineEngine instead.

@soullivaneuh
Copy link
Contributor

I understand your point of view about 32/64 bits issue.

But apart of this, do this make sens to validate an integer for a credit card serial? It should be a string IMHO.

If you accept #93, you will earn two thing: 32 bit compatibility and stricter comparison.

If I was a Windows user, I would prefer having the same return value on both 32/64 bit system.

@soullivaneuh
Copy link
Contributor

it is not the IsoCodes project responsibility to handle Microsoft Windows limitations on implementing of the integer type.

It's an understandable decision, but in this case I think you should add a note that you don't support any 32 bits OS anymore.

And not only Windows. 32bits linux system should have the same issue. We didn't see it on Travis because it run only 64bits builds AFAIK.

@ronanguilloux
Copy link
Owner

It's an understandable decision, but in this case I think you should add a note that you don't support any 32 bits OS anymore.

=> OK for that.

And not only Windows. 32bits linux system should have the same issue. We didn't see it on Travis because it run only 64bits builds AFAIK.

=> travis-ci/travis-ci#986 (comment) : not in current Travis roadmap.

@soullivaneuh
Copy link
Contributor

soullivaneuh commented Apr 27, 2016

=> travis-ci/travis-ci#986 (comment) : not in current Travis roadmap.

Yes, this is why I'm saying that 32bits linux systems must have this bug too, but we can't see it on Travis. 😉

My final opinion for this issue is: We should invalid integer values on this validator because passing an integer for a credit card serial make no sense. It's a serial, not a number.

And I think it not really worth to drop 32 bits support just for this case.

@ronanguilloux
Copy link
Owner

See #93 (comment) => no merge for now => put that in the fridge for a next major release.

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

No branches or pull requests

3 participants