Deprecate other values than string for CreditCard #93
Conversation
| @@ -30,7 +30,6 @@ public function getValidCreditCards() | |||
| array('4903010000000009'), //Switch | |||
| array('4111111111111111'), //Visa | |||
| array('6304100000000008'), //Laser | |||
| array(6304100000000008), //Laser | |||
There was a problem hiding this comment.
I don't get the point of removing an existing valid test.
There was a problem hiding this comment.
This is not removed but added as invalid here: composer/composer#5249 (comment)
See #75 (comment)
There was a problem hiding this comment.
What's valid cannot become invalid without a strong good reason
There was a problem hiding this comment.
This is the goal of #75 discussion.
There was a problem hiding this comment.
yes obviously, but #75 is not about valid/invalid test dataset, but rather about valid/invalid 32bits OS. So array(6304100000000008) remains a 100% valid test.
There was a problem hiding this comment.
That depends of the issue finality. See #75 (comment).
If you agree that having a integer for a credit card make no sense, so this PR is the way to do.
Otherwise, I'll update or close it.
There was a problem hiding this comment.
Comment not relevant anymore, test is here: https://github.com/ronanguilloux/IsoCodes/pull/93/files#diff-391157587170daf579f5855197312e0aR41
|
Since now, we allowed |
|
@ronanguilloux I could rework my PR to make it BC with a deprecation message. 👍 |
|
BTW, it would be great to have |
soullivaneuh
force-pushed
the
credit-card-string
branch
from
e9d1d5a to
1479765
Compare
soullivaneuh
changed the title
|
@ronanguilloux I updated my PR on a BC way. Could you please take a look? |
|
Wait before merge: I think we should add an exception for 32 bits system and not pass integer values on test. Will work on it. cc @enumag |
|
Done. @enumag could you please checkout my fork on your 32bits system and tell us if test passed? Thanks. |
|
@ronanguilloux the deprecated message produce travis fail. We should implement |
soullivaneuh
force-pushed
the
credit-card-string
branch
from
d20e0fb to
4c2cb30
Compare
|
@ronanguilloux The PR is ready for me. 👍 I introduced legacy values for tests: ea8cad5 This is a smoother way than More info: http://symfony.com/doc/current/components/phpunit_bridge.html#mark-tests-as-legacy |
|
@ronanguilloux Are you planning to tag a new minor version after that? |
soullivaneuh
force-pushed
the
credit-card-string
branch
from
4c2cb30 to
1ab4f7e
Compare
Test for card number `6304100000000008` 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. CreditCard::validate should only allow string arguments. This is done in a BC way and should be updated for next major.
Integer credit card fails on 32bits system because the integer is too long. Method for 64bits detection: http://stackoverflow.com/a/5424121/1731473
soullivaneuh
force-pushed
the
credit-card-string
branch
from
1ab4f7e to
f25295d
Compare
|
@ronanguilloux PR rebased agains master. |
|
@ronanguilloux Sorry for the ping, any chance to get this merged before the next minor release? This PR is now completely BC. 👍 |
|
@soullivaneuh Actually my system and php are both x64. Can't help with 32bit. |
|
@ronanguilloux Does anything is missing to merge this one? Maybe do you want to integrate AppVeyor service for Windows testing? I can provide a configuration for that. |
|
ping @ronanguilloux Please tell me what do you want to do! 😉 |
|
@ronanguilloux Can I please have any input for this? Thanks. |
|
@soullivaneuh everything is in the (too long) #75 conversation. Summary: we don't support any 32 bits OS anymore. At max, you can PR a note on README |
|
Yes, but allowing only string is bc break. It would be great to prevent that before the next major with a deprecation warning. This PR is designed for that like I said in #93 (comment). So this one can be merged now, and then remove the deprecation notice on next major. BTW, we can support only PHP7 for next major (as 5.x is not maintained anymore) and take benefit of strict type. Tell me. If you prefer to break the compatibility directly on next major, I'll rollback my PR modification for that and we will be able to put it on the fridge. |
Use PHPUnit test suites instead of groups
Fixes #75.