The locality.name data is missed in PlainFormater

Thanks for the issue and the patch! This is a more appropriate status, indicating that you have some code that needs review. There's nothing to be "ported" yet, which usually implies porting to a different version/branch. However, address currently still only has a single 8.x-1.x branch.

Anyway, I'll hopefully take a look and try this in the next few days. Maybe someone else will get to it before then.

Cheers,
-Derek

Patch works well. Thanks!

This patch is fixing a bug, but there is no failing test to indicate what the bug is (the break/continue part, it seems).

The template intentionally doesn't have locality.name, because it has locality.code
The same logic is used for administrative_area as well.

Tagging this that it needs a test demonstrating the bug. We should have a test-only patch that adds a new test that fails, and a full patch with the same test and the fix from #2 that passes.

Also, adding this to the list of possible issues for the 8.x-1.8 release.

Cheers,
-Derek

Thanks for the patch!

I was getting this issue on one of the listings I had:
Notice: Undefined index: locality in .../docroot/modules/contrib/address/src/Plugin/Field/FieldFormatter/AddressPlainFormatter.php on line 196

Updating the code to use !empty() instead of !== NULL fixed my issue:

$parents[] = ($index && $index > 0 && !empty($original_values[$subdivision_fields[$index - 1]]) )

#9 works well. I could use {{ locality.name }} to override the plain address format twig.

It looks like I'm not the only one who is happy with the patch. What's keeping it from being merged?

My entire feedback from #6 4 years ago is unchanged:

The template intentionally doesn't have locality.name, because it has locality.code
The same logic is used for administrative_area as well.

So right now this issue looks like a "won't fix" unless we can prove an actual problem with a failing test.

For "Italien" Addresses there is no locality.code. The locality name is just a string in 'locaity'. And what does code in this context mean? I understand "code" for the country is the ISO shortcut. But why would the city name e.g. "Berlin" for a German address be in `locality.code` and not `locality.name` ?

See:

Added only tests per @dww comment that check plain address formatter ,so this should fail.
There are 2 tests: one to check if all address data present in plain formatter output and the other checks if all data is shown when some property is optional and empty (apart from country_code which determines empty field).

Now I apply same tests from #17 with fixes from #9 into one patch.
This should now pass.

For "Italien" Addresses there is no locality.code. The locality name is just a string in 'locaity'. And what does code in this context mean? I understand "code" for the country is the ISO shortcut. But why would the city name e.g. "Berlin" for a German address be in `locality.code` and not `locality.name` ?

The template file that you're modifying tells you this:

 * if a subdivision (dependent_locality, locality, administrative_area) was
 * entered, the array will always have a code. If it's a predefined subdivision,
 * it will also have a name. The code is always preferred.

This is because if a subdivision is predefined, it has two names, "name" which is supposed to be used in the UI when selecting a value, and "code" which is supposed to be shown when rendering an address. Sometimes this "code" matches the "name", sometimes it matches an ISO code, depending on the rules for that country. The use of "code" to mean user-facing label is unfortunate, but we inherited that decision from Google 10 years ago and we can't change it.

We can extend the comment in the template file to explain what I just explained here.

@sickness29
Thank you for illustrating a case in which the current code fails, something I've been asking for since 2019.

However, we already have a AddressPlainFormatterTest in Drupal\Tests\address\Kernel\Formatter, can we add a new test case there instead of introducing an independent test?

Furthermore, the twig file changes don't seem correct, the twig file now looks like this:

  {% if locality.name %}
    {{ locality.name }} <br>
  {% endif %}
  {% if dependent_locality.code %}
    {{ dependent_locality.code }} <br>
  {% endif %}
  {% if locality.code or postal_code or administrative_area.code %}
    {{ locality.code }} {{ postal_code }} {{ administrative_area.code }} <br>
  {% endif %}

So we output the locality, then the dependent_locality, then the locality again?

I still think this patch should not make any changes to the twig file itself, nor should it switch "code" for "name" in the formatter plugin.

Hi @bojanz
I have added changes to Kernel\Formatter\AddressPlainFormatterTest to illustrate the issue in test failure.
When only country and admin area were selected, there is no admin area in output, however it should be as module allows optional values.
I will add this same test with fix as next comment

The issue with output should now be fixed

Fix looks great now. Needs a reroll though, #3144823: The country is rendered in the wrong language when the entity is translatable changed the tests.

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

Rerolled.

And committed.