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

iso_code in class Country definition and table schema does not match with the actual usage of iso_code property #24254

Open
adeelq opened this issue Apr 28, 2021 · 4 comments
Labels
1.7.7.3 Affects versions CO Category: Core Feature Type: New Feature Languages Label: Which BO under menu is concerned Needs Specs Status: issue needs to be specified

Comments

@adeelq
Copy link

adeelq commented Apr 28, 2021

Describe the bug

Not sure how to explain that this as a bug. I'm gonna try my best to explain :) I'm working on an integration for PrestaShop, while i was digging through APIs and schemas i stumble upon something which does not add up.

Here It states that public $iso_code; class property size is 2

string 2 letters iso code

However here in class static property $definition the size is defined 3

'iso_code' => ['type' => self::TYPE_STRING, 'validate' => 'isLanguageIsoCode', 'required' => true, 'size' => 3],

And here in create syntax size is also defined 3

'iso_code' varchar(3) NOT NULL,

Expected behavior

The size should be consistent. I searched everywhere in PrestaShop for usage of class country property $iso_code and it is used as 2 for size. For Example here country data provider is using 2 letter iso code as expected.

In both of these places here and here size should be changed to 2 from 3.

Also here for validation 'validate' => 'isLanguageIsoCode' is being used. PrestaShop is using isLanguageIsoCode to validate both the ISO 2 letter country code and ISO 2 letter language code. However in actual validation class and method Validate::isLanguageIsoCode the regex pattern '/^[a-zA-Z]{2,3}$/' checks for 2 to 3 characters which is also misleading.

Additional information

  • PrestaShop version: 1.7.7.3
  • PHP version: 7.3
@hibatallahAouadni
Copy link
Contributor

Hello @adeelq

Thanks for the details 🚀
Well, you're right! this is a kind of misleading.

Ping @PrestaShop/prestashop-core-developers could someone please explain why we declare the $iso_code sometime 2 and other times 3?

@hibatallahAouadni hibatallahAouadni added 1.7.7.3 Affects versions Bug Type: Bug CO Category: Core Languages Label: Which BO under menu is concerned Needs Specs Status: issue needs to be specified Waiting for dev Status: action required, waiting for tech feedback labels Apr 29, 2021
@matks
Copy link
Contributor

matks commented May 14, 2021

Ping @PrestaShop/prestashop-core-developers could someone please explain why we declare the $iso_code sometime 2 and other times 3?

@eternoendless will have the answer. I think it's because at the beginning 2-letter ISO codes were used but it was later found 3 were needed so code relying on 2-letter ISO code might be obsolete

@eternoendless
Copy link
Member

I researched a bit and it looks like this has been like this since at least 2008. IMO the initial idea must have been to support both ISO 3166-1 alpha-2 and alpha-3, but I don't think anyone cares about alpha-3 really.

There's also a historical confusion between country iso codes and language iso codes (hence the isLanguageIsoCode)

In any case, I think we should pick one or the other, and since we use the alpha-2 codes already, it will probably be better to limit the field size to 2.

@eternoendless eternoendless added Improvement Type: Improvement Needs Specs Status: issue needs to be specified and removed Bug Type: Bug Needs Specs Status: issue needs to be specified Waiting for dev Status: action required, waiting for tech feedback labels May 19, 2021
@eternoendless
Copy link
Member

There's probably some research to be done before making a final decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.7.3 Affects versions CO Category: Core Feature Type: New Feature Languages Label: Which BO under menu is concerned Needs Specs Status: issue needs to be specified
Projects
None yet
Development

No branches or pull requests

5 participants