Skip to content
This repository has been archived by the owner on Dec 11, 2020. It is now read-only.

Expand IBAN interface #607

Merged
merged 4 commits into from
Aug 24, 2015
Merged

Expand IBAN interface #607

merged 4 commits into from
Aug 24, 2015

Conversation

okj579
Copy link
Contributor

@okj579 okj579 commented Jun 15, 2015

Here I've:

  • Added implementations of bankAccountNumber to all existing, supported locales
  • Made made the iban() function public
  • Allow null as the $countryCode (where it chooses a random, supported country)
  • Added unit tests for all supported countries

This allows the explicit use of iban() for any locale and allows for the generation of IBANs from random countries.

The unit tests also warn (skip tests) about locales where bankAccountNumber is not implemented, even though an IBAN format is available. This currently warns about the "fake" locales at_AT and be_BE, which are deprecated in #594.

The one point I'm not sure on is the default value of iban($countryCode). I've set it to the current locale for IBAN countries and null (random) on the base class. For example, it would currently work like this:

Faker\Factory::create('it_IT')->bankAccountNumber; // 'IT66F5186468682C65NOP68ZYK9'
Faker\Factory::create('it_IT')->iban; // 'IT04U8856763833CPN8P3H3QDJ4'
Faker\Factory::create('it_IT')->iban(null); // 'AT681337042751049061'

Faker\Factory::create('en_US')->iban; // 'HR6628746829188144538'
Faker\Factory::create('en_US')->bankAccountNumber;
// InvalidArgumentException with message 'Unknown formatter "bankAccountNumber"''

@fzaninotto
Copy link
Owner

Great work! But I'm worried about the duplication of formatters (iban and backAccountNumber). Rather than making iban public, why don't you rename it to backAccountNumber and ban the iban formatter altogether?

@nineinchnick
Copy link
Contributor

That's why iban wasn't public in the first place. It's not a formatter, because not all countries use it. Many other formatters are bound to specific locale.

@okj579
Copy link
Contributor Author

okj579 commented Jun 15, 2015

True, but it would lead to bankAccountNumber being available (as an IBAN) for non-IBAN countries, for example in the en_US locale, where returning an IBAN for bankAccountNumber would be wrong. A new formatter for en_US could be written, but then it would have to be overwritten in every locale to prevent them from returning misleading values. IBANs would also once again be unavailable for many locales.

After looking at the example above again, I have another idea. I could make iban only available as a function call (not a property) with the $countryCode mandatory (explicit null allowed). This removes a lot of duplication of functionality between iban and bankAccountNumber.

@fzaninotto
Copy link
Owner

yep, I like your latest suggestion.

@okj579
Copy link
Contributor Author

okj579 commented Jun 15, 2015

iban() is now only available as a function call and the country code is mandatory. For a random country, null can be passed.

{
$countryCode = strtoupper($countryCode);
$countryCode = is_null($countryCode) ? array_rand(self::$ibanFormats) : strtoupper($countryCode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't array_rand be called on $ibanFormats keys, instead of values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I've missed that. BTW, maybe self::randomKey() should be used here? @fzaninotto often comments that mt_rand gives better randomness than lots of built-in shuffling PHP functions.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, array_rand() is verboten: it doesn't use the Mersenne Twister randomizer.

@okj579
Copy link
Contributor Author

okj579 commented Jun 16, 2015

Alright, I've switched it to self::randomKey

fzaninotto added a commit that referenced this pull request Aug 24, 2015
@fzaninotto fzaninotto merged commit 277d51d into fzaninotto:master Aug 24, 2015
@fzaninotto
Copy link
Owner

Thanks a lot!

@okj579 okj579 deleted the okj579-patch-1 branch August 25, 2015 18:36
@soullivaneuh
Copy link

This PR fixes a blocking error:

  [Symfony\Component\Debug\Exception\ContextErrorException]                                                                                        
  Warning: call_user_func_array() expects parameter 1 to be a valid callback, cannot access protected method Faker\Provider\fr_FR\Payment::iban()  

Could we have a new patch tag for this?

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

Successfully merging this pull request may close these issues.

4 participants