- Issue created by @YazanMajadba
- Merge request !7175Issue #3435850: Convert Country code to ISO 3166-1 alpha-3 β (Open) created by YazanMajadba
- Status changed to Needs review
8 months ago 10:41am 25 March 2024 - Status changed to Needs work
8 months ago 10:44am 25 March 2024 The Needs Review Queue Bot β tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request β . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
- Status changed to Needs review
8 months ago 12:08pm 4 April 2024 - Status changed to Needs work
8 months ago 12:15pm 4 April 2024 - πΊπΈUnited States nicxvan
This is interesting, I think this needs two things.
1. Remove the comments after the Countries, that is throwing phpcs errors.
2. This will need a test, the nice thing is this is really easy to test.To create the test go to:
core/tests/Drupal/KernelTests/Core/Locale/CountryManagerTest.phpAdd a new test method called something like test twoCodeToAlphaThreeCode
Call your new iso2ToIso3 method with a known two code and assert that it returns the correct 3 code. - Status changed to Needs review
8 months ago 1:43pm 4 April 2024 - π―π΄Jordan YazanMajadba
@nicxvan Thanks for your feedback.
1-I removed the comments from the code.
2-I created a test method. - Status changed to Needs work
8 months ago 2:07pm 4 April 2024 The Needs Review Queue Bot β tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- Status changed to Needs review
8 months ago 2:53pm 4 April 2024 - Status changed to Needs work
8 months ago 3:04pm 4 April 2024 - πΊπΈUnited States nicxvan
Those assertions are not the same, so they will always fail. you probably want something like:
self::assertArrayHasKey('AC', $iso3_codes);
and
self::assertSame($iso3_codes['AC', 'ASC');You can then remove $expected_iso3_codes.
And rename iso3_codes to iso3Codes
- Status changed to Needs review
8 months ago 6:05pm 4 April 2024 - Issue was unassigned.
- First commit to issue fork.
- Status changed to RTBC
8 months ago 1:06am 6 April 2024 - πΊπΈUnited States smustgrave
Test-only feature https://git.drupalcode.org/issue/drupal-3435850/-/jobs/1241704
Seems like a reasonable request and core already maintains the other list so if ever a change is needed just update here too, same file.
Added a small typehint, super duper nitpicky.
LGTM
- Status changed to Needs review
8 months ago 1:12am 6 April 2024 - π³πΏNew Zealand quietone
I need the code of the country with ISO 3166-1 alpha-3 standard
Thanks for explaining your need. However, this looks like something that should be done in contrib or custom code. As far as I know, there is no need for this in core. Is this perhaps required by some other issue?
There is a core policy to help decide what is added to core. It is Criteria for evaluating proposed changes β . So far I think this is a won't fix.
- πΊπΈUnited States nicxvan
Going by the criteria I think it should be included, it hits most of the items on that list.
Aim: It does not conflict with the aims of the Drupal project as far as I can tell.
On the correct branch: It is on the correct branch.
Risk: Very low.
Coding standards: This follows the coding standards for Drupal.
Demand: While there isn't significant demand there is some demand and the user that requested it put in the effort. Also the guidelines do not quantify how much demand is required, though obviously more demand means something is more likely to get included.
Usage: This is the one that falls farthest short I think. It says that it will be used by a significant portion of the Drupal base. I would think this type of utility would still pass that bar, you can't use something if it doesn't exist.
Technical Debt: Very low.I agree this could be achieved in contrib or custom code, but for those users that do need this they will already have country manager injected and for users that don't need it this change does not add significant overhead.
I do recognize I'm not the one maintaining this, but from my perspective this should be included.
I'm curious if my evaluations align with the general core team's thought process, it would be helpful to know when looking at other feature requests in the future if I should encourage a user to use contrib for a feature.
Also @YazanMajadba if you have any additional insight into how this will be used and if there are other issues depending on it.
- Status changed to RTBC
8 months ago 1:55pm 8 April 2024 - πΊπΈUnited States smustgrave
Very well put @nicxvan don't have much to add other then agree. Seems like a nice feature to have that won't be burdensome to maintain.
- π¬π§United Kingdom longwave UK
To me this is won't fix, https://www.drupal.org/project/address β provides this (and much more address handling) in contrib via the
commerceguys/addressing
library: https://github.com/commerceguys/addressing/blob/master/src/Country/Count... - Status changed to Postponed: needs info
8 months ago 4:35pm 8 April 2024 - π¬π§United Kingdom catch
Given this is already provided by a well-used third party library I also don't see why we'd re-implement that in core. @longwave is also correct that we don't currently use the default country setting anywhere in core.
- π΅πΉPortugal introfini
For those looking to achieve this, I recommend using the suggestion by @longwave:
use CommerceGuys\Addressing\Repository\CountryRepository; function convert_two_letter_to_three_letter_country_code($two_letter_code) { $countryRepository = new CountryRepository(); // Get the country information based on the two-letter code. $country = $countryRepository->get($two_letter_code); if ($country) { // Return the three-letter country code. return $country->getThreeLetterCode(); } return null; }