Background:
In order to fix bug #2392059: Choosing -None- still saves a row in the database, with the deeper cause being Core bug #1253820: It's impossible to submit no value for a field that has a default value, D7 addressfield had to #2392863: Remove the default values instance setting, introduce a default country instance setting.

When porting addressfield to D8, we had to stick with this workaround, because at that time the Core bug still wasn't resolved. Therefore, address tries to disable Core's default value functionality while providing a 'default country' widget setting. The widget is then "manually" initialized with the 'default country' widget setting and otherwise with empty values.

Problems:
This custom and quite peculiar workaround succeeds in fixing the original issue, but added quite some otherwise unnecessary code, and introduced a UX inconsistency compared with all (or most) other Drupal field types. In the original post, @dww also discovered that our 'default country' widget setting, once saved, still does make its way via Core to the field settings, therefore introducing new bugs, such as #2760387: Default country is imposed on fields with an empty address (which currently prevents changing the default country at all). While for that one we found a rather shallow #access FALSE fix, the situation remains unsatisfactory.

Solution:
Further research (#4/#5) however revealed that in mid 2016, the underlying Core bug #1253820: It's impossible to submit no value for a field that has a default value was fixed in D8, starting with the D8.3 branch. Now that we can finally do better than D7 addressfield, there are many reasons to remove all our custom workarounds and return to Core's default value mechanism.

Plan:

  1. Stop disabling the default value functionality.
  2. Remove the "Default country" widget setting, and write an update hook that transfer it to the default value.
  3. The widget must add a #process / #after_build callback that makes all address fields optional if it detects it's being rendered inside the default value form.
  4. Provide an update hook that replaces the "site default" setting with the actual 'site default' country
  5. Figure out how to handle the "Site default" setting for new fields:
    a) Preselect the site default on the default value widget only.
    b) Apply the site default inside the widget if the default value is empty.

Original report:

While investigating #2760387: Default country is imposed on fields with an empty address (which currently prevents changing the default country at all) I uncovered a deeper bug in how the default value for address fields is being handled.

Commit 5a6a1317 attempted to hide the default value from the field config form simply by setting #access to FALSE via address_form_field_config_edit_form_alter():

function address_form_field_config_edit_form_alter(&$form, FormStateInterface $form_state) {
  $form['default_value']['#access'] = FALSE;
}

However, once the field widget has a default country selected, the next time you save the field settings form, the default value is actually set to whatever country you had in your widget settings. Since the "upstream" field/entity code is adding that default_value to the form, via the default widget, the value is being saved, even if the end user can't see it (or change it) via the field settings form. The field settings yaml reflects this if you export the field configuration.

For example, brand new field, never set a default county via widget settings:

langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_address
    - node.type.directory_listing
  module:
    - address
id: node.directory_listing.field_address
field_name: field_address
entity_type: node
bundle: directory_listing
label: Address
description: ''
required: false
translatable: false
default_value: {  }
default_value_callback: ''
settings:
  available_countries: {  }
  fields:
    administrativeArea: administrativeArea
    locality: locality
    dependentLocality: dependentLocality
    postalCode: postalCode
    sortingCode: sortingCode
    addressLine1: addressLine1
    addressLine2: addressLine2
    organization: '0'
    givenName: '0'
    additionalName: '0'
    familyName: '0'
  langcode_override: ''
field_type: address

Set a default country in the field widget settings, visit the field settings form, submit, and now you see this:

langcode: en
status: true
dependencies:
  config:
    - field.storage.node.field_address
    - node.type.directory_listing
  module:
    - address
id: node.directory_listing.field_address
field_name: field_address
entity_type: node
bundle: directory_listing
label: Address
description: ''
required: false
translatable: false
default_value:
  -
    langcode: null
    country_code: CA
    administrative_area: ''
    locality: ''
    dependent_locality: null
    postal_code: ''
    sorting_code: null
    address_line1: ''
    address_line2: ''
    organization: ''
    given_name: ''
    additional_name: null
    family_name: ''
default_value_callback: ''
settings:
  available_countries: {  }
  fields:
    administrativeArea: administrativeArea
    locality: locality
    dependentLocality: dependentLocality
    postalCode: postalCode
    sortingCode: sortingCode
    addressLine1: addressLine1
    addressLine2: addressLine2
    organization: '0'
    givenName: '0'
    additionalName: '0'
    familyName: '0'
  langcode_override: ''
field_type: address

Remove the default country via the widget settings, re-save the field settings, and the original default_value persists in the field configuration. Set it to a different country, and the field settings still show the original default_value.

Due to #2760387: Default country is imposed on fields with an empty address (which currently prevents changing the default country at all), in practice this means you can never change the default country. However, I don't think the fix there is sufficient. It's a good fix for another bug (I'm about to re-title it to more accurately reflect what it's fixing -- something like "default country is being forced onto fields that want an empty address"), but it hides these deeper problems of the field settings actually having a default value set, even though we're attempting to avoid that.

Reasons I think this is a bug worth fixing properly:

A) It's buggy and weird to have field settings in the field config YAML that aren't being honored /used by the field.

B) There are clearly parts of the code that are still honoring these settings, even though we don't intend them to be set or used at all.

So, I think we need a stronger fix than #access FALSE to hide the default value from the field settings page, and perhaps a DB update to purge those entries from existing field configurations in the wild.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

dww’s picture

Related issues: +#2825923: Default Address

#2825923 is related -- it's a feature request for just being able to set a default value like normal. ;)

dww’s picture

Had a brief chat in IRC with bojanz about this. He pointed me to the addressfield issue queue where this stuff has been discussed at great length. In particular:

#2392863: Remove the default values instance setting, introduce a default country instance setting
#2392059: Choosing -None- still saves a row in the database
#2392855: Provide a hook for altering address defaults
...

So yeah, we need to really hide the default value and ensure Entity API and/or Field API aren't stashing a default value.

dww’s picture

Interesting. The core bug that caused addressfield to go this route has been fixed in D8: #1253820: It's impossible to submit no value for a field that has a default value. So maybe it's possible to do better in D8 address than we could in D7 addressfield? Needs further research and testing.

bojanz’s picture

Researched.

You are right, the problem we are working around no longer exists. And our workaround is now buggier than the actual functionality that we are disabling.

Our basic plan is this:
1) Stop disabling the default value functionality.
2) Remove the "Default country" widget setting, and write an update hook that transfer it to the default value.
3) The widget must add a #process / #after_build callback that makes all address fields optional if it detects it's being rendered inside the default value form.

An open question is how to handle the "Site default" setting that the widget currently supports. The update hook can look up the actual site default and set that on the default value instead. For new fields we have two options:
a) Preselect the site default on the default value widget only.
b) Apply the site default inside the widget if the default value is empty.

Addressfield used to do b).
I prefer a) because it's more explicit, but opinions welcome.

bojanz’s picture

Title: Default value saved into field settings and partially used when the intention is to hide it » Re-enable the default value functionality for Address fields
Category: Bug report » Task
heddn’s picture

Issue summary: View changes
jsacksick’s picture

Just posting my WIP patch here (it's far from being complete as tests need to be updated etc)...

Since the "none" option doesn't have an #empty_value, there's currently no way to distinguish a "default value" that was never set and an explicit "none" selection (so setting the site country, only for the initial default value configuration isn't possible atm).

Several tests need to be updated and haven't started to work on the update hook.

jsacksick’s picture

Tests are passing with the attached patch.

The patch doesn't set the default country on the default value form to the site default.

Additionally the event for setting initial default values isn't fired anymore, therefore, the test for testing this behavior has been removed.

AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @jsacksick

I tested #9 on my site and it works well for me

The last submitted patch, 8: address-wip-on-default-value-2838457-8.patch, failed testing. View results

bojanz’s picture

Status: Reviewed & tested by the community » Needs review

This patch is not complete, so it can't be RTBC.

scoff’s picture

Thanks for the patch.

The only thing I'm missing now in 1.4 is a way to enforce defaults per element. Hidden/Required/Optional/ + Default or Disabled or Read-only
Not sure if it's worth including in this module, though.

ohmdesbois’s picture

Patch #9 work for me
Thanks

dww’s picture

Re: #12: @bojanz: What's missing before you consider this complete? It's not clear from your prior comments in the issue. Thanks.

Re: #13: @scoff: See #2514126: Add field settings for global overrides of required/optional behavior

Cheers,
-Derek

realityloop’s picture

Status: Needs review » Needs work

@dww

#9 requires you to mark fields as optional to be able to set default values where some are not pre-filled (it will fail saving on validation of required fields otherwise). Once you have set the default values and saved you can then edit again and change the optional values back to required and save (but it disables saving changes again after that as it will fail validation again).

heddn’s picture

Just popping in here to mention, the work in here doesn't help #2951637: Provide more context to initial values event.

Pancho’s picture

Issue summary: View changes

Updated issue summary.

Pancho’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
1.41 KB
14.17 KB

Here's a patch fixing the problem described in #16 (input is validated against the field definition, so to allow empty default values on otherwise required fields we need to disable validation rather than just non-required widget styling.) Tested it a bit manually, and everything seems to work as expected! :)

We however still need to figure out a few things, including what we are going to do with the now stale InitialValuesEvent. We could deprecate, rename and repurpose it to a DefaultValuesEvent triggered whenever default values are changed. Is that something somebody might need?

By any definition this is a major task, so recategorizing.

dww’s picture

Calling #3033847: [PP-1] No initial values when address field is translatable a child of this, since I think that, too, would be fixed as a side effect of doing this properly.

Haven't reviewed the latest patches, but I anticipate much goodness. ;)

I have no idea why we'd need an event when the default field values change for this one field type. We don't have that anywhere else (AFAIK), not sure we need it here. I vote full deprecate and move on.

Pancho’s picture

Assigned: Unassigned » Pancho
Status: Needs review » Needs work

Thanks for your response.

I'm fine with plain-deprecating the stale event. As the core patch was fixed in the D8.3 branch only, this however has to go into a 8.x-2.x-dev branch that only supports D8.3+.

I'll try and get this one finished by the end of the week. The update hook needs both some manual and automated testing, too.

Pancho’s picture

Status: Needs work » Needs review

Or: are we actually starting a new 8.x-2.x-dev branch that only supports D8.3+? Or do we simply require D8.3 instead?
In the former case we would have to deprecate. In the latter case we could just remove the event.
Think we need to figure that out before putting more work into the patch.

heddn’s picture

8.3 is no longer supported. The only version of drupal that has even some support is 8.5.

Pancho’s picture

Core doesn't actively support 8.3 anymore, sure. However, BC remains in place.
Also, many contrib modules do support older Core versions, and as I couldn't find a notice on the project's main page, I don't want to assume one or the other.
Address maintainers need to decide if we want to provide BC for < D8.3 (thus starting a new branch) or not (then just going ahead).

AdamPS’s picture

I agree that back-compatibility is an important consideration, and I agree that if it's not BC then we ought to have a new branch and new major version. On the other hand moving to a new major version is a bit tedious for everyone so we should avoid it unless necessary.

However I think there is some confusion developing over what it means to be BC.

I would say back-compatibility means that sites using the address module continue to operate correctly without having to make changes. If you can write good update hooks to migrate field settings in all cases, then that's BC. Removing an event is arguably not BC, but if we are confident no one uses it then maybe it's OK. These are the areas where we should ask the maintainers if we are not sure.

However I agree with heddn I don't see any use in being compatible with Core

Pancho’s picture

Thanks @AdamPS, and as I said, I'm fine with either way.

Just want to point out that it's not just about the event. Once we remove our workaround and return to Core's default value functionality, address module will not work any longer on older Drupal installs < D8.3, at least not properly.
And no matter how we're deprecating the event, it simply won't get triggered anymore, so will break assumptions by the (probably just a handful of) people who are subscribing to that event.

This may be okay or not - it's just something @bojanz needs to decide, based on his experience with how address module is used, particularly in Commerce settings.
A good compromise might be doing another 8.x-1.5 release, starting a new 8.x.2.0 branch that explicitly requires D8.3 and marking the 8.x.1.x branch unsupported.

bojanz’s picture

Let's just bump the core requirement to 8.6 in a separate issue.
All modules I maintain only support the supported major core branch, nobody has complained so far.

I'll try to review the patch itself soon.

EDIT: Done in #3034697: Raise the core requirement to Drupal 8.6.x.

bojanz’s picture

Status: Needs review » Needs work

1. We should deprecate the event that is no longer fired.
That way the code that used to subscribe to it won't crash.
No need to introduce replacements at this point.

2. It would be nice to have a test that covers the default values UI, so that we have confirmation that we can only set a few values (such as the country & the administrative area), and that the form is saved, and the widget reflects that.

3. Question: Should we always preselect the site default country on the default values form?

-  /**
-   * Tests the initial values and available countries alter events.
-   */
-  public function testEvents() {

4. We should not be removing this entire test, the available countries event still exists and is in use.
Of course, the method needs a new name (testAvailableCountries()?)

+        if (!empty($address_component['settings']['default_country'])) {
+          $country_code = $address_component['settings']['default_country'];
+        }
+        else {
+          $country_code = \Drupal::config('system.date')->get('country.default');
+          $country_code = $country_code ?: 'US';
+        }

5. An empty default country should result in no default value being set.
The else here actually needs to cover the special "site_default" value, which is right now transferred verbatim.

Pancho’s picture

Assigned: Pancho » Unassigned

Too bad, I have to unassign for now, as I'm going on holidays. See you in a week.

bojanz’s picture

Status: Needs work » Needs review
FileSize
19.45 KB

Addressed my feedback. Made many tweaks to the update function. The default value form now has test coverage.

  • bojanz committed 1e90491 on 8.x-1.x authored by jsacksick
    Issue #2838457 by jsacksick, Pancho, bojanz: Re-enable the default value...
bojanz’s picture

Status: Needs review » Fixed

And committed, finally!

Pancho’s picture

Yay! This is big!

kristiaanvandeneynde’s picture

Stumbled upon this issue today, googled a bit and ended up here. Such a nice change to see this go in when you need it and be part of a new release. You made my day :)

Status: Fixed » Closed (fixed)

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