Missing theme suggestions for address_plain

No theme suggestions are provided. We have a bug.

Ideally we would have field, entity bundle, view mode suggestions.

Patch to add suggestions for 'address_plain'.

Fix previous patch.

Suggestions needs to start with theme hook name.

Tested out patch #3 but it did not work at all. Patch #4 did the trick, thanks!

Marking RTBC

How about making it so we can use the view mode as well as the entity type and bundle? Modified patch attached. Thanks.

@new-zeal please provide an interdiff.

On D8.5.x-dev with commerce-8.x-2.x-dev #4 и #7 doesn't work.
i've tried address_plain__address.html.twig.

Thanks for the work @new-zeal. I applied the patch manually and I am able to leverage theme suggestions functionality with address field now.
Rerolled the existing patch so that it is compatible with the latest version of D8.5 and passes testing.
Moving it for further review.

1. It's odd that we're hardcoding address_plain__. We should be using $variables['theme_hook_original'].

Here's a commerce example, for an entity template:

  $original = $variables['theme_hook_original'];
  $entity = $variables['elements']['#' . $entity_type_id];
  $sanitized_view_mode = strtr($variables['elements']['#view_mode'], '.', '_');

  $suggestions = [];
  $suggestions[] = $original . '__' . $sanitized_view_mode;
  $suggestions[] = $original . '__' . $entity->bundle();
  $suggestions[] = $original . '__' . $entity->bundle() . '__' . $sanitized_view_mode;
  $suggestions[] = $original . '__' . $entity->id();
  $suggestions[] = $original . '__' . $entity->id() . '__' . $sanitized_view_mode;

  return $suggestions;

2. It makes no sense to provide a suggestion for the field type, we always have the same field type ("address").

In general we need to decide which suggestions we want to provide. I'll discuss that with the team tomorrow.

Here's another patch to keep this going.

Removed the /docroot prefix in the paths.

Thanks @hstandaert for your patch and welcome to d.o!

Thanks @mpp! I added the patch again since something seems to have gone wrong the first time.

Sorry, this no longer applies.

I notice #17 is using paths like so: a/modules/contrib/address/address.module (e.g. from inside a fully built site). Patches need to be relative to the project's own git repo, not a site. E.g. a/address.module

Like so. Didn't look at the patch yet, but hopefully the bot will like this.

Looked more closely at the patch. I found some concerns:

1. +++ b/address.module
   @@ -24,12 +24,32 @@ function address_theme() {
   +  $item = $address->getParent();
   +  $entity = $item->getParent()->getValue();
   

   These are unneeded.
   $field (from getFieldDefinition() can directly tell you the entity type ID and bundle.

2. +++ b/address.module
   @@ -24,12 +24,32 @@ function address_theme() {
   +  $suggestions[] = $original . '__' . $entity->getEntityTypeID() . '__' . $entity->bundle() . '__' . $variables['element']['#view_mode'];
   

   Per the sample code from @bojanz above, this #view_mode wants to be "sanitized" before use in a theme suggestion.

3. +++ b/address.module
   @@ -24,12 +24,32 @@ function address_theme() {
   +  return $suggestions;
   

   Generally, I'm not totally sure these specific suggestions make sense like this. I'd like to see more thought put into these.

4. +++ b/src/Plugin/Field/FieldFormatter/AddressPlainFormatter.php
   @@ -51,6 +51,13 @@ class AddressPlainFormatter extends FormatterBase implements ContainerFactoryPlu
      /**
   +   * The view mode.
   +   *
   +   * @var \CommerceGuys\Addressing\Subdivision\SubdivisionRepositoryInterface
   +   */
   +  protected $viewMode;
   +
   +  /**
   
   @@ -80,6 +87,7 @@ class AddressPlainFormatter extends FormatterBase implements ContainerFactoryPlu
   +    $this->viewMode = $view_mode;
   

   These are pointless, since FormatterBase already has this.

5. +++ b/src/Plugin/Field/FieldFormatter/AddressPlainFormatter.php
   @@ -147,6 +155,10 @@ class AddressPlainFormatter extends FormatterBase implements ContainerFactoryPlu
   +      '#element' => [
   +        '#address' => $address,
   +        '#view_mode' => $this->viewMode,
   +      ],
   

   Since we're inserting this into the available variables for the address-plain.html.twig template, we should also update the doc comment in that template to document that these are available for anyone who wants to use them.

Thanks,
-Derek

I needed this to finally solve #2838509: Allow a field formatter to output names and codes for all subdivisions (e.g. for use with search indexing) for a site I’m building. ;)

Attached patch fixes everything from #21.

Re: #21.3: I looked at both node_theme_suggestions_node() and system_theme_suggestions_field() for inspiration on what suggestions to provide and in what order. I think this is a pretty solid list:

  $suggestions[] = $original . $entity_type_id . '__' . $sanitized_view_mode;
  $suggestions[] = $original . $entity_type_id . '__' . $bundle;
  $suggestions[] = $original . $entity_type_id . '__' . $bundle . '__' . $sanitized_view_mode;
  $suggestions[] = $original . $field->getName();
  $suggestions[] = $original . $entity_type_id . '__' . $field->getName();
  $suggestions[] = $original . $entity_type_id . '__' . $field->getName() . '__' . $bundle;

While I was looking closely at all this again, I didn’t love #element as a template variable with nested values for the address field and the view mode. Seems potentially confusing with form / render “element”, etc. If anything, I’d maybe call it #context or something. But the node.html.twig template gets both node entity object and view_mode as separate variables. So I went with that approach here, too. This also made it easier to document what’s going on in address-plain.html.twig (#21.5).

Finally, I added test coverage by extending tests/src/Kernel/Formatter/AddressPlainFormatterTest.php (inspired by core/modules/node/tests/src/Functional/NodeTemplateSuggestionsTest.php). So I’m attaching a test-only that (obviously) fails, and the full patch.

Any final reviews / concerns / objections before I commit this?

Thanks!
-Derek

p.s. This would be good to get in before 8.x-1.8

It seems assertEqual() is deprecated (although only for D10), so we might as well avoid using it now.

Thanks, rtbc!

Looks good!

Great, thanks for the reviews!

Committed and pushed to 8.x-1.x.

Cheers,
-Derek

Note: we all completely forgot that '#theme' => 'address_plain', is not only used for address fields. This issue broke things for address form elements and other render arrays that aren't necessarily a Field API field. :( See #3129410: Error in hook_theme suggestions