Country field should display the "- Select -" option to match core behavior

There's a patch here, but the formatting looks suspicious, and there are no tests. Updating status.

Updated the issue summary to make the case that this is Drupal core default behavior and should be addressed.

A client asked for the country address field which is required on all our forms to not default to Afghanistan, as shown above. I updated to the latest version of Address (1.7.0) in hopes this would have been resolved. I found that the patch still applied cleanly and provided the desired effect. I second the call to getting this addressed.

I am currently working to re-roll the patch following Coding Standards and with tests. Should be up soon, but I am having issues getting FunctionalJavascript tests to play nicely with Lando.

Re-rolling patch. This is just added test coverage for this issue.

Okay, that test passed. It was not supposed to. I think I have resolved my local FunctionalJavascript testing woes. I'll circle back around tomorrow morning.

@jcandan - Once you are set let me know and I can test for you.

Re-rolled patch. It is a surprisingly simple fix. This patch includes test changes which fail if the fix is not applied.

I was finally able to run FunctionalJavascript testing locally. It turns out that the reason the test passed, before the fix was applied, was that the assertOptionSelected would not fail. This was because it did not find an empty value option, and therefore never asserted any option was selected. I have addressed this gap in this patch.

Hiding obsolete files.

@jcandan - Doesn't look like #10 works. I replaced the patch in #2 with #10 and the country fields first values went back to Afghanistan.

@johnpicozzi, that is very strange; this is working for me. What PHP version and Drupal version are you testing on? Did you set a default value? Make sure it is set to "None" in the field settings. As far as I can tell, if this patch is not working, it would not pass the test I provided.

Okay, I am getting a validation error at the field settings page.
```
The country "_none" is not valid.
```

I am looking into it.

Update - After re-saving the address field the new patch started working. RTBC +1

Re-rolled patch #10. This patch needs work.

This latest patch includes a set of Country field-type specific tests. The module currently does not include any tests of the Country field-type by itself, without the other Address fields. This patch includes these missing tests. It unfortunately includes a test that fails with the current fix for this issue.

The following validation error occurs after save after editing the field settings of a Country field-type field, and setting its default value to "None":
The [FIELD_NAME] "_none" is not valid.

We need to identify how we can allow the field config default value to be _none, while still ensuring the field widget is required, and _none is not valid.

Got it! Re-rolled #16.

Went back and had a look at the patch #2. Like #2, using a blank empty value instead of _none did the trick. It also helped me catch that my patch #10 missed a default value fallback when none is set and the field is required. Also fixes a hidden country field when not part of the address field-type. Also noticed #16 had a huge commented code chunk; whoops. :)

I think this patch catches several test gaps, and fills them nicely while addressing this issue. Although several issues are addressed, each fix and test change was necessary for and closely linked to this issue's fix. I believe it is ready for community review.

Just a bit more work. Just noticed that cardinality above one has an issue. If the user does "Add another item", and then wanted that item removed, the validation will not accept a blank value on that added item, which will not allow the user to submit the form, which might then remove the additional item were it to submit since it is blank.

Need to dig a little.

This patch significantly simplifies the fix and adds or modifies tests to deal with just the one issue: match core Select widget behavior for the Country widget; required or not, default to empty when no default specified.

For the Address widget, which includes a country select box, this patch adds an empty option. But does nothing to change its default value behavior, which is to default country to the first available valid option.

Added patch in which select a value option is also added in country list. Please review.

I have just added the - symbols to make it more like Drupal core default behaviour. Also added the translation function. Hopefully, this will help someone.

Thanks, everyone, for working on this. Agree this would be a big UI improvement!

Patches #22 and #23 are definitely not valid code, ignore all the prior work in here, etc. Hiding those from the file table.

#21 looks pretty good. However, I'm sad about the massive copy/paste nature of tests/src/FunctionalJavascript/AddressCountryDefaultWidgetTest.php. Yes, we want separate test coverage for the only-a-country widget, instead of only testing country as part of the address widget. Yay! However, I'd much rather see both of these extend a shared base class or have a trait where we can put everything duplicated between them in one place. That'll make it much easier to maintain these tests going forward.

A few other nits/concerns on a quick skim:

1. +++ b/src/Element/Country.php
   @@ -95,6 +103,8 @@ class Country extends FormElement {
   +        '#empty_option' => '- Select -',
   

   We need t('- Select -');.

2. +++ b/tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php
   @@ -508,6 +508,9 @@ class AddressDefaultWidgetTest extends WebDriverTestBase {
   +    // Assume an empty option is valid.
   +    // Filter out the empty option, if it exists, before we assert field values.
   +    $elements = array_filter($elements, function($k) { return !empty($k); }, ARRAY_FILTER_USE_KEY);
   

   Sorry, these comments don't really explain WTF is happening here. ;) I probably need to apply the patch and look at the rest of this, but on quick glance, these comments need help.

Cheers,
-Derek

@jcandan Are you interested in moving this forward based on my feedback in #24? I would like to get this fixed before the next release. Thanks!

-Derek

p.s. Also fixing up issue credits based on meaningful contributions here so far.

Nicely done. Yes, I agree these are valid points. What's your timeline for release? I'll try to put some time in soon to knock this out.

@jcandan: Thanks! No specific timeline on the next release. I don't know @bojanz's plans on that, either. I'm just wanting to get obvious wins into Git before it happens. ;)

Cheers,
-Derek

Addressed the concerns noted in #24.

@jcandan, thanks!

However, test classes don't have a $this->t() to call (as the test results show).

The bot should have already marked this issue 'Needs work' but somehow that didn't happen.

In addition to reverting all the t() -> $this->t() stuff, additional concerns/nits from looking at the patch:

1. +++ b/src/Element/Country.php
   @@ -73,18 +73,26 @@ class Country extends FormElement {
   +    // Filter out empty select options before count.
   +    if (count(array_filter($country_list)) == 1 && $element['#required']) {
   +      $only_available = key(array_filter($available_countries));
          $element['country_code'] = [
            '#type' => 'hidden',
   -        '#value' => key($available_countries),
   +        '#value' => $only_available,
   

   Doesn't look like we need the separate $only_available variable, we could just do the key(array_filter(...)) directly when setting #value.

2. +++ b/tests/src/FunctionalJavascript/AddressCountryDefaultWidgetTest.php
   @@ -0,0 +1,213 @@
   +  public static $modules = [
   

   This should be protected static $modules

3. +++ b/tests/src/FunctionalJavascript/AddressCountryDefaultWidgetTest.php
   @@ -0,0 +1,213 @@
   +  protected function setUp() {
   +    parent::setUp();
   +
   

   Haven't applied patch and inspected both test classes separately, but I wonder how much of this setUp() could be moved into a helper method in the trait and shared...

4. +++ b/tests/src/FunctionalJavascript/AddressWidgetTestTrait.php
   @@ -0,0 +1,156 @@
   +  /**
   +   * User with permission to administer entities.
   +   *
   +   * @var \Drupal\user\UserInterface
   +   */
   +  protected $adminUser;
   +
   +  /**
   +   * Address field instance.
   +   *
   +   * @var \Drupal\field\FieldConfigInterface
   +   */
   +  protected $field;
   +
   +  /**
   +   * Entity form display.
   +   *
   +   * @var \Drupal\Core\Entity\Display\EntityFormDisplayInterface
   +   */
   +  protected $formDisplay;
   +
   +  /**
   +   * URL to add new content.
   +   *
   +   * @var string
   +   */
   +  protected $nodeAddUrl;
   +
   +  /**
   +   * URL to field's configuration form.
   +   *
   +   * @var string
   +   */
   +  protected $fieldConfigUrl;
   +
   +  /**
   +   * The country repository.
   +   *
   +   * @var \CommerceGuys\Addressing\Country\CountryRepositoryInterface
   +   */
   +  protected $countryRepository;
   +
   

   If the trait is responsible for all these data members, I'd like it to also be responsible for initializing them. Again, I suspect most (if not all) of this is duplicate between the two classes. So let's add something like an initialize() method to the trait that's called by both setUp() functions (assuming all of setUp() can't be shared and moved wholesale into the trait).

Thanks again! Excited to see this getting fixed. :)

Cheers,
-Derek

Applied the suggestions from #29.

Sweet, thanks! Almost there. ;)

1. Subject: [PATCH 1/2] Issue #2995992 by jcandan: Supply empty option in Country
   

   I was rather confused at first since I didn't realize you did a git patch with separate commits. Made it a bit harder to read, and is confusing the hell out of interdiff locally. So for reference, here's a clean version of #30 as a single commit.

2. +++ b/tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php
   @@ -31,53 +32,6 @@ class AddressDefaultWidgetTest extends WebDriverTestBase {
   -  /**
   -   * {@inheritdoc}
   -   */
   -  protected $defaultTheme = 'stark';
   -
   

   We need this. @see #3093294: Specify the $defaultTheme property in all functional tests

3. +++ b/tests/src/FunctionalJavascript/AddressWidgetTestTrait.php
   @@ -51,6 +55,78 @@ trait AddressWidgetTestTrait {
   +   * A helper function for initializing Address widget tests.
   

   Method doc blocks are supposed to start with an action verb. "Initializes Address widget tests."

4. +++ b/tests/src/FunctionalJavascript/AddressWidgetTestTrait.php
   @@ -51,6 +55,78 @@ trait AddressWidgetTestTrait {
   +  public function initialize($field_name, $field_type, $label, $default_value_key, $form_display_widget_type) {
   

   This can be protected.

5. +++ b/tests/src/FunctionalJavascript/AddressWidgetTestTrait.php
   @@ -51,6 +55,78 @@ trait AddressWidgetTestTrait {
   +    $this->nodeAddUrl = 'node/add/article';
   

   Seems more logical to initialize this up next to where we create the article bundle.

Attaching a patch to fix all of the above, too. Seems cruel to have you do this. ;)

HOWEVER, I just tried testing this on a local dev site where I'm actually using address (and why I care about this issue). Once I remove the default_country (which I was only setting since this bug wasn't fixed), if I load the node form, I'm still defaulting to 'Afghanistan'. :( It looks like the changes to src/Element/Country.php are specifically undoing the benefit of this fix for the country element inside an address field. :( Why so? Seems we want to duplicate (*cough*) the test coverage and actually do the fix:

• +++ b/src/Element/Country.php
  @@ -73,18 +73,26 @@ class Country extends FormElement {
  +      // Is this country element part of the Address field-type?
  +      $is_address_field = in_array('address', $element['#parents'], TRUE);
  +      if ($is_address_field && $element['#required']) {
  +        // Fallback to the first country in the list if the default country is
  +        // empty and the field is required.
  +        $element['#default_value'] = key(array_filter($country_list));
  +      }
  

  Why are we doing this? I thought fixing this behavior was the whole point of this issue. ;)

• +++ b/tests/src/FunctionalJavascript/AddressCountryDefaultWidgetTest.php
  @@ -0,0 +1,213 @@
  +    // Required field with no default.
  +    // Country should be required and set to empty value option.
  +    $this->field->setDefaultValue([['value' => '']]);
  +    $this->field->save();
  +    $this->drupalGet($this->nodeAddUrl);
  +    $this->assertNotEmpty((bool) $this->xpath('//select[@name="' . $field_name . '" and boolean(@required)]'), 'Country is shown as required.');
  +    $this->assertOptionSelected($field_name, '', 'The empty option is selected.');
  

  We want this behavior for address fields, too. Let's include this in AddressDefaultWidgetTest.php.

• I still wonder if we could further share the test code itself (assertions, etc) between the two test classes. Haven't dug in deeply enough to see, but it still looks like there's a lot of (mostly) duplicate code. This could probably be a follow-up, unless you feel particularly motivated to further improve it now.

Thanks, @jcandan! I promise to get this into Git ASAP once we're happy. :)

Cheers,
-Derek

+++ b/tests/src/FunctionalJavascript/AddressCountryDefaultWidgetTest.php
@@ -0,0 +1,159 @@
+    $this->assertNotEmpty((bool) $this->xpath('//select[@name="' . $field_name . '" and boolean(@required)]'), 'Country is shown as required.');

Further nit: I think it's safer/better to avoid casting and such for cases like this. I tend to do:

$country = $this->xpath('//select[@name="' . $field_name . '" and boolean(@required)]');
$this->assertCount(1, $country, 'Country is shown as required.');

Slightly more verbose, but IMHO easier to read and avoids relying on typecasting.

I didn't mess with #32 (starting to get way out of scope for this bugfix).
But this addresses my concerns at the end of #31.
Tests pass locally.

Thoughts?

Thanks!
-Derek

+++ b/src/Element/Country.php
@@ -73,18 +73,14 @@ class Country extends FormElement {
+    // Filter out empty select options before count.
+    if (count(array_filter($country_list)) == 1 && $element['#required']) {
...
+        '#value' => key(array_filter($available_countries)),

I'm confused how or why we need to run array_filter

+++ b/tests/src/FunctionalJavascript/AddressWidgetTestTrait.php
@@ -0,0 +1,236 @@
+  /**
+   * Waits for jQuery to become active and animations to complete.
+   */
+  protected function waitForAjaxToFinish() {

There is

assertSession()->asserWaitOnAjax() now

@mglaman Thanks for the review!

#34.1:

I'm confused how or why we need to run array_filter

Because of the code comment you pasted:

// Filter out empty select options before count.

array_filter() is maybe overkill here, but we're simply purging the empty choice before we count if there's only 1 valid option and decide to hide the country entirely and set it as a form value.

#34.2: True, but this code totally pre-dates that. ;) Maybe this isn't the best issue to change that? Do that as a quick separate issue then re-roll this?

#34.2 is now at #3096129: Replace custom waitForAjaxToFinish() with assertWaitOnAjaxRequest(). Should be a quick fix, then we can re-roll this.

Assuming we commit #3096129: Replace custom waitForAjaxToFinish() with assertWaitOnAjaxRequest() quickly, here's a re-roll of this one to apply on top of that one. ;)

Don't queue this for testing until #3096129 lands since this won't apply.

#3096129: Replace custom waitForAjaxToFinish() with assertWaitOnAjaxRequest() is now in 8.x-1.x branch, so I queued #37 for testing.

Does anyone care about #34.1 enough to change it from an array_filter()? If so, what do you propose as the alternative? ;)

Anything else before this is RTBC?

Thanks,
-Derek

The patch and general approach make sense. Leaving it to dww to wrap up.

Updating title to match the final patch. This doesn't actually have anything special to do with Required fields, and the actual text for the choice is just - Select -.

Committed and pushed to 8.x-1.x.

Thanks, everyone!

Cheers,
-Derek

Opened a followup: #3116564: Selecting the "- Select -" country option does not hide address elements in some (TBD) conditions.

Unfortunately I have had to revert this issue in https://git.drupalcode.org/project/address/commit/b95f4d6 because it introduced several regressions inside Commerce.

The first one is at checkout: #3116564: Selecting the "- Select -" country option does not hide address elements in some (TBD) conditions. The second one is on the address book UI (user/1/address-book/add/customer), where no country is selected and no fields are shown (unlike at checkout), but then selecting a country and going back to "- Please select -" shows a "Revision log message" field which I have no clue where it came from. Furthermore, a number of tests are now failing. This shows that this change is potentially too disruptive for a regular release and might need to be something we leave for 2.x.

In any case, I am reverting for the time being, to be able to tag a D9-compatible Address release. We can then come back to this issue.

Alas. :( Sorry for the trouble. Not sure what I could have done to avoid commerce's problems. All of address.module tests were happy, and we added new tests for this, too.

Wonder if we need some additional tests in address module to make sure everything is happy inside IEF? I assume that's where most of the commerce trouble are coming from?

What can we do to move this forward? How can I or anyone else test this feature in the contexts that you were having trouble with it? Are we supposed to install commerce locally? Is there a commerce quickstart distro or something? Are there docs on running a commerce test suite that shows the problems this was having? Is there something we can/should do to flesh out the test coverage here in address.module itself to catch any of these problems?

Thanks!
-Derek

Commerce quick start: https://github.com/drupalcommerce/demo-project
#45 has the problematic urls (checkout/ and user/1/address-book/add/customer).
Tests are run like for any other Drupal module. There is a CustomerProfileTest which covers the customer profile inline form used at checkout, and a AddressBookTest functional test which covers the user pages. However, I don't think the tests are super helpful cause it's going to be hard to distinguish the expected failures (due to the select box having an extra option, for example) from the real ones.

It's very possible that the Address tests can be expanded to cover the problematic cases, but I don't know how, since I don't know what is causing the problems yet. My plan was to focus on user/1/address-book/add/customer first since it is a dumb entity form (unlike checkout which is layers of complexity).

Unassigning myself because I no longer work on Commerce. This will have to be coordinated with mglaman or jsacksick (the current Commerce maintainers).

Re-rolling patch from comment 10 for the latest release of address, 8.x-1.9.

I found issues with this patch when used in multi-step forms in particular. The field plugin and validators need to recognise the '_none' value as being empty, also when there is no default value the first available country is still selected.

Attached patch deals with these issues but I am sure further work is required. I am not sure why the default key has to be '_none' rather than an empty string for one, and ideally the behaviour when the field is required would be controlled by configuration.

Revised version of previous patch because it causes a notice error on country widgets.

Re-rolling patch from #37 against HEAD.

Fixing deprecations in AddressCountryDefaultWidgetTest

Just replace select with choose country

Reviewed and tested #57 patch and works as expected.

I am going with #56 which appears to work for my requirements (replacing a basic country selection with an Address field that only captures Country & Administrative Area). Not sure why anything other than "- Select -" would be a better placeholder so not applying #57 myself.

Thanks for the work!

Moving to 2.0.x-dev, as we've closed 8.x-1.x for any risky changes, and this one definitely counts (as evidenced by #45).

Also, bumping the status back to "needs review". Both #45 and #51 indicate that the patch needs further work (or at least exploration).

At a glance I can definitely say that we must not modify the address element like this:

diff --git a/src/Element/Address.php b/src/Element/Address.php
index b8df3be..f6a55c5 100644
--- a/src/Element/Address.php
+++ b/src/Element/Address.php
@@ -118,9 +118,7 @@ class Address extends FormElement {
     // Preselect the default country to ensure it's present in the value.
     $element['#default_value'] = (array) $element['#default_value'];
     $element['#default_value'] = self::applyDefaults($element['#default_value']);
-    if (empty($element['#default_value']['country_code']) && $element['#required']) {
-      $element['#default_value']['country_code'] = Country::getDefaultCountry($element['#available_countries']);
-    }
+

This is the big backwards compatibility break that is breaking Commerce tests.

For this patch to be committable, it needs to limit itself to the country element/field only, leaving the address UX unchanged.

Re-roll of the patch on the 2.x branch as starting point

Re: #64: I respectfully disagree. This *needs* to fix the UI of the actual address element, too, not just the country element. If you add a required address field to an entity, and do not provide a default value, the UI is quite bad. It defaults to selecting the first country in the list, which is typically not the right behavior. In all other cases, if you have a required field without a default, the form loads in a state that forces you to make a choice, and if you don't, you get a validation error. Here's a series of screenshots about it:

Current UI

Initial state of proposed UI

Browser validation error if you don't select a country

Working UI once you select a country

Sure, this isn't backwards compatible, and commerce core (and other modules that make use of address fields) might need to be modified to work with this behavior. I'm okay with putting this into a 2.1.x branch. But I'm not okay saying that only the country element can be changed, and that we're going to leave the current default-to-the-first-in-the-list behavior for address elements indefinitely.

For right now, I'm going to work on turning this into an MR, make sure it all applies cleanly to 2.0.x branch, and all the address tests are working fine. If we create a 2.1.x branch for this (and the other RTBC new features), this will merge to that branch instead of 2.0.x. I pinged folks in #commerce Slack about this, and I'm willing to coord with the Commerce Core maintainers to make sure we handle this in a way that doesn't break anyone's sites. But I'm not okay leaving this broken "forever". 😅

Thanks,
-Derek

Okay, I took patch #37 (what I had previously committed), and turned it into an MR against 2.0.x.
Interdiff in #56 was helpful, so I used that.
I had to fix quite a bit of other stuff to get this to pass locally (on a 10.5 test site).

The pipeline is failing here, but it seems our pipelines are all sorts of messed up right now 😬 😅
https://git.drupalcode.org/project/address/-/pipelines

Probably need to pivot to cleaning that up before I get much further with this.

But the MR's plain diff is now applying happily to a local clone of a client site where I need this fixed. And it seems to work as expected, so that's all positive. 😂

Unassigning myself for now, and moving to NR, although there might be additional work to get this actually committable...

We just ran into this and indeed this is confusing and for users it's a very risky issue, because on required (=important!) fields users potentially won't see that "Afghanistan" is selected. The core validation to show an error if nothing was explicitly selected is the only valid one UX-wise.

Additionally, it's confusing for site-builders and administrators to have a different behaviour here.
And if anyone wants this, they can still preselect the default value. So this hidden bug definitely should get fixed. :)

Thanks for your great work and feedback on this @dww!

PS: Regarding the discussion on Address vs. Country: Either we come to a good conclusion here or I'd vote to split the more complicated Address fix into a follow-up to get this fixed for country! Still I agree finally it should be consistent with core.

Please note that the test pipeline is already broken upstream for 2.0.x-dev!

I left a comment on the MR, but would be great to be reviewed by @bojanz and @dww.

Static patch of the current MR for the time being.

Looks like there are several related issues caused by this special treatment. So there are further good reasons to fix this.

If needed perhaps special handling can be configured, if needed.