🇷🇸Serbia @bojanz

I think it's nice for Address to have a US-specific service that takes a postal code and returns an administrative area. That service can then hardcode the data it needs (the postal code prefixes that are matched against).

Would it make sense to call it a locator? \Drupal\address\Locator\UnitedStates? I'll let you and Jonathan bikeshed that.

@mishradeepak1991 I have no clue what you're doing. All code posted to this issue was already committed.

There you go then. You'll have to fix that patch (just add the use statement you're missing).

And no, that feature is not in 2.0.x yet.

AddressDefaultFormatter does not have a settingsForm() method in the 2.x branch:
https://git.drupalcode.org/project/address/-/blob/2.x/src/Plugin/Field/F...

It sounds like you are applying a patch which is incomplete (didn't add a use statement for the FormStateInterface).

I honestly don't see why the processor should every quit when the queue is empty. I know we originally implemented it that way, but I have no recollection why. Most other queue processors will sleep for a configurable amount of seconds, then try to claim a job again.

bojanz → created an issue.

8.x-1.x will not be made compatible with Drupal 11.

Quoting myself from the Address issue [#3439726#comment-15544880]:

I am fine with not having or using this setting at all. We only introduced it in the 2.0 release ~4 months ago, to have a fallback when the field has no default value configured, and it was community requested.

Similarly, Commerce uses the site country as a last fallback simply because it's there, the main source of the default country is the Store entity.

+1 to what dww said.

I am fine with not having or using this setting at all. We only introduced it in the 2.0 release ~4 months ago, to have a fallback when the field has no default value configured, and it was community requested.

1. Shipping rate: base flat rate plus extra per item. It's like minimum charge is $10 and add $2 per item in the order;

Commerce Shipping supports per-item rates, but it doesn't support setting a base rate yet. You need either ✨ Base rate for per item flat rate shipping Needs review or ✨ Add Shipping method with combination of Flat rate + Flat rate per item Needs review .

4. customizable workflow: configure more status or different status as needed

https://docs.drupalcommerce.org/commerce2/developer-guide/orders/workflows

This was a decision made by Google, who defined our initial dataset for Chrome/Android use.

The organization comes first for 31 countries:
- AT, AU, AX, BE, BL, BR, BY, CC, CH, CX, DE, FI, FR, GF, GP, HM, IQ, IR, LI, LT, LU, MF, MQ, NC, NF, NL, PM, RE, SE, WF, YT

So this includes Austria/Germany/Switzerland, France/Luxembourg/Belgium, Australia, and others.

I can do some research to see whether this default still makes sense. In the meantime, you can implement an event subscriber for the AddressFormatEvent, and alter any format as you wish.

@VladimirAus
Can you give us the output of this:

    var_dump($language_manager->getConfigOverrideLanguage());
   var_dump($language_manager->getConfigOverrideLanguage()->getId());

From what I can see, core issue #2684873: ConfigurableLanguageManager::getConfigOverrideLanguage() returns NULL → was fixed ages ago (Drupal 8.3), so our logic should be as simple as:

    $this->defaultLocale = $language_manager->getConfigOverrideLanguage()->getId();

According to the interfaces, getConfigOverrideLanguage() always returns a language, and getId() always returns a string, so a crash should be impossible.

Apologies. I haven't used this module in almost a decade, so I do not monitor its issue queue. Additionally, I do not have any email from cafuego in my inbox, nor did I get pinged on Drupal Slack.

I did find the email from gisle, but GMail decided to send it to spam, which is the first time I've seen that happen to drupal.org emails.

I've now given cafuego full access to the project.

https://www.drupal.org/project/simple_oauth → is what others seem to be using.

This is the crashing code:

  public function __construct(CacheBackendInterface $cache, LanguageManagerInterface $language_manager) {
    parent::__construct();

    $this->cache = $cache;
    // The getCurrentLanguage() fallback is a workaround for core bug #2684873.
    $language = $language_manager->getConfigOverrideLanguage() ?: $language_manager->getCurrentLanguage();
    $this->defaultLocale = $language->getId();
  }

It should not be possible for $language->getId() to be null. It indicates something very wrong with your site (or a core bug).

I suggest adding some debug code here to see which language is used (config override or current language) and what the value of $language is.

Is there enough value in actually support the "merge" strategy? Or the concept of strategies at all?

In all other implementations I've used, a job/task is either unique/deduplicated (meaning that a job with the same fingerprint is rejected on enqueue) or it isn't.

My assumption without reading the code would be that I do:

// If I don't provide a fingerprint, it is autogenerated for me.
$job = Job::createUnique("my_type", $payload, ['fingerprint' => 'my-unique-value']);

enqueueJobs() can check if $job->getFingerprint() is non-empty, and if the backend does not implement SupportsUniqueJobsInterface, throw an exception.

3. I REALLY want to rename the DB column we're adding from unique_id to something like job_hash. It's going to be a source of confusion and bugs with the current name. I appreciate that sites are already using this patch.... but that's a risk you take when using a patch.

FWIW I've been using "fingerprint" as the column name for this purpose in a few other places. It is fairly common in various libraries.

I do agree that "job_hash" sounds better than "unique_id" at least. Every ID is unique so the column name sounds confusing.

This issue is now unblocked (we switched the module to addressing v2.2.0)

Thanks, everyone!

Drupal.org is insisting that "Bluespark" is the customer I'm doing this for, no clue why.

Anyway, committed.

bojanz → created an issue.

You need to subscribe to the AddressFormat event.

You can then modify $definition['format'] to always have %additionalName.

See https://git.drupalcode.org/project/address/-/blame/2.x/tests/modules/add... for an example.

@lazzyvn
We need either a test or a small form class where the problem can be observed.

Changing status to "needs review" to signal that there is a patch here.

Changing version to 2.x-dev, this will have to go into the 2.1.0 release, 8.x-1.x is now closed.

Went ahead and tagged 2.0.1: https://www.drupal.org/project/address/releases/2.0.1 →

I am fine with proceeding with 2.0.1 without 🐛 Mismatched field definitions after address upgrade Needs review . It only affects a portion of the users, and I personally have no time to work on it, so I don't want to hold off other fixes because of it.

Layout Builder integration was done to solve the site builder use case.

So those are the two theming options. Twig or Layout Builder. Full control just via the "manage fields" screen will never be possible.

Can we please try to debug the original update function, and fix the problem there, before we introduce update functions with workarounds?

Our main problem here is that none of the maintainers have sites that are affected, so we need a db dump where the problem can be observed, or we need a developer with such a site to do the debugging themselves.

We're already discussing this in 🐛 Mismatched field definitions after address upgrade Needs review so let's continue there. I am glad you are able to reproduce the problem, that means you should be able to debug the existing update function and see why it's failing (is the field not getting found? what is the error behind the WSOD? etc)

Let's go borderless.

There is no patch here, changing status.

No, it's an Address bug for sure.

Unfortunately, I have no time to chase it, someone from the Commerce (Reporting) side will have to take a look.

This module needs a full rewrite and hasn't had a real sponsor since 2016. It is completely unsustainable. But that's the reality of most of Drupal contrib.

There was a Drupal core issue about implementing IEF in core: #2820235: Add inline entity form to core as experimental → but it didn't receive a lot of attention so far.

I misunderstood the request when I first closed this issue.

Unfortunately, Address has no designated property for US counties (Google calls it `administrative_area_level2`), it's not a use case we support. Adding a new property from outside the module would be impossible, you'd have to essentially fork the entire module codebase. A workaround is needed: repurposing an existing property.

We have two possible ways to achieve this:

1) Repurpose address_line3 to hold the county.
Implement a hook_field_widget_form_alter() + #after_build for the address element, which looks at the currently selected country, and if it's US, converts address_line3 to a dropdown, gives it a new label, and populates the options based on the administrative area.

2) Repurpose the locality field to hold the county, and move the City to the dependent_locality field.
This allows you to use SubdivisionEvent to define the counties per administrative area, and it will just work. An alter is only needed to adjust the labels. However, you need to teach all other code (payment gateways, shipping methods) to pass $address->getDependentLocality() instead of $address->getLocality() for the city.
You also need an AddressFormatEvent to add the %dependentLocality to the format.

Updating version. We will commit this for Address 2.1.0.

I've done what I promised in #24, the library has been fixed, now it just needs a new release.

Thanks!

This is now 2.1.0 material, so bumping the version.

Made a 2.x branch and a 2.x-dev release with the same warnings as 11.x:
https://www.drupal.org/project/address/releases/2.x-dev →

Responded in the linked thread.

Honestly I preferred the original logo by a mile. Didn't even cross my mind to associate the red & blue with any flag colors.

Purple&green feels garish.

The rainbow is now universally associated with LGBT movements, so I feel like the rainbow makes the logo look seasonal (one color scheme for pride week, one for halloween, etc).

This leaves orange and green. So I vote for that one.

Good to know, cheers.

Composer can't find the commerceguys/addressing library anymore, so it sounds like you did something odd to your vendor folder.

Use Composer to reinstall the entire drupal/address module (composer remove + conposer require).

I am honestly fine with just modifying the constructor. It's our own views plugin, if you decide to override that one then you can keep it up to date.

But I know maintainers have gotten more cautious over the past few years.

Looks good at a glance!

so that if there are still issues with patches, those continue to be tested until the DA turns off DrupalCI entirely.

I think it's wise to do that as long as we can (also, I'm old fashioned and still like patches).

Thanks, everyone!

bojanz → created an issue.

Shortening the title somewhat, and updating the status.

The message is telling you that the crash is coming from quick_node_clone.
Looking at the issue queue for quick_node_clone, I see this issue: 🐛 Incompatibility with Address module version 2.x RTBC with a patch you can use.

We could also open 2.x branch now and use that as ‘main’, like core is doing with 11.x, so we don’t have as much version and MR churn once we do decide to branch 2.1.x and move to a new stable release.

I am fine with that if that's what you prefer. Leaving the decision to you.

This is a low-traffic module (~100 open issues, ~20 of which have patches or MRs), so whatever we choose will be manageable.

Yes:

Where number is the category ID defined in core.field_type_categories.yml:

So setting "address" results in a broken reference if the same type is not defined in the yaml file.

Thanks for working on this!

Open a 2.1.x branch.
All issues + MRs should target that from here on out.
We backport bug fixes and non-disruptive tasks (like test improvements) to 2.0.x when appropriate (ideally via cherry-pick).
We tag and ship 2.0.x releases as needed.

Makes sense to me.
I'm also fine with not backporting fixes to 2.0.x once there's a 2.1.0 release, we want people to stay on the latest minor version.

Discuss if we want to aim for a ~yearly minor release schedule, or otherwise start to think about when we want to do a 2.1.0 release (with alpha, beta, rc, etc).

I wouldn't try to create a fixed schedule for minor releases, since neither of us is paid to work on Address, we can't guarantee when we'll be active. We might be able to do two minor releases one year, then nothing for another year. It's fine to say "once there's enough in a branch to release, we release".

dww → credited bojanz → .

Sounds good to me.

Thanks for the patches, but we're not looking for co-maintainers. The module is already D10 compatible, small in scope, and with very little issue queue activity.

Crediting AstonVictor for the duplicate at 📌 Add validation for delta Needs review .

Duplicate of 🐛 Module is missing a config schema for validation Needs work .

Sorry, I merged 📌 Fix snake_case/camelCase Needs review without seeing that this issue is in conflict, we'll need to rebase it.

Sure, why not. Thanks.

"needs review" is for issues with patches.

The error doesn't sound like something we can help you with. If it's reproducible only with address_suggestion, then it might be caused by address_suggestion.

Thanks for the feedback!

Needs a reroll for 2.0.x.

We also need to add address_line3 the same way we added address_line2.

Good idea!

I would prefer to let the sites be responsible for the validity of their data, because silently returning lowercase country codes could break other modules, REST clients, etc.

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

What's super funny is that I spent serious time (both personal and Centarro-sponsored) back in 2017 to develop Ludwig, a UI -driven way of downloading and using libraries, then documented its use for both Address and Commerce, and then its adoption was symbolic at best (a few thousand installs in the first 3 years, now at 10k installs).

Every moaning comment here is younger than Ludwig, and yet nobody has bothered to read the project page. So, expecting them to learn a truly better tool (which Composer is) would definitely be too much.

Note that Address 2.0.0 has fixed ✨ Subdivision event subscriber doesn't receive a list of subdivisions already set for a country Fixed , which means that the best way to modify subdivisions (and their translations) is using the SubdivisionEvent, the same one documented here.

Of course #3 works, though I generally prefer #after_build for all address element modifications, as documented here or shown here 💬 There is not possible to set a description for form elements of address (fields of address) Fixed .

Retitling for clarity. And changing version because new features only go into 2.0.x.

We are talking about the field override UI here. Disabling fields via form_alter already works via #after_build:

  public function buildForm(array $form, FormStateInterface $form_state) {
    $form['address'] = [
      '#type' => 'address',
      '#default_value' => [
        'country_code' => 'US',
        'administrative_area' => 'CA',
        'locality' => 'Mountain View',
        'postal_code' => '94043',
        'address_line1' => '1098 Alta Ave',
        'organization' => 'Google Inc.',
        'given_name' => 'John',
        'family_name' => 'Smith',
      ],
      '#after_build' => [[get_class($this), 'disableFields']],
    ];
    $form['submit'] = [
      '#type' => 'submit',
      '#value' => $this->t('Submit'),
    ];

    return $form;
  }

  /**
   * #after_build callback: Disables certain fields.
   */
  public static function disableFields(array $element, FormStateInterface $form_state) {
    $element['address_line2']['#attributes']['disabled'] = 'disabled';

    return $element;
  }

We should add this example to the documentation.

The documentation page is still valid, #after_build still works.

I've also provided the same example (which I just retested) in this comment: https://www.drupal.org/project/address/issues/3167404#comment-15375862 💬 There is not possible to set a description for form elements of address (fields of address) Fixed

And ultimately the reason why I said no to #field_descriptions is also the reason why I'm saying no to #placeholders. We do not need a custom mechanism on the form element level.

On the other hand, I do agree that it is very tempting to allow setting field descriptions and placeholders via the UI, as core allows for individual fields. However, the interest for that seems very low, so I would like to get more buy-in from the community before we start implementing and supporting either or both.

I am going to tentatively close this, in order to give sickness29 issue credit. I will also ping @dww to get his thoughts on the matter. Then, if and once enough interest materializes, we can restart this effort.

I've given this issue some thought, and decided not to proceed with it.

The reason why is because we are introducing an Address-specific mechanism to replace a more generic Form API mechanism, namely using #after_build to modify subelements:

public function buildForm(array $form, FormStateInterface $form_state) {
    $form['address'] = [
      '#type' => 'address',
      '#default_value' => [
        'country_code' => 'US',
        'administrative_area' => 'CA',
        'locality' => 'Mountain View',
        'postal_code' => '94043',
        'address_line1' => '1098 Alta Ave',
        'organization' => 'Google Inc.',
        'given_name' => 'John',
        'family_name' => 'Smith',
      ],
      '#after_build' => [[get_class($this), 'modifyDescriptions']],
    ];
    $form['submit'] = [
      '#type' => 'submit',
      '#value' => $this->t('Submit'),
    ];

    return $form;
  }

  /**
   * #after_build callback: Modify the address descriptions.
   */
  public static function modifyDescriptions(array $element, FormStateInterface $form_state) {
    $element['address_line1']['#description'] = 'This is the first address line';
    $element['address_line2']['#description'] = 'This is the second address line';

    return $element;
  }

This doesn't feel too verbose, especially taking into account the fact that this is not a majority use case (as evidenced by the few subscribers on this issue). It also allows developers to modify all other properties, such as the #title, #placeholder, #maxlength, #size, etc. I am worried that if we make an exception for field_descriptions, we'll just end up having the same conversation for titles and other settings.

bojanz → created an issue.

All feature requests are 2.0.x-dev only at this point, we will only commit essential bug fixes to 8.x-1.x.

@sickness29
I am happy to review any and every patch that you provide, but I would like to remain the sole maintainer (along dww) for now. This module has a big install base (100k) and a very wide set of use cases (Commerce, non-Commerce), as well as complex interactions with the underlying library. This makes it very easy to introduce regressions for a large number of sites, so I would rather ensure that each change gets my approval. Since we have less than two pages of issues, this feels doable. With important modules slow development is a feature, not a bug.

+        $subdivisions = $this->subdivisionRepository->getList([$country_code], $this->langcode);
+        foreach (array_keys($subdivisions) as $administrative_area_code) {
+          // Administrative_area codes format check.
+          if (is_string($administrative_area_code) && ctype_upper($administrative_area_code)) {
+            $when[] = "WHEN $country_field = '$country_code' AND $field_name = '$administrative_area_code' THEN " . $i++;
+          }
+        }

So if an administrative area code is numeric (e.g. "82"), we will just skip it, and the handler won't work for that country?
Can't we just not pass it to ctype_upper() in that case, and add it as-is to the query?

And committed.

Rerolled.

Renamed the setting to be "wrapper_type" so that it matches its label and committed. Right in time for 2.0.0!

Thanks, everyone.

Committed #21 to 8.x-1.x, changing version to focus on the 2.0.x reroll.

Fix looks great now. Needs a reroll though, 📌 Usage of content language instead of the language of the interface. Needs work changed the tests.

Patch looks good, now we need to implement the library side.

Leaving that on my plate for 2024.

Thank you!

Committed. Thanks to everyone at the European Commission for pushing this fix forward!

Manual testing showed $langcode to be preferable to $address->getLocale() for non-translatable entities. Reverting to the original approach, and adding a comment explaining why.

$address->getLocale() and $langcode are the same for translatable entities.
For non-translatable entities $address->getLocale() exists to record the langcode at the time of address creation, so that we can rerender the address the same way (e.g. if it was created in Chinese, when we render, we continue to maintain the major-to-minor field order and the subdivisions in Chinese script). However, if we did the same for countries, then the site admin would be unable to recognize to which country the address belongs to. So we'll maintain old behavior and continue to show the country in the current interface language in that case.

My patch is newer than the MR and contains the renames I proposed in #32. i can commit it directly if we agree on the points, we don't need to keep updating the MR.

Looks great, thank you!

bojanz → made their first commit to this issue’s fork.

The problem here can be narrowed down. The country is rendered in an unexpected language.

For country and zone fields, the fix is simple, we follow core and use $langcode.
Address fields are a bit more complex, they have their own getLocale() getter, which returns the locale stored by initializeLocale:

  /**
   * Initializes and returns the langcode property for the current field.
   *
   * Some countries use separate address formats for the local language VS
   * other languages. For example, China uses major-to-minor ordering
   * when the address is entered in Chinese, and minor-to-major when the
   * address is entered in other languages.
   * This means that the address must remember which language it was
   * entered in, to ensure consistent formatting later on.
   *
   * - For translatable entities this information comes from the field langcode.
   * - Non-translatable entities have no way to provide this information, since
   *   the field langcode never changes. In this case the field must store
   *   the interface language at the time of address creation.
   * - It is also possible to override the used language via field settings,
   *   in case the language is always known (e.g. a field storing the "english
   *   address" on a chinese article).
   *
   * The langcode property is interpreted by getLocale(), and in case it's NULL,
   * the field langcode is returned instead (indicating a non-multilingual site
   * or a translatable parent entity).
   *
   * @return string|null
   *   The langcode, or NULL if the field langcode should be used instead.
   */
  public function initializeLangcode() {

We are already using getLocale() elsewhere in the formatters, we just forgot to use it when fetching the country list.

Two other problems I noticed:
1) The Italian test is not needed, we can use the existing Taiwan test instead, which has an additional benefit of also ensuring the field order and subdivision names are selected via the right langcode.
2) We need to update the used cache contexts.

So, let's try this.

Let's try to fix the tests.

Here's my attempt to illustrate #32. Renamed the setting, its label, and its options.

I am still unsure about the "Wrapper type" label.

I did try to rename the options so that both people intimately familiar with Drupal and causal users could figure out what each option does. I haven't used Drupal at my day job for a few years now, and I had to remind myself that "details" is "fieldset, but collapsible", so I was my ideal test user.

Tweaked the variable name, and committed, thanks!

This doesn't hurt, more precise defaults will still apply (field default, anything given via #default_value, etc). That includes any defaults set by Commerce (store default country).

Of course, at this point a change like this can only be made to 2.0.x

Committed, thanks everyone!

The default address format for Kazakhstan was fixed in 2.0.0-beta3 ( 📌 Fix the address format for Kazakhstan, add administrative areas Fixed ).

One last tweak (don't process definitions twice).