Switch to commerceguys/addressing v2.0.0

Initial boilerplate.

The GreatBritainSubscriber is no longer wrong, but we might want to rewrite it to contain the full ISO list, keyed by ISO code, since that would be a better example.

This is a full composer report while installing commerceguys/addressing v2.0.0-beta1:

- Installing doctrine/deprecations (v1.0.0)
- Installing doctrine/collections (2.1.2)
- Installing commerceguys/addressing (v2.0.0-beta1)

The ludwig.json file should be updated like this:

{
     "require": {
         "doctrine/collections": {
-            "version": "1.6.8",
-            "url": "https://github.com/doctrine/collections/archive/1.6.8.zip"
+            "version": "2.1.2",
+            "url": "https://github.com/doctrine/collections/archive/2.1.2.zip"
+        },
+        "doctrine/deprecations": {
+            "version": "v1.0.0",
+            "url": "https://github.com/doctrine/deprecations/archive/v1.0.0.zip"
         },
         "commerceguys/addressing": {
-            "version": "v1.4.2",
-            "url": "https://github.com/commerceguys/addressing/archive/v1.4.2.zip"
+            "version": "v2.0.0-beta1",
+            "url": "https://github.com/commerceguys/addressing/archive/v2.0.0-beta1.zip"
         }
     }
 }

Same patch with the signature change to the getLocale() method, let's seee if there are less test failures (haven't ran the tests locally :)).

We always return a string so we can change the return type to just : string.

Added @devad's ludwig.json changes from #3.

Finished the address_line3 logic (the final few places require the field column to actually be created).

Oh, whoops, I see... we're now requiring PHP 8, so the PHP 7* tests are doomed.

Yeah, definitely would want a 2.0.x branch for this. Wonder if there's anything else that would be good to land in 8.x-1.x before we create that and have to start maintaining 2 branches...

p.s. Sort of out of scope here, but it's always bothered me that this module depends so heavily on an external library that:

1. Some (many?) of the maintainers here have no access to.
2. Is branded "commerceguys" (sorry y'all, but that name has always made me cringe with the gendered language).

Any interest / possibility of moving this out of the "commerceguys" namespace into a new location that's more welcoming and that the rest of us could be added to help co-maintain?

Thanks!
-Derek

@dww
I've granted you access to the library. Every address maintainer should have access to the underlying library.

As for the name, that ship has sailed. While GitHub allows us to rename an organization/repository, Packagist doesn't. At this point, 9 years in, the library is too popular and widely used for us to break every single composer.json file.

Re: #8

Wonder if there's anything else that would be good to land in 8.x-1.x

This Ludwig update patch #3 would be nice to commit to 8.x-1.x before 1.x new release: #306611: Fatal error when trying to invoke non-existing action callbacks →

For anyone who needs it, here are the composer.json changes I used to add this to my project:

"repositories": [
...
        {
            "type": "vcs",
            "url": "https://github.com/commerceguys/addressing"
        }
],
"require": {
...
        "commerceguys/addressing": "2.0.0-beta1 as 1.9.0",
        "drupal/address": "dev-1.x",
...
}
"extra": {
...
        "patches": {
...
            "drupal/address": {
                "#3354976-7: Addressing v2": "https://www.drupal.org/files/issues/2023-04-20/3354976-7.patch"
            },
...
        }
...
}

Then running composer update commerceguys/addressing drupal/address downloaded commerceguys/addressing v2.0.0-beta1 and applied the patch.

Using the address field and changing the country from one to another results in the following error in the AJAX request:

Fatal error: Type of Drupal\address\Plugin\Validation\Constraint\AddressFormatConstraint::$blankMessage must be string (as in class CommerceGuys\Addressing\Validator\Constraints\AddressFormatConstraint) in /var/www/html/web/modules/contrib/address/src/Plugin/Validation/Constraint/AddressFormatConstraint.php on line 16

I suspect this is the AJAX error the tests are hitting.

For anyone who's interested, because of API changes in commerceguys/addressing v2, module 8.x-1.x is no longer compatible with the library, so you can't currently shoehorn the two together to make them work, it gives this error when you clear the caches:

PHP Fatal error:  Declaration of Drupal\address\Plugin\Field\FieldType\AddressItem::getLocale() must be compatible with CommerceGuys\Addressing\AddressInterface::getLocale(): ?string in modules/contrib/address/src/Plugin/Field/FieldType/AddressItem.php on line 351

@DamienMcKenna
The whole point of this issue is to fix those incompatibilities, the issue summary mentions the typehints.

There's no use in mentioning that an unfinished patch doesn't work.

The country selector AJAX request fails with this error:

Error: Typed property CommerceGuys\Addressing\AddressFormat\AddressFormat::$administrativeAreaType must not be accessed before initialization in CommerceGuys\Addressing\AddressFormat\AddressFormat->getAdministrativeAreaType() (line 236 of vendor/commerceguys/addressing/src/AddressFormat/AddressFormat.php).

This is because in AddressFormat::__construct() $this->administrativeAreaType is only defined if the address definition has "administrative_area_type" defined.

I suspect some of the bugs are in the library, e.g. this change:

diff --git a/src/AddressFormat/AddressFormat.php b/src/AddressFormat/AddressFormat.php
index f15c39a..6594a9e 100644
--- a/src/AddressFormat/AddressFormat.php
+++ b/src/AddressFormat/AddressFormat.php
@@ -29,7 +29,7 @@ class AddressFormat
      */
     protected array $uppercaseFields = [];
 
-    protected ?string $administrativeAreaType;
+    protected ?string $administrativeAreaType = null;
 
     protected string $localityType;
 

.. results in the following error:

( ! ) Fatal error: Type of Drupal\address\Plugin\Validation\Constraint\AddressFormatConstraint::$blankMessage must be string (as in class CommerceGuys\Addressing\Validator\Constraints\AddressFormatConstraint) in modules/address/src/Plugin/Validation/Constraint/AddressFormatConstraint.php on line 16

Indeed, there was a library bug in the AddressFormat class.

Tagged https://github.com/commerceguys/addressing/releases/tag/v2.0.0-beta2

Here's an updated patch, and it covers #18.

Still incomplete, of course (missing postLoad(), other typehint fixes).

Now with the missing file.

Fixing the addViolation() method return type allows the AJAX request to load the per-country fields to work.

FWIW I manually tested the following:
* Added an Address field to a content type.
* Customized the field widget settings.
* Created a node with that field - the address field's country selector showed correctly.
* Selected a country - the fields showed correctly.
* Selected a different country (Ireland) - the new fields showed correctly.
* Entered invalid data into the address fields and clicked "save" - the data was correctly validated and an error message was shown.
* Fixed the invalid data and clicked "Save" - the node saved correctly.
* Examined the data in Devel, the data was stored correctly.

Great work!

Working on the test coverage.

This fixes tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php locally.

This fixes tests/src/Kernel/Formatter/AddressDefaultFormatterTest.php locally.

The last problem stems from the fact that the Views administrativearea filter requires a default value, even though it's optional; I loaded the view into Drupal, edited the filter, but when I try to save it I get an error that says "The predefined country must be set for this filter to work" even though the field is not required.

Inconsequential tweaks that just tidy the code a little.

(from the last patch)

What's weird is that with 8.x-1.x/v1.4.2 the address-test/views/filter-administrative-area page has the administrative area field:

Looking at the code I see the following:
* The address_test_filter_administrative_area view sets the "country_static_code" item of the "field_address_test_administrative_area" exposed filter to "AR".
* In AddressTestEventSubscriber::getAvailableCountries() it includes AU, BR. CA, GB and US, but not "AR".

Does this mean that the country_static_code value is wrong and it should actually be one of the other values? Or is there a bug in the 8.x-1.x/v1.4.x codebases that allows this field to store an invalid value?

Changing country_static_code to "BR" seems to allow this to work.

(I was incorrect in my assessment that the tests worked for me locally, I had ran the wrong test)

The bug editing the included address_test_filter_administrative_area view with the patch also happens with the 8.x-1.x codebase. This can be reproduced as follows:
* Manually enable the address_test module.
* Edit the view.
* Open the "Content: Address:administrative_area (exposed: fixed country: AR)" filter.
* Try clicking the "Apply" button.
* The form will reload with the following error message: The predefined country must be set for this filter to work.

That said, I tracked the test failure down to the fact that in v1.4.2 the country Costa Rica did not have subdivisions but in v2 it does.

So in the end all that was needed to fix the Views tests were some updates to match changes in the v2 library.

And the tests are green!

Thank you, Damien!

We are now down to one todo:

7) Add postLoad() logic to the field type to use the SubdivisionUpdater (in order to update previous data).

I'll circle back to this issue by the end of the week.

In our case we've been writing custom update scripts, as I found the site had legacy address data from before it was migrated to Drupal 7 about eight years ago.

Selecting some of the countries leads to further bugs, so I need to expand the test coverage and see if I can fix the bug.

Selecting some countries results in the following error:

Error: Typed property CommerceGuys\Addressing\AddressFormat\AddressFormat::$administrativeAreaType must not be accessed before initialization in CommerceGuys\Addressing\AddressFormat\AddressFormat->getAdministrativeAreaType() (line 236 of /code/vendor/commerceguys/addressing/src/AddressFormat/AddressFormat.php).

Selecting others results in the following:

Error: Typed property CommerceGuys\Addressing\AddressFormat\AddressFormat::$localityType must not be accessed before initialization in CommerceGuys\Addressing\AddressFormat\AddressFormat->getLocalityType() (line 249 of /code/vendor/commerceguys/addressing/src/AddressFormat/AddressFormat.php).

Furthermore, selecting some countries doesn't update the country fields, but doesn't log an error. For example, select United States, wait for the fields to update, then select Singapore, it still shows the fields for United States but doesn't show an error.

Need to work on something else, will look into this later.

FWIW I was starting by expanding the test coverage to include a few additional countries:

diff --git a/tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php b/tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php
index 5945b5d..32c987c 100644
--- a/tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php
+++ b/tests/src/FunctionalJavascript/AddressDefaultWidgetTest.php
@@ -367,9 +367,10 @@ class AddressDefaultWidgetTest extends WebDriverTestBase {
 
     // US has all fields except sorting code and dependent locality.
     // France has sorting code, and China has dependent locality, so these
-    // countries cover all fields.
+    // countries cover all fields. Added some additional fields for completeness
+    // sake.
     $this->drupalGet($this->nodeAddUrl);
-    foreach (['US', 'FR', 'CN'] as $country) {
+    foreach (['US', 'FR', 'CN', 'HK', 'MX', 'IE', 'SG'] as $country) {
       /** @var \CommerceGuys\Addressing\AddressFormat\AddressFormat $address_format */
       $address_format = $this->addressFormatRepository->get($country);
       $used_fields = $address_format->getUsedFields();

@DamienMcKenna
Is there a chance that you somehow got an older version of the library, and not v2.0.0-beta2?

Because beta2 clearly initializes these properties to null, so the error from #38 should not be possible:

    protected ?string $administrativeAreaType = null;

    protected ?string $localityType = null;

    protected ?string $dependentLocalityType = null;

I tried to write a unit test retrieving an address format for a country without an administrative area and it passed successfully.

Apologies, yes, I missed that the composer file was loading beta1 and hadn't updated to beta2. With beta2 it's working great.

So now just back to needing the postLoad() logic.

For anyone who needs it, here are the updated composer.json lines:

"repositories": [
...
        {
            "type": "vcs",
            "url": "https://github.com/commerceguys/addressing"
        }
],
"require": {
...
        "commerceguys/addressing": "2.0.0-beta2 as 1.9.2",
        "drupal/address": "dev-1.x",
...
}
"extra": {
...
        "patches": {
...
            "drupal/address": {
                "#3354976-33: Addressing v2": "https://www.drupal.org/files/issues/2023-05-09/address-n3354976-33.patch"
            },
...
        }
...
}

Opened the 2.0.x branch. Here's a reroll of the patch.

Added the BC logic.

Let's do one last test run before committing.

Let's party.

@DamienMcKenna
Big thank you for your help driving this home.

Fantastic news! And you are very welcome, I'm glad I was able to organize time to work on it.

Great. Thanks for all the work and commit.

I have noticed one detail in Ludwig integration. As suggested in comment #3 doctrine/collections library update to v2.1.2 would be nice if possible:

-            "version": "1.8.0",
-            "url": "https://github.com/doctrine/collections/archive/1.8.0.zip"
+            "version": "2.1.2",
+            "url": "https://github.com/doctrine/collections/archive/2.1.2.zip"

Do we need to open a follow-up ticket for this?

@devad
2.1.2 requires PHP 8.1, our minimum is 8.0.

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