[Intl] Optionally allow Kosovo as a component region

[llupa]

Q	A
Branch?	7.4
Bug fix?	yes
New feature?	yes
Deprecations?	no
Issues	Fix #32482, Fix #40020, Fix #54711
License	MIT

[llupa]

Edit:

In the past I attempted at one solution in this PR: #59693 where I got great feedback.

In the meantime, I tried to make something that:

• No need to regenerate the data set as this can take a very long time
• Tried to keep the size of generated data set as small as possible
  • looking at helper methods that even compress the already existing data set
• This solution while not revolutionary, keeps happy both aspects of the static functions: listing and targeting
  • When trying to find a smart approach to use meta.php to make the get* specific country data this would break the listing methods and vice versa. I am aware that I might have missed a very obvious solution, please let me know!
• Added a separate test class that includes XK and re-tests the entire data set with this code in it. Adapted certain tests to check both for backward compatibility and specifically for XK

Left to do: Update Changelog!
I wanted to know if this approach is good to begin with, but a documentation of the new ENV var is to follow.
Sidenote: Is the env var name enough? Needs to be renamed?

cc: @nicolas-grekas @ro0NL @connorhu my followup after trying very hard not re-writing the bundle readers.

[ro0NL]

LGTM. some tests for SYMFONY_ALLOW_OPTIONAL_USER_ASSIGNED would be nice.

[GromNaN]

I find this env var name not very explicit. It should mention "intl" for the component and "country" for the feature.

SYMFONY_INTL_ADD_COUNTRIES

This variable could contain a list of country codes if we add some others (XA, XA, XO, XI, XN, XU, XV, XX). That would let the developers pick what they need instead of being binary all or nothing.

It was proposed the other way with the deny list: #59693 (comment)

[llupa]

I find this env var name not very explicit. It should mention "intl" for the component and "country" for the feature.

I can get by adding _INTL_ but I really want to add some context, because I am noticing that the user assigned part is interpreted differently by different people when talking about this specific issue.

The User-Assigned codes are always from entities that have the power to assign them, this in turn for this component means that the ISO team in Geneva that maintains ISO 3166-1 will allocate them, then someone puts them in ICU and in turn we distill it further. That means an Alpha 2, Alpha 3 and Numeric code, all three, need to exist so that the getters that find each other (2-3-Nr.) work. If a developer wants to add custom codes, they need to add all three and we need to resolve the values correctly.

For Kosovo, because of reasons (I am happy to expand with relevant comments in this repo), the team decided to add it in DENYLIST which in turn through the years has caused issues. People working for projects that have a Spanish-centric view of country codes were happy since Spain does not recognize Kosovo, but users that worked for say a German-centric view of country codes and allowed purchases from Kosovo needed the code.

The user for this specific code (XK) is the European Commission ref and the implementation focuses on enabling the most recurring issue.

I am generally opposed to the idea to let developers add codes because from personal experience none usually really checks if the code(s) they want to assign is in use or not from User Assigned or if it collides with other ISO norms. As en example my former Product Owner started to use ERK for Erklärvideo, until we had the issue that ERK is the language code for Nafsan (ISO 639-3).

With that said, I am not opposed to implement this in a separate PR, I just really would like to have the confirmation from multiple core devs that this indeed OK and when custom User Assigned are introduced how do we validate if they are in actuality OK to be used? Maybe even have an RFC first.

[javiereguiluz]

@llupa I don't fully understand your comment.

Is Kosovo the only territory/country in that "not official yet" status? If there are more (or there will be more) then I think @GromNaN's idea makes sense so people can enable one or more of those "not official countries" in their apps.

I don't understand either the fear that people can make up codes and territories/copuntries. That's a risk/mistake on their side and I don't think we need to protect against that.

[ro0NL]

SYMFONY_INTL_ADD_COUNTRIES

the feature here is to allow existing ISO/ICU user-assgined codes, not give full control to developers

in that sense im fine with the current naming

AFAIK there's no real convention currently in SF on env keys, unless we find another example

for full explicity, maybe SYMFONY_INTL_ALLOW_ISO_USER_ASSIGNED=1

[llupa]

@javiereguiluz I want to be sure that this ENV param will only enable similar codes to Kosovo's case. As of now Kosovo is the only one that has this need, the other user assigned codes are Outlying Oceania, Pseudo-Accents, and Pseudo-Bidi.

If the "allow" is for these then for me it is fine. ✅

If it is for completely made up codes, then I would strongly recommend to have an RFC 🤓, because we need to have an agreement for value setting (XA, XAA, 999) and how do we tie them together at runtime as these will probably need their own new BundleReader implementation.

Edit:

I don't understand either the fear that people can make up codes and territories/copuntries. That's a risk/mistake on their side and I don't think we need to protect against that.

Honestly, we can do it, but it seems like a new functionality for this component for me, and I am already imagining the bug reports because their sf-intl allows it but it collides with some other OS intl or even php intl 😅

[ro0NL]

i agree with @llupa to be conservative by default

[llupa]

Hey all, so I implemented the following:

• ENV param name is more INTL explicit and keeps ISO glossar
• there is no optional anywhere left, now only refer to UserAssigned.
• dropped the ensure function and adopted the ??= proposal in a single method.

If you think this PR fixes the 3 linked issues above you could approve it. If you want something changed for specifically handling on those issues, let me know.

For the other topic of letting people create their own standard on top of ISO standard, I want to make a separate RFC and put some ideas how to approach this as I am certain at least one new BundleReader class needs to be created to support that.

Thanks ❤️

[connorhu]

I still think that this direction is over-engineered. We just need a setter method where the developer can specify where their custom files are. That way the developer can set the data in boot time. If the developer wants to make a list that includes Kosovo, or if they want to display historical things (e.g. Czechoslovakia), that's up to them.

[llupa]

@connorhu For formerly used codes I would open an RFC and a proposal of how to implement it.

They have an ALPHA-4 standard on top and this component does not support the format. Some of the formerly used codes collide with current ones too. For example today's Slovakia and former Sikkim share the same code.

[javiereguiluz]

Maybe this is technically required for some reasons, but I'd prefer to not have nullable booleans. Can we make this option truly binary and give it a false default value?

[GromNaN]

It should not be initialized by default. So that if it's used directly, an error will occur.

Suggested change

	private static ?bool $allowUserAssigned = null;
	private static bool $allowUserAssigned;

[ro0NL]

when it's null, we populate it from the env var

[llupa]

This was so that I could write this test that does in fact "simulate" env var behavior.

I am also not a fan of the nullable but I could not find a way to write a test that was clear what it was testing. Open to ideas as Roland was the one who wanted that test, and I also agree it is good to have.

Edit: I see the link does not work when UI is collapsed. I meant:

    public function testEnvVariableAllowsUserAssigned(): void
    {
        // when env param is not set
        $ref = new \ReflectionProperty(Countries::class, 'allowUserAssigned');
        $ref->setValue(null, null);

        $this->assertFalse(Countries::exists('XK'));

        // when env param is set to false
        $ref = new \ReflectionProperty(Countries::class, 'allowUserAssigned');
        $ref->setValue(null, null);

        putenv('SYMFONY_INTL_ALLOW_USER_ASSIGNED=false');

        $this->assertFalse(Countries::exists('XK'));

        // when env param is set to true
        $ref = new \ReflectionProperty(Countries::class, 'allowUserAssigned');
        $ref->setValue(null, null);

        putenv('SYMFONY_INTL_ALLOW_USER_ASSIGNED=true');

        $this->assertTrue(Countries::exists('XK'));
    }

[GromNaN]

There is a design issue that leads to use the reflection. The environment variable should not be the only way to define this configuration. We could add a static method to assign the value.

public static function setWithUserAssigned(bool $withUserAssigned): ?bool
{
    $previous = self::$withUserAssigned;

    self::$withUserAssigned = $withUserAssigned;

    return $previous;
}

The class being final, we can change the parameter type later to accept a list of code, we we decides to do so.

[Pierstoval]

Maybe you could keep the value as non-nullable and uninitialized, and (unfortunately for perfs) run each tests related to this var in a separate process, so that the var is truly reset every time. It is also closer to what happens in a real execution environment.

[llupa]

I am going with GromNaN's recommendation because it is something I considered myself. This could also cover cases when the country list you need to use is tied to a sf-user-account, so for some you have it with-user-assigned and for some without-user-assigned

[llupa]

So, I found a way to implement all ideas from all three.

• the internal property now is bool only, not nullable
• there is a public method to change this property at runtime if you wish to do so
• changed GHA a bit 🤏 so I can run the ENV tests in isolation (this can be done easier with PHPUnit 11)

Let me know. But I will have to pick up next Monday, as work calls now :)