Fix token default formatter usage

DuaelFr, yes I believe testing for 8.x is up to date: https://www.drupal.org/node/1319154#multiple-versions

Tests pass again now.

This sounds like it might have some non-trivial runtime overhead. Maybe a better approach would be to provide a default view display for the token view mode and automatically configure it with the default formatter when a new field is added? Then it would be clear for a user what is actually used, unlike changing it at runtime in some magical way?

Or, we could check the existence of a token view display when actually viewing a token. And if we can't find it, just explicitly pass the default configuration/formatter to view() ?

This basically rerolls the patch in #5 and improves it a little bit. Basically, I remove the load() from inside the loop and replaced it with a loadMultiple(). I am not sure if it will address the runtime overhead concerns @Berdir raised in #9 but it should definitely help.

About the points in #9 itself:

provide a default view display for the token view mode and automatically configure it with the default formatter when a new field is added? Then it would be clear for a user what is actually used, unlike changing it at runtime in some magical way?

This sounds good. I know we have another issue that really deals with that - #2430821: Token view modes are only created for entity types that exist when token is installed. However, it sounds to me that the first point is actually what happens now if the token view display is present and enabled.

Or, we could check the existence of a token view display when actually viewing a token. And if we can't find it, just explicitly pass the default configuration/formatter to view()

Isn't this what we are doing here? The hook is called only when viewing the token, right?

This makes the hook even simpler. There is no need to load the view display when the current view display gives the information we need.

There are some other optimizations that can be done like removing getDefinition() from inside the loop but they are not that impactful. They just return cached data anyway. We are back at the question at what we really should do here.

Isn't this what we are doing here? The hook is called only when viewing the token, right?

Kind of, yes. But we do it very indirectly, so when you look at the code that generates the token, it's hard to understand why the token looks the way it does. We can help with that by adding a simple @see in the code there.

Or we could do something like this:

if ( $token_view_display) {
  $entity->$field->view('token');
else {
  $entity->$field->view($formatter_default_settings);
}

I see. Either way, it means that we will still need the hook we are working on at the moment, right? There isn't going to be any view display ready with all the formatters set to the default_token_formatter, which means we have to alter it in the entity_display_alter like we do in the patch. Is this a correct assessment?

view() accepts two argument types. A string, then it's a view mode and it will use the settings of that. Or, an array, then it's the formatter type + settings. It will create a runtime view display and work directly with those settings. We still need more or less the code from the hook but it wouldn't be a hook anymore, it would be a helper function/method that directly returns the array we need.

That has two advantages IMHO:
* The code flow is easier to understand
* We only need to get the information for the fields we are actually displaying. If you look at getSingleFieldDisplay(), we first get the full view display, alter all fields and then throw all non-matching ones out.

Thanks for the tips. The patch attached does that. Interdiff is missing because it mostly won't make sense at all as this is a very different change.

Removing the use statements from the token.module.

I guess it is still moving:

21:20:39 fatal: unable to access 'https://git.drupal.org/project/drupalci_testbot.git/': server certificate verification failed. CAfile: /etc/ssl/certs/ca-certificates.crt CRLfile: none
21:20:39 error: Could not fetch origin

I will call it a day now and see if the CI issues have been resolved by morning.

All pass. Back to needs review.

+++ b/token.tokens.inc
@@ -1264,7 +1270,19 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
+          $display_options = $token_view_display ? 'token' : ['type' => $formatter];

this conditional here can never be TRUE?

We should also make sure that the label is hidden. Needs to be on the same level as type I think, not inside settings.

Other than that, yes, I think its is easier to understand?

Lets wait with committing this for #2646316: Add tests for field tokens, then we can add test coverage.

Ah, that was left over from before when I was trying tertiary. Thanks, and fixed! I also set label to hidden. By default 'label' is set to 'above'.

About waiting for #2646316: Add tests for field tokens, the only change will be to remove the manual token block, right? I am not sure if that is enough. The patch here only makes sure the token view mode is used, if available. Otherwise default token formatter or default formatter is used. I don't think the patch is testing for any of these things. Besides, I think this also depends on what happens in #2430821: Token view modes are only created for entity types that exist when token is installed, which will decide how the view display will be created in the first place.

#2646316: Add tests for field tokens is in, lets improve the test coverage here.

I think we can test two things, which should test this sufficiently:

1. We remove the form display/view mode from where it is now. The test should then still pass as we should automatically use the default settings.
2. We re-add the view display later in the test and configure it explicitly to non-default settings (show the label, maybe use trimmed and only show a few characters, something like that. Then it should replace those explicit settings.

#2430821: Token view modes are only created for entity types that exist when token is installed shouldn't affect this. It's a kernel test, we already have to add the view mode by hand anyway.

I am just checking for point 1 in the patch. Let's see what happens.

Adding it in a new test method. This is somewhat similar to the existing testEntityFieldTokens method and I was considering putting it in there but I wanted to know your thoughts.

Learnings while implementing it (possible bugs):

• We need to call Unicode::check() in kernel tests if the tests (in)directly use Unicode::strlen().
• If one of the assertEqual fails and the comparison is on an object, PHPUnit may try to unserialize it in a different process which might through weird exceptions like ContainerNotInitializedException. I had to debug this in PHPUnit to figure it out. Even when the test was executing normally but there was only a slight difference in asserted values, I faced this seemingly weird error.

  The reason is that PHPUnit apparently serializes the object and sends it to stdout. The runner process takes this and unserializes it. In this case, the field token might be holding a reference to the entity, which when unserialized tries to get the container, which fails.

Just two tiny things then we should be good to go here.

1. +++ b/tests/src/Kernel/FieldTest.php
   @@ -155,4 +147,53 @@ class FieldTest extends KernelTestBase {
   +    // Create a node with a value in the text field and test its token.
   +    $format = FilterFormat::create([
   +      'format' => 'test',
   +      'weight' => 1,
   +      'filters' => [
   +        'filter_html_escape' => ['status' => TRUE],
   +      ],
   +    ]);
   +    $format->save();
   

   Should we move the filter into setUp()? I don't think we'll need different filters...

2. +++ b/tests/src/Kernel/FieldTest.php
   @@ -155,4 +147,53 @@ class FieldTest extends KernelTestBase {
   +    $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_trimmed',
   +      'settings' => [
   +        'trim_length' => 50,
   +      ]
   +    ]);
   +    $entity_display->save();
   +
   +    $this->assertTokens('node', ['node' => $entity], [
   +      'test_field' => Markup::create(substr($value, 0, 50)),
   +    ]);
   +  }
   

   A commit to explain this part would be great. Just that we explicitly create a token view display that should override the default configuration.

I hope the comment is proper. I also added a comment explaining Unicode::check().

+++ b/tests/src/Kernel/FieldTest.php
@@ -152,24 +157,20 @@ class FieldTest extends KernelTestBase {
+
+    // The formatter we are going to use will eventually call Unicode::strlen.
+    // This expects that the Unicode has already been explicitly checked, which
+    // happens in DrupalKernel. But since that doesn't run in kernel tests, we
+    // explicitly call this here.
     Unicode::check();

Yeah, that's strange. Maybe open a core issue to do this by default? Surprised that nobody so far did run into this, I'd expect we have some Unicode:: calls in places that are often invoked?

Committed.