Statically cache the list of address fields during token generation

I have found this too while profiling with xhprof

Looks fine for me too - i could drop the number of calls from of addressfield_field_map_filter from 37,008 to 144.

I've been using this for a month now. I can't see any downsides to this approach.

Extra whitespace lines can be fixed on commit.

We have access to the entity type in $data['entity_type'], we can pass that to addressfield_get_address_fields() to further restrict the returned data.
Will save us from iterating on a bunch of non-needed fields.

Added $entity_type as an optional argument to addressfield_get_address_fields(). It needs to be optional since entity_type is not always available in the caller function.

Thank you @czigor:)

Just some nit picks, but I think this looks pretty good as well.

1. +++ b/addressfield.module
   @@ -53,6 +53,29 @@ function addressfield_module_implements_alter(&$implementations, $hook) {
   +function addressfield_get_address_fields($entity_type = '') {
   

   Could you add this param to the docblock above?

2. +++ b/addressfield.module
   @@ -53,6 +53,29 @@ function addressfield_module_implements_alter(&$implementations, $hook) {
   +  $fields = &drupal_static(__FUNCTION__ . '_' . $entity_type);
   

   Is it ok to just have the static's function name with an underscore when it's defaulting to empty string?

3. +++ b/addressfield.module
   @@ -53,6 +53,29 @@ function addressfield_module_implements_alter(&$implementations, $hook) {
   +  if (is_null($fields)) {
   

   We ususally use !isset() it seems to 5% faster(but both are fast it's mostly for consistency). Also could just return early to avoid the extra nesting. (this of course was from the first patch)

Thanks for the review!

1 and 3 have been fixed.

I can sleep with leaving 2 as it is, but if it's a problem, I can fix it as well. It's just one more line of code which I thought was not worth it.

A ! was missing from the previous patch.
@joelpittet Where should I return early?

@czigor I was thinking like:

$fields = &drupal_static(__FUNCTION__ . '_' . $entity_type);
if (isset($fields)) {
  return $fields;
}
...

To avoid the overly nested code.

@joelpittet Ah yes, that makes sense.

Thank you for your persistence @czigor, this should do the trick.

Lightly updated patch attached. (Documentation fixes / a variable name.)

Ok ... so, this isn't the fault of this patch. In theory, it's right. The problem is that the $field_name value isn't actually going to match anything in the token data array because field names there use hyphens in place of underscores. Arguably, we could commit this improvement without fixing that, but I'd prefer to kill two birds with one stone ... otherwise the solution to this issue could've just as well have been to remove this logic entirely. : P

Updated patch attached that uses strtr() to convert underscores to hyphens in what I believe to be the appropriate places needs review.

Also, I realized my patch changes the logic from previous patches. Previously, filtering fields worked like so:

if (!empty($fields)) {
  // Keep only the fields present on this entity_type to spare some
  // iterations in the calling function.
  foreach ($fields as $field_name => $field) {
    if (!isset($field['bundles'][$entity_type])) {
      unset($fields[$field_name]);
    }
  }
}

But my patch added an additional !empty($entity_type) to that if statement. My read on the earlier version is that if $entity_type were empty, every field would have been unset. Am I reading that wrong? In any event, review the approach in the current patch to see if it's screwing something up now, because I do think it's the intended behavior.

We've been using the patch by steven.wichers for more than half a year on freitag.ch without a problem. I gave the patch in #15 a try on my local machine, I could see no problems. The checkout process was fast, the sent emails (using addressfield tokens) were right.

The code looks good to me. As per the strstr() part: I'm no expert here but isn't the token module way (_) more general? Not all entities are based on entity api (-).

@rszrama doesn't the token string replacement changing the API in this patch. It may be right but may be scope creep for this specific issue. I'm not sure if it would break a site's existing tokens, maybe @czigor tested that as it sounds like it. Dropping it back to NR for that question

@joelpittet In my limited understanding if the addressfield is on an entity using the API of the entity API contrib module, then field tokens with '-' work. Otherwise '_' should be used. But this is just a gossip-level knowledge, would be nice if someone could confirm it.

According to comments here https://www.drupal.org/project/addressfield/issues/2935041, this issue is blocking release 1.3 (which would include a fair number of improvements and fixes since last release). Can we either push this issue forward or attempt to release 1.3 without this issue? <-- @bojanz

Given the point at which the discussion is on this issue, it would seem best to leave this issue behind in a 1.3 release, and allow the other fixes to go in.

I've been troubleshooting this issue and testing the patch from comment #15 today and found out the following:

First of all, here's the list of tokens when the token module is NOT enabled:

When enabled, the list is slightly different:

When the token module is not enabled, only the token with "dashes" exists.

So with the patch in #15, printing the following would work:

[commerce-order:commerce-customer-billing:commerce-customer-address:country]

But [commerce-order:commerce-customer-billing:commerce_customer_address:country] wouldn't.

The attached patch supports both (I tested it locally by printing the 2 tokens in the order confirmation email and it worked as expected).

Good deal, thanks for the reroll / improvements. I've confirmed what you've said, that the address field tokens with hyphens or underscores will still be present / identified as address fields before and after the patch. Committed!

Be aware,

previously working token do not work anymore : [message:message-commerce-order:commerce-customer-billing:commerce-customer-address:first_name] instead need to use [message:message-commerce-order:commerce-customer-billing:commerce-customer-address:first-name].

Unlike, specified in generate token list help (exemple for message type) :

Field "commerce_customer_address". The following properties may be appended to the token: country (Country), name_line (Full name), first_name (First name), last_name (Last name), organisation_name (Company), administrative_area (Administrative area (i.e. State / Province)), sub_administrative_area (Sub administrative area), locality (Locality (i.e. City)), dependent_locality (Dependent locality), postal_code (Postal code), thoroughfare (Thoroughfare (i.e. Street address)), premise (Premise (i.e. Apartment / Suite number)), sub_premise (Sub Premise (i.e. Suite, Apartment, Floor, Unknown.)

Is this somehow affecting 8?