Add support for field properties

+++ b/token.tokens.inc
@@ -1205,6 +1207,15 @@ function field_token_info_alter(&$info) {
+      foreach ($field->getPropertyDefinitions() as $property => $property_definition) {
+        $info['tokens'][$token_type][$field_name . ':' . $property] = [
+          'name' => new HtmlEscapedText($label),

@@ -1261,6 +1272,14 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
+      $split = explode(':', $name);
+      $property_name = NULL;

: is the separator for recursion but this somehow cheats its way around that, I think that is very weird.

I guess we need a token type per field type instead and expose the properties below there.

Also, there is the case of deltas and accessing a specific delta, not just the first. A thing that I've done elsewhere is check if the first key is a number, then use it as the index and if it is a string, use it as the property name for the first delta.

Also, $label there seems wrong, that should be the property label?

#2493559: Chained tokens for field tokens is related AFAIK, might want to combine this, not sure.

1. +++ b/src/Tests/TokenEntityTest.php
   @@ -104,4 +113,67 @@ class TokenEntityTest extends TokenKernelTestBase {
   +  public function testEntityFieldTokens() {
   

   Yay tests.

   Would be great to have a second check with a node that has no value for that field, to make sure that doesn't result in any errors (see what I wrote down below).

   And what would be great too is checking the defined token info for our field.

2. +++ b/src/Tests/TokenEntityTest.php
   @@ -104,4 +113,67 @@ class TokenEntityTest extends TokenKernelTestBase {
   +    $view_mode = EntityViewMode::create([
   +      'id' => 'node.token',
   +      'targetEntityType' => 'node',
   +    ]);
   +    $view_mode->save();
   +    $entity_display = entity_get_display('node', 'article', 'token');
   +    $entity_display->setComponent('test_field', ['type' => 'text_default']);
   +    $entity_display->save();
   

   Related to #2603652: Fix token default formatter usage. Whatever solution we end up with exactly there, that should make this part unnecessary. Maybe we can just remove it there to get some sort of test coverage.

3. +++ b/src/Tests/TokenEntityTest.php
   @@ -104,4 +113,67 @@ class TokenEntityTest extends TokenKernelTestBase {
   +    $renderer = \Drupal::service('renderer');
   +    $context = new RenderContext();
   +    $renderer->executeInRenderContext($context, function() use ($entity, $format) {
   +      $this->assertTokens('node', ['node' => $entity], [
   +        'test_field' => Markup::create('foo'),
   

   Is this really still needed after the renderPlain() fix has been committed? I wrote tests for pathauto that use field tokens and it works just fine, so you should be able to drop this.

4. +++ b/token.tokens.inc
   @@ -1196,6 +1198,15 @@ function field_token_info_alter(&$info) {
   +      // Property tokens.
   +      foreach ($field->getPropertyDefinitions() as $property => $property_definition) {
   +        $info['tokens']['entity_field__' . $field_name][$property] = [
   

   Hm, interesting, So we define a token type for each field, not field type. Kind of makes sense, since their properties vary per field storage.

   However, this still overlaps across entity types. So if you have a field_something as textfield on nodes and field_something as an entity_reference users, then this is going to end in a mess :)

   Maybe use the same pattern as we use for field storage tables names?

   $entity_type_id . '__' . $field_name?

   Either way, this will result in a huge amount of token types on sites with many fields but I don't see a way around that.

   And last, I think you also need to define this as a token type, not just the tokens for it?

5. +++ b/token.tokens.inc
   @@ -1196,6 +1198,15 @@ function field_token_info_alter(&$info) {
   +          'name' => (string) new HtmlEscapedText($label),
   

   Why the indirection? This is the same as just calling Html::escape() ?

6. +++ b/token.tokens.inc
   @@ -1253,27 +1264,55 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
        foreach ($tokens as $name => $original) {
   +      $split = explode(':', $name);
   +      $property_name = NULL;
   

   It's a pretty funky trick, but what about appending a ':' like this: list($name, $property_name) = explode(':', $name, 2) to ensure we always have at least two parts with the second then being emty? See HtmlEntityFormController::getFormObject()

   At least you need to limit either the explode to two parts or make it count($split) > 1 as we'll eventually want to support tokens like 'node:uid:entity:name' ?

7. +++ b/token.tokens.inc
   @@ -1253,27 +1264,55 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +        $field_names[$name] = $name;
    
            // Do not continue if the field is empty or not a configurable field.
            if ($entity->get($name)->isEmpty() || !($entity->getFieldDefinition($name) instanceof FieldConfigInterface)) {
              continue;
   

   should we move the $field_names part down, so we don't bother trying to handle an empty or non-supported field?

8. +++ b/token.tokens.inc
   @@ -1253,27 +1264,55 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +    foreach ($field_names as $field_name) {
   +      $field_tokens = \Drupal::token()->findWithPrefix($tokens, $field_name);
   +      $replacements += \Drupal::token()->generate('entity_field__' . $field_name, $field_tokens, ['field_item' => $entity->get($field_name)], $options, $bubbleable_metadata);
   +    }
   

   Looks pretty solid and as performant as it gets but some better variable names would be great ($fields_to_recurse? $fields_with_properties?, ..?) and also some comments that explain what we are doing here.

9. +++ b/token.tokens.inc
   @@ -1253,27 +1264,55 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +
   +    foreach ($tokens as $name => $original) {
   +      $replacements[$original] = $field_item->first()->{$name};
   +    }
   

   What happens if you try to use a non-scalar property like entity, language, date? Maybe we should not expose them as tokens for now?

   There's PrimitiveInterface, so we could only expose those where the class implements that and here use get($name) so we can check the interface too.

Is this a duplicate of #2493567: Allow field tokens also for base fields?

No. The other issue is about automatically supporting tokens for e.g. node:uid too, not just configurable fields.

This is about supporting tokens for properties, e.g. something like [node:body:format].

If you're looking for ways to help, a good way to do so would be to take the tests that are being added here, and open a new issue to just add them, adjusting them so that they test our existing coverage. We have various other issues that are trying to fix bugs around field tokens. Having a starting point for those tests would make it much easier to expand field token test coverage in each issue that is touching them. So far, we've postponed test coverage on bugfixes.

That would be part of the #2430827: [meta] Improve field support and add test coverage, just like all the other field token issues. What would also help is to update that, make sure all related issues are sub-issues of that and provide an overview there.

Created #2646316: Add tests for field tokens as a child issue.

Rerolling #11 after all the recent changes. The test is pretty much the same to what was added in #2646316: Add tests for field tokens but I added the asserts for the properties.

1. +++ b/token.tokens.inc
   @@ -1200,6 +1202,15 @@ function field_token_info_alter(&$info) {
   +          'name' => (string) new HtmlEscapedText($label),
   

   I think that this should be (string) $property_definition->getLabel().

2. +++ b/token.tokens.inc
   @@ -1200,6 +1202,15 @@ function field_token_info_alter(&$info) {
   +          'description' => $description,
   

   This one should be (string) $property_definition->getDescription().

3. +++ b/token.tokens.inc
   @@ -1254,27 +1265,55 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +  if (strpos($type, 'entity_field__') === 0) {
   

   Wouldn't it be better (as @berdir suggested in a previous comment) to keep these tokens under the type? The getInfo() method would return something like this https://gist.github.com/juampynr/0022585c6dfaf7d74a71.

Just a reroll of #17 so that it's applicable.

This is still bleeding from several wounds but a feedback if the direction is right would be nice. The main change is that it's using token types instead of the split() hack. I think one token type per field_name is enough (i.e. no need for 'entity_type__' . $entity_type_id . '__' . $field_name), the $data will contain the actual context anyway.

TODO:
- Tests
- Rendering
- Field deltas
- Look through #12 if anything is not addressed.

What if you have a field field_example on node and users? and on node, it is an entity reference and on users, it is a text field? You need the entity type in there to avoid that conflict, the field name is no longer globally unique.

Also, it is theoretically possible that someone would create a field called "node". that would likely also result in a nice mess ;)

Which is why I'm suggesting the same namespacing as the storage tables use.

@Berdir
The "same field name on different entities" case is taken care of by the following lines:

 if ($field_tokens = \Drupal::token()->findWithPrefix($tokens, $field_name)) {
        $data = [
          'token_type' => 'field_property',
          $field_name => $data['entity']->get($field_name)->getValue(),
          'field_name' => $field_name,
        ];
        $replacements += \Drupal::token()->generate($field_name, $field_tokens, $data, $options, $bubbleable_metadata);
      }

which calls hook_tokens() again which will execute:

  elseif ($data['token_type'] == 'field_property') {
    foreach ($tokens as $name => $original) {
      $replacements[$original] = reset($data[$data['field_name']])[$name];
    }
  }

So hook_tokens() actually does not need to know about the entity type, the $field_name and the property name (included in $field_tokens) is enough.

Just to make sure, I tried it and it works. I have created a field_test on node (long text) and user (content reference). I have created some content and ran the following code in devel/php:

$node = node_load(2);
$string = 'node [node:field_test]  [node:field_test:format] ';
dpm(\Drupal::token()->replace($string, ['node' => $node]));
$user = user_load(1);
$string = 'user [user:field_test]  [user:field_test:target_id]';
dpm(\Drupal::token()->replace($string, ['user' => $user]));

The result was:

node <p>body2</p>  basic_html 
user <a href="/node/2" hreflang="en">Article</a>  2

as it should be.

Or is this not how hook_tokens() is meant to be used?

I will test the "use 'node' as field name" case too, but I can't see why it would cause any problem.

Sure, hook_tokens() doesn't have a problem with it. But hook_tokens() doesn't even need field types to exist in the first place. It has no validation or checking of those. The info hook exists solely for token browsing/UI. And that isn't going to work.

Now I get it.

This patch combines the two approaches. It creates token types with the name 'node-field_test' and 'user-field_test'. At the same times tokens are
[node:field_test] (the original, rendered field token)
[node:field_test:value] (the value property)
[node:field_test:format] (the format property)
[user:field_test:target_id] (the target_id property)
This way tokens inside entities appear where one would expect them to appear.

The attached screenshot contains pretty much all this.

Is this a good approach? If so, I can continue with the TODOs in #20.

To make it clear: my question refers to the using of [node:field_test:value] instead of [node:node-field_test:value].

Also, using a '-' to separate the entity type and the field name instead of '_' or '__' looked safer since '-' is not allowed in machine names.

yes, node:field_test:value makes sense.

Not 100% sure about - yet, we'll have to check that with dave.

Also, please provide interdiffs, it's very hard otherwise to review incremental changes.

Thanks for working on this.

I'm looking at entity_token_token_info_alter() in the D7 entity module to solve the field delta tokens. It's defining "list<$token_type>" token types. Do we want to implement this only for multivalue fields or is there anything else that this could be useful for?

This patch is adding field deltas to only hook_token_info() (hook_tokens() are coming). The pattern is the same as in D7 entity module entity_token_token_info_alter().

Just a screenshot of the token browser. Note that for node:field_test (being a multivalue field) there are deltas while for user:field_test (being a single value field) there are not.

Added the hook_tokens() part for field deltas.

The patch handles the following tokens:
[entity:field_test] (renders the whole field)
[entity:field_test:0] (renders the 0th delta field item)
[entity:field_test:0:value] (shows the 'value' property of the 0th field item)
[entity:field_test:value] (same as [entity:field_test:0:value])
[entity:field_test:1:value] (shows the 'value' property of the 1st field item)

Adding a check for the property data type implementing PrimitiveInterface in hook_token_info().

If this is ok so far then I will focus on tests.

Removing a change that served just testing purposes.

1. +++ b/tests/src/Kernel/FieldTest.php
   @@ -94,6 +94,8 @@ class FieldTest extends KernelTestBase {
        $this->assertTokens('node', ['node' => $entity], [
          'test_field' => Markup::create('foo'),
   +      'test_field:value' => 'foo',
   +      'test_field:format' => $format->id(),
        ]);
   

   We'll need to expand tests, not just for the replacements but also for token info.

2. +++ b/token.tokens.inc
   @@ -1195,11 +1197,54 @@ function field_token_info_alter(&$info) {
   +      $cardinality = $field->getCardinality();
   +      $cardinality = ($cardinality < 0 || $cardinality > 3) ? 3 : $cardinality;
   +      $field_token_name = $token_type . '-' . $field_name;
   

   I would make this an explicit check on the unlimited constant. It's kind of an implementation detail that it is -1.

3. +++ b/token.tokens.inc
   @@ -1195,11 +1197,54 @@ function field_token_info_alter(&$info) {
            'module' => 'token',
   +        // For multivalue fields the field token is a list type.
   +        'type' => $cardinality > 1 ? "list<$field_token_name>" : $field_token_name,
   

   One design principle for entity field api in 8.x was that non-delta access always works, even when the field is multi-value.

   I'm not sure if we can do that here, but I think it would be good to at least support it when doing replacements. Otherwise making a field multiple could break existing tokens.

4. +++ b/token.tokens.inc
   @@ -1195,11 +1197,54 @@ function field_token_info_alter(&$info) {
   +            'description' => t('The list item with delta @delta. Delta values start from 0 and are incremented by one per list item.', ['@delta' => $delta]),
   

   do we really need that to be repeated on every line? descriptions are optional in 8.x, and the list starts with 0, so it should be pretty obvious that it starts with that. I'd just remove it.

5. +++ b/token.tokens.inc
   @@ -1195,11 +1197,54 @@ function field_token_info_alter(&$info) {
   +      // Property tokens.
   +      foreach ($field->getPropertyDefinitions() as $property => $property_definition) {
   +        if (is_subclass_of($property_definition->getClass(), 'Drupal\Core\TypedData\PrimitiveInterface')) {
   

   Nice, so in the other issue about references, we can extend this to also support entity references.

6. +++ b/token.tokens.inc
   @@ -1195,11 +1197,54 @@ function field_token_info_alter(&$info) {
   +            'name' => (string) $property_definition->getLabel(),
   +            'description' => (string) $property_definition->getDescription(),
   

   Do we really need the string cast here? it should be possible to have it translated when actually accessed, the t() above does the same.

7. +++ b/token.tokens.inc
   @@ -1261,12 +1304,17 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +      list($field_name, $delta) = explode(':', $name, 2);
   +      if (!is_numeric($delta)) {
   +        unset($delta);
   

   php > list($field_name, $delta) = explode(':', 'example', 2);
   PHP Notice: Undefined offset: 1 in php shell code on line 1

   One thing you can do is append a ':' on the explode, or check if you have a : in there, before exploding.

8. +++ b/token.tokens.inc
   @@ -1284,16 +1332,46 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +    $fields = \Drupal::entityManager()->getFieldStorageDefinitions($data['entity_type']);
   +    foreach ($fields as $field_name => $field) {
   +      /** @var \Drupal\field\FieldStorageConfigInterface $field */
   +      if (!($field instanceof FieldStorageConfigInterface)) {
   +        continue;
   +      }
   +      if ($field_tokens = \Drupal::token()->findWithPrefix($tokens, $field_name)) {
   +        $data = [
   

   We already have the field name above. can't we put this logic into the existing loop, otherwise we have to loop over this for every field for that entity type, which likely means dozens.

   Also can't see how the $delta then works here when you have field_name:0:value, which I think is actually more common than just field_name:0.

9. +++ b/token.tokens.inc
   @@ -1284,16 +1332,46 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +      // Handle [entity:field_name:0:value] tokens.
   +      list($delta, $rest) = explode(':', $name, 2);
   +      $name = is_numeric($delta) ? $rest : $name;
   +      // [entity:field_name:value] is treated as [entity:field_name:0:value].
   +      $delta = is_numeric($delta) ? $delta : 0;
   +      $replacements[$original] = $data[$data['field_name']][$delta]->getValue()[$name];
   

   oh, you access that here. I think we should probably handle the delta separately, and just pass along to the field property. Also, you can just do ->$delta, instead of going through getValue(), it will also prevent notices.

   We also need to check if the delta is set, or an exception will be thrown.

@Berdir Thanks for the review!
1. Todo, coming this week hopefully.
2. Fixed.
3.

One design principle for entity field api in 8.x was that non-delta access always works, even when the field is multi-value.

With the current patch hook_tokens handles [node:field_test:format] the same way as [node:field_test:0:format], [node:field_test] however renders the whole field (with all deltas). So making a field multivalue should not break anything. (In token_info() this is not explained atm.)
Is this not how hook_tokens() is supposed to work?
4. Removed.
5. To be done in the other issue.
6. Removed (string).
7. Fixed, I chose checking first if I have : inside.
8. Fixed, now there's only one loop.
9.
I did not refactor the delta handling, I will think about it how to do it nicely.
I'm making a check if delta isset.
The ->$delta does not work for me, it creates a NULL object.

I rebased the patch and removed the parts of the test that was causing it to fail. I added a test for multivalued fields. Also, for fields of cardinality x, x + 1 tokens were generated :

-        for ($delta = 0; $delta <= $cardinality; $delta++) {
+        for ($delta = 0; $delta < $cardinality; $delta++) {

I tested this locally and it works fine.

Quick review.

1. +++ b/tests/src/Kernel/FieldTest.php
   @@ -98,15 +119,28 @@ class FieldTest extends KernelTestBase {
          ],
   +      'test_list' => [
   +        'value' => 'key1',
   +      ],
   

   I would store at least two values

2. +++ b/tests/src/Kernel/FieldTest.php
   @@ -98,15 +119,28 @@ class FieldTest extends KernelTestBase {
        $this->assertTokens('node', ['node' => $entity], [
          'test_field' => Markup::create('foo'),
   -      'test_field:value' => 'foo',
   -      'test_field:format' => $format->id(),
   +      'test_list:0' => Markup::create('value1'),
   +    ]);
   

   We still need :value and :format, we want to make sure that works and also test_list:1:value (once you have one) and test_field:0:value.

3. +++ b/tests/src/Kernel/FieldTest.php
   @@ -98,15 +119,28 @@ class FieldTest extends KernelTestBase {
   +    $this->assertNoTokens('node', ['node' => $entity], [
   +      'test_list:1',
   

   this simply changes to 2 then.

4. +++ b/tests/src/Kernel/FieldTest.php
   @@ -98,15 +119,28 @@ class FieldTest extends KernelTestBase {
   +    // Test the test_list token metadata.
   +    $tokenService = \Drupal::service('token');
   +    $token_info = $tokenService->getTokenInfo('node', 'test_list');
   +    $this->assertEqual($token_info['name'], 'test_list');
   +    $this->assertEqual($token_info['module'], 'token');
   

   we also need to check the type and then the token info for that type and so on.

Here we go.

Reroll, it still needs work.

I could not create a proper interdiff (interdiff: Error applying patch1 to reconstructed file) so I'm attaching a plain diff.

Proper extension

Fix tests.

Needs work, just curious of the testbot's result.

-    /** @var \Drupal\Core\Field\FieldTypePluginManager $fieldTypeManager */
+    /**
+     * @var \Drupal\Core\Field\FieldTypePluginManager $fieldTypeManager
+     */

The former way is how projects usually do it, if we disregard the camelCase variable instead of the expected snake_case.

@bojanz: Fixed it.
Current test should pass with this patch now.

Let's add the missing tests.

All points in #40 have been addressed. Now the patch is ready for review.

An issue I noticed is with the node body field which has its token integration in the node module. Since we don't want to overwrite any token integration here node body field property tokens are not available. Might be worth a separate issue.

Small change to no longer show nested tokens on the token browser (before and after screenshots attached). Otherwise, the token browser was excessively cluttered.

Note that the last change is now being worked on in #2760315: Some nested tokens should not show as top-level tokens.. If that gets in first, we'll build on top of it, otherwise it can be completed in there.

What I'm worried most about this and especially also the next level of reference/chained tokens (#2493559: Chained tokens for field tokens) is the performance impact.

Based on our install profile that has 138 field storage configs, 241 field configs and 20 content entity types (going to try and repeat this with the biggest project based on this which has a lot more field configs), I compared the current alpha version, dev (base field token support) and this patch.

I compared three values, the size of the token_info cache entry, the time it took to render /admin/help/token and the number of token types. Note that the cache is stored in the discovery cache bin, which is in APCu if available.

version: cache size / ~render time / token types
alpha2: 130kb / 5.7s / 25
dev: 256kb / 10.2s / 78
patch: 523kb / 14s (10.5s with the improvement by @Bambell) / 388

That's an interesting explosion :) Note that caching of the token tree is currently not yet implemented, that will help a bit, but much more time is spent in the browser displaying it than building it on the server.

For comparison, that makes it the second-biggest cache entry in cache_discovery, only the full views_data cache entry is bigger (3x as big, in fact). #2493559: Chained tokens for field tokens should *not* result in more types, but I expect it will basically result in the same problem that all big D7 sites already suffer. A dead-slow to completely unusable/broken token browser.

We don't really have any alternatives, we can't change how the token API works. But this is something to keep in mind as we work on those issues. One small possible improvement that I identified is that we now also define token types for config entities, with only two tokens, url and :original. I think we should skip those for now until we actually have something to show for them.

Code review, I think we're getting close here:

1. +++ b/token.tokens.inc
   @@ -1284,46 +1327,111 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +          if ($field_delta = $entity->{$field_name}[$delta]) {
   +            $field_output = $field_delta->view($display_options);
   +          }
   +          // If no such delta exists, let's not replace the token.
   +          else {
   +            continue;
   +          }
   

   This is usually called $field_item (vs. just field or field_items for the whole field object), delta would be just the number.

2. +++ b/token.tokens.inc
   @@ -1284,46 +1327,111 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
      }
   -
   +  elseif (!empty($data['token_type']) && $data['token_type'] == 'field_property') {
   +    foreach ($tokens as $name => $original) {
   

   token_type is a bit strange, I don't think that's used anywhere else?

   If we'd prefix those tokens, then we could match it on that and split it up again.

3. +++ b/token.tokens.inc
   @@ -1284,46 +1327,111 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
   +      if ($field_delta = $data[$data['field_name']][$delta]) {
   +        $field_delta_value = $field_delta->getValue();
   +        if (isset($field_delta_value[$name])) {
   +          $replacements[$original] = $field_delta_value[$name];
   +        }
   +        // This is not a field property token.
   +        else {
   +          continue;
   +        }
   

   This will not work for the changed tokens that the mentioned issue will introduce. But that will need a different approach anyway.

   Still, I think just doing $data[$data['field_name']][$delta]->$name should give you the same.

   Also, this should check for a non-existing delta to avoid throwing an exception. Lets make sure we also check that in the test, with :3:value or so. Same for above where we render the field item.

Thanks everyone for working on this.

We don't really have any alternatives, we can't change how the token API works.

For the record, we *can* change how the token *browser*/UI looks and works like. Things like on-demand loading of nested/chained tokens and not everything up front would make a huge difference there. There are huge 7.x issues about that topic, no idea about the current state there.

Thanks for the review!

#1: Fixed.
#2: Setting a magic prefix seems weird to me but I understand that token_type is not good either. What about a 'field_property' => TRUE ?
#3: Fixed.

Added an additional test for 'test_list:2:value'. All token tests pass locally.

Shoot, I haven't included Bambell's changes.

That's better.

Ok, I plan to commit this soon, last chance for @davereid to jump in ;)

Btw, I committed #2760315: Some nested tokens should not show as top-level tokens., so this won't apply anymore cleanly, still seems to apply with git apply -3 though.

As promised, I did test this on our largest site, the numbers for this patch are: 1M cache size (vs 3M for views data), 648 token types (!) and a lovely 33s to render /admin/help/token. So already getting very close to being unusable and breaking other pages when not using the dialog. But that's a problem we need to fix with a better token browser.

Easy re-roll.

Ok, that's it, committed!

Thanks everyone who worked on it, this is a big one :)