Problem/Motivation

A required Country field does not match core's "List (text)" field's "Select list" widget required field behavior.

When a List (text) field is marked required and the form display widget chosen is "Select list", then the "- Select a value -" option is shown. A required Country field does not show this option.

Screenshot showing the difference in behavior between address country widget and core select list.

Note: If this field is overlooked, the user will inadvertently submit a value they did not explicitly choose and may be incorrect.

Original report by edsoncarlos

Add "Select" option if field is required
Add "Select" option in the country list if field is required. But a country still must be selected.

CommentFileSizeAuthor
#57 2995992-57.patch25.53 KBRahaf Albawab
#56 interdiff_54-56.txt1.66 KBjacktonkin
#56 2995992-56.patch25.52 KBjacktonkin
#54 reroll_diff_37-54.txt3.91 KBjacktonkin
#54 2995992-54.patch25.48 KBjacktonkin
#53 address-required-country-field-empty-option-2995992-53.patch6.92 KBalfaguru
#52 address-required-country-field-empty-option-2995992-51.patch6.74 KBalfaguru
#50 address-required-country-field-empty-option-2995992-50.patch3.28 KBethomas08
#37 2995992.33_37.interdiff.txt1.72 KBdww
#37 2995992-37.only-after-3096129.patch25.57 KBdww
#33 2995992.31_33.interdiff.txt3.11 KBdww
#33 2995992-33.patch26 KBdww
#31 2995992.30_31.interdiff.txt3.33 KBdww
#31 2995992-31.patch24.54 KBdww
#31 2995992-30.one-commit.patch24.37 KBdww
#30 interdiff_28-30.txt8.44 KBjcandan
#30 address-required-country-field-empty-option-2995992-30.patch45.49 KBjcandan
#28 interdiff_21-28.txt25.17 KBjcandan
#28 address-required-country-field-empty-option-2995992-28.patch27.4 KBjcandan
#23 address_required_country_field_empty_option-2995992-23.patch426 bytesmarko.romsak
#22 fixedfirstvalue-2995992-22.patch1.3 KBJuhi Rathi
#21 address-required-country-field-empty-option-2995992-21.patch17.28 KBjcandan
#18 interdiff_16-18.txt10.08 KBjcandan
#18 address-required-country-field-empty-option-2995992-18.patch19.26 KBjcandan
#16 interdiff_10-16.txt16.14 KBjcandan
#16 address-required-country-field-empty-option-2995992-16.patch19.63 KBjcandan
#10 address-required-country-field-empty-option-2995992-10.patch3.28 KBjcandan
#7 address-required-country-field-empty-option-2995992-7.patch1.45 KBjcandan
#4 Screen Shot 2019-06-04 at 10.59.48 AM.png32.95 KBjcandan
#2 add-select-option-2995992-1.patch2.43 KBedsoncarlos

Issue fork address-2995992

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

edsoncarlos created an issue. See original summary.

edsoncarlos’s picture

bojanz’s picture

Status: Active » Needs work

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

jcandan’s picture

Title: Add "Select" option if field is required » Required Country field should display the "- Select a value -" option to match core behavior
Category: Feature request » Bug report
Issue summary: View changes
FileSize
32.95 KB

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

johnpicozzi’s picture

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.

jcandan’s picture

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.

jcandan’s picture

jcandan’s picture

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.

johnpicozzi’s picture

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

jcandan’s picture

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.

jcandan’s picture

johnpicozzi’s picture

@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.

jcandan’s picture

@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.

jcandan’s picture

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

I am looking into it.

johnpicozzi’s picture

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

jcandan’s picture

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.

jcandan’s picture

jcandan’s picture

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.

jcandan’s picture

jcandan’s picture

Status: Needs review » Needs work

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.

jcandan’s picture

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.

Juhi Rathi’s picture

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

marko.romsak’s picture

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.

dww’s picture

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

dww’s picture

@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.

jcandan’s picture

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.

dww’s picture

@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

jcandan’s picture

dww’s picture

Status: Needs review » Needs work

@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

jcandan’s picture

dww’s picture

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

dww’s picture

+++ 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.

dww’s picture

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

mglaman’s picture

+++ 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

dww’s picture

@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?

dww’s picture

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

dww’s picture

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.

dww’s picture

#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

bojanz’s picture

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

Chris Matthews’s picture

  • dww committed 502ee52 on 8.x-1.x
    Issue #2995992 by jcandan, dww, edsoncarlos: Country field should...
dww’s picture

Title: Required Country field should display the "- Select a value -" option to match core behavior » Country field should display the "- Select -" option to match core behavior
Status: Needs review » Fixed

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

bojanz’s picture

bojanz’s picture

Status: Closed (fixed) » Needs work

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.

dww’s picture

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?

dww’s picture

Assigned: Unassigned » bojanz

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

bojanz’s picture

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).

bojanz’s picture

Assigned: bojanz » Unassigned

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

ethomas08’s picture

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

alfaguru’s picture

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.

alfaguru’s picture

alfaguru’s picture

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

jacktonkin’s picture

Status: Needs work » Needs review
FileSize
25.48 KB
3.91 KB

Re-rolling patch from #37 against HEAD.

Status: Needs review » Needs work

The last submitted patch, 54: 2995992-54.patch, failed testing. View results

jacktonkin’s picture

Status: Needs work » Needs review
FileSize
25.52 KB
1.66 KB

Fixing deprecations in AddressCountryDefaultWidgetTest

Rahaf Albawab’s picture

Just replace select with choose country

shivamitakari’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested #57 patch and works as expected.

The last submitted patch, 50: address-required-country-field-empty-option-2995992-50.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 52: address-required-country-field-empty-option-2995992-51.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

sp3boy’s picture

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!

bojanz’s picture

Version: 8.x-1.x-dev » 2.0.x-dev
Status: Reviewed & tested by the community » Needs review

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).

bojanz’s picture

Status: Needs review » Needs work

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.