Currently field_tokens does not consider the language provided in $options['langcode']. However, when the enity belonging to this field has a translation of this language, that one should be used.

We should also consider not translatable ER fields which are translated.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work
Maouna’s picture

Status: Needs work » Needs review
FileSize
895 bytes

Status: Needs review » Needs work
Maouna’s picture

Once https://www.drupal.org/node/2499973 is included, the patch above won't fail anymore.

Maouna’s picture

Status: Needs work » Needs review

darol100’s picture

#2499973: Remove usage of getSystemPath(), compatibility with PHP7 this seem to be solve already. I add for re-test your patch again. This patch still apply to the current project, I just test it out on my local environment.

Berdir’s picture

Status: Needs review » Needs work
+++ b/token.tokens.inc
@@ -1253,6 +1253,11 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
+    // Get the entity in the chosen translation.
+    if ($langcode && $entity->hasTranslation($langcode)) {
+      $entity = $entity->getTranslation($langcode);
+    }

I think we should use \Drupal::service('entity.repository')->getTranslationFromContext($entity, $langcode) instead?

What that basically does is figure out fallbacks, so maybe you don't have a german translation but a french one and then site is configured/implemented to prefer the french version over the english one as fallback.

darol100’s picture

Re-rolling this patch base on #10.

darol100’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: 2497247-Field-tokens-in-correct-language-11.patch, failed testing.

darol100’s picture

@Berdir, Any idea why my patch is failing ?

Berdir’s picture

Issue tags: +Needs tests

Yeah, the tests were broken, had nothing to do with this.

Would be great to get test coverage here, however. We currently have any field token tests, but we need to start somewhere :)

+++ b/token.tokens.inc
@@ -1256,6 +1256,11 @@ function field_tokens($type, $tokens, array $data = array(), array $options = ar
+    // Get the entity in the chosen translation.
+    if ($langcode && $entity->hasTranslation($langcode)) {
+      $entity = \Drupal::service('entity.repository')->getTranslationFromContext($entity, $langcode);
+    }

You don't need hasTranslation(), that defeats the purpose of getTranslationFromContext(). Which is to use the best language available, considering fallbacks.

darol100’s picture

@Berdir,

So it should looks like this ?

    if ($langcode) {
      $entity = \Drupal::service('entity.repository')->getTranslationFromContext($entity, $langcode);
    }
Berdir’s picture

Yes.

darol100’s picture

Here is a patch with the fixes base on #17. Sorry for the late response.

Berdir’s picture

Status: Needs review » Needs work

Hm, indentation seems be wrong now and we lost the comment, which I think was useful?

juampynr’s picture

@Maouna, how can I reproduce this issue? I tried the following:

1. Added a text field to the article content type.
2. Added a Pathauto pattern for a language that contains the field as a token.
3. Addded a translation for a node for that language.
4. Recreated aliases > the generated alias was correct.

Berdir’s picture

See #2648252: entity reference field token does not get translated, I asked the author there to update this issue here. I'm not sure yet if the patch here will help or not, IMHO, it shouldn't be necessary in the case of pathauto as we already pass in the entity in the right language.

That's a problem we only fixed recently in core: #2584745: Entity references should be displayed translated on non translatable entity types. Looking at the code, it *should* work nicely, as we should pass the language of the parent entity along and it should do its best to get matching translations.

We need a test for a scenario like that to be able to reproduce it cleanly.

We should have basic field test coverage soon with #2646316: Add tests for field tokens, we'll have to extend that to a multilingual environment to replace that.

yobottehg’s picture

I'll report this here also because it belongs here and not in : https://www.drupal.org/node/2648252

First of all this is in combination with pathauto latest dev release from github. After speaking with berdir there i think it's a token issue and i'm not doing something wrong.

Steps to reproduce:

  • Enable latest ctools, token and pathauto
  • create a taxonomy vocabulary which is translatable, create some terms and translate them
  • Create a content type with an entity reference field (field_category) to the taxonomy vocabulary, the field should be for all languages so you don't have to choose a category for each language
  • Create a pattern for the content type in pathauto for all languages: [node:field_category]/[node:title]
  • Generate the aliases for the content type

path auto generates the following:

german_field_category/german_title
fr/german_field_category/french_title

Summary:

Token does not look for translations of not translatable entity reference fields.

And i'm closing the other issue.

yobottehg’s picture

Issue summary: View changes
juampynr’s picture

Issue summary: View changes
Status: Needs work » Active
FileSize
24.3 KB
66.63 KB
54.93 KB
30 KB
44.95 KB

@yobottehg, I can reproduce the issue but I think that the behavior is correct. The culprit is not activating the "Users may translate this field" checkbox in the entity reference field that points to the taxonomy term. If you activate it, then patterns are created as expected.

Here is a detailed walk through of your steps:

Enable latest ctools, token and pathauto

Done.

create a taxonomy vocabulary which is translatable, create some terms and translate them

I created this term and translated it:

Create a content type with an entity reference field (field_category) to the taxonomy vocabulary, the field should be for all languages so you don't have to choose a category for each language:

Create a pattern for the content type in pathauto for all languages: [node:field_category]/[node:title]

Generate the aliases for the content type
path auto generates the following:

And here is the gotcha: if you edit the entity reference field at the content type and activate Users may translate this field
, then you will get the desired results after re-generating aliases:

I admit that it is a tricky scenario, but I don't think that this is a bug. If you want to use translated references, then you need to activate it in the entity reference field. Am I missing something else @yobottehg?

Berdir’s picture

It should work in both cases.

It's the *reference* that is translatable or not (Translate in this context really means "different values per language". The value of a reference field is just the ID). A translatable reference field means you essentially have different categories/references per language (or the possibility to have that). The more common case with fields like that is that you always want to specify the same category, but have the translated term name shown according to the language of the current entity.

That was broken in core as well but is now explicitly supported and tested since #2584745: Entity references should be displayed translated on non translatable entity types (Well, it worked before too, but that fixed the regression that broke it).

If you set up a node like that and look at it, will also display the correctly translated term name, even if the field is untranslatable. Tokens should work the same way.

juampynr’s picture

Thanks @Berdir. I will start debugging it and see if I can reproduce it in a test.

yobottehg’s picture

@berdir, @juampynr i started writing a test for my scenario and also basic other multilingual Tests:

https://gist.github.com/yobottehg/fafb0e5ba2a26eff8aac

I can't get the field_category tests to work "Token for [node:field_category] was not generated", the title translation tests works so, i think the entity reference does not get added to the node. could you help me here ? It's my first Drupal Test ... ;)

Thanks

juampynr’s picture

Status: Active » Needs work
FileSize
1.12 KB

@Berdir, I can't force Drupal to load the field translation. I will continue debugging but this is what I have got so far. Am I going in the right direction? The thing is: even though we get the node translation, the field is not translatable so Drupal finds this out and does not bother to search for a translation. This is why I am trying to fool the system by setting the field translation settings myself.

@yobottehg, I will try your test locally and see if I can help.

Berdir’s picture

I'm not sure. Based on looking through the code, I'd expect it to work. If you can provide a patch for the test that @yobottehg started, then I can look into it when I have time.

hussainweb’s picture

@yobottehg:

Thanks for the test. It always helps to post a patch which is easy to review and comment. Anyway, I posted your test as a patch here - https://www.drupal.org/files/issues/field_tokens_in_correct-2497247-27.p...

The test does not work and gives the error you describe. It took me a while to figure it out but I got some parts of it working. The changes are in the interdiff and the final patch is at https://www.drupal.org/files/issues/field_tokens_in_correct-2497247-30.p...

The failure now is that it is loading the French node instead of the original node. I am not sure if that is the bug we are discussing here or something else which is wrong with the test. It was quite tricky to get permissions working (or even find out that the permissions were part of the problem here).

The reason you saw the message was you were simply testing the wrong file name. There were some other typos that fixed other issues as well.

I think that the remaining issue with incorrect translation could be to do with rebuilding the container. I have no idea how to do this in KernelTestBase. Maybe @Berdir or @dawehner know better.

hussainweb’s picture

FYI, if the baseLanguage is "en", the first test passes. The second test fails with the error "Token value for [node:field_category] was not generated."

I think the problem with the first test failure is to do with how language is set up in the test. I suppose the container should be rebuilt somehow after the language is added. I found some instances of $langcode being NULL and eventually being set to 'en' earlier while debugging.

I will put in the patch from #28 to see what happens.

hussainweb’s picture

I combined the patch but still doesn't work (not even the first test). I also tried passing in langcode like this:

    $this->assertTokens("node",['node' => $this->article], $tokens, ['langcode' => 'de']);

The last submitted patch, 30: field_tokens_in_correct-2497247-27.patch, failed testing.

Berdir’s picture

Having a look at this. The second patch in in #30 actually passed the tests? Maybe a difference in how phpunit and simpletest call tests?

Berdir’s picture

Ok. The scary exception that happens that forces you to set protected $runTestInSeparateProcess = FALSE; is because it serializes the whole stacktrace apparently, that contains an entity, that entity then tries to access the container when being unserialized, which doesn' exist. And boom.

It does fail for me locally though, with phpunit. Investigating.

dawehner’s picture

FileSize
670 bytes

IMHO we should patch core with this. I tried that at some point but alex denied it.

Berdir’s picture

Thanks @dawehner. Yeah, I think that makes sense. Maybe we can limit it to only do that if we're in PHPUnit context and otherwise fail hard? Or something like that.

This is a lot easier to debug when you apply that and remove the protected $runTestInSeparateProcess = FALSE;.

yobottehg’s picture

The Test fails for me too locally with PHPUnit with this error:

Fatal error: Cannot redeclare t()

Which is ... kinda strange

Berdir’s picture

This works for me. Relevant changes:

1. Change base lang to en. You can't use a language that doesn't exist in the system.
2. translatable => TRUE on vocabulary doesn't exist. Instead, you need to make the node type and possibly vocabulary (didn't check if that's really required but it makes sense to do it) translatable with the content_translation service. This is the crucial part to make EntityViewDisplay::buildMultiple() work. See this part:

          // The language of the field values to display is already determined
          // in the incoming $entity. The formatter should build its output of
          // those values using:
          // - the entity language if the entity is translatable,
          // - the current "content language" otherwise.
          if ($entity instanceof TranslatableInterface && $entity->isTranslatable()) {
            $view_langcode = $entity->language()->getId();
          }
          else {
            $view_langcode = NULL;
          }

Maybe that is the reason it doesn't work for @yobottehg? Is your node type translatable? If not, then that's why. If you think that behavior is wrong, then you need to open a core issue. I'm not sure yet, but I think I agree. If you have a french node, then you always want the french term label, even if you don't have translations.

3. When calling addTranslation(), i had to pass along a value for the category field too. Not exactly sure why that was necessary but that's why you did not get a token for it.

I've also added another assertion now that uses the langcode option. That should work in the same way and the patch above should fix that (and only that), not tested yet. Commented out so we can see it passing. Note that you need the core patch to debug this in a useful way. It works when it passes but explodes when not.

General feedback on the test
* We could try to share some code with the existing field test. maybe a common base class, some methods on the trait or something like that.
* Coding standards (e.g. spaces around .)
* I don't really like the use of so many variables/properties. It makes the test way long, especially with all of them being documented. And harder to understand. You always have to look up the variable names to see their value and understand what is exactly compared/expected. I'd just hardcode most of them.
* I'd move all objects that you explicitly use like the term/node to the test method.
* While working on this, it would be great to also have a normal e.g. text field that actually is translatable covered and make sure that works too.
* The permission is required, we can't do anything about that. Real installations have that by default.
* That we have to configure the view display should be avoidable. token actually tries that but the code for that is outdated. See token_field_info_alter(): "'taxonomy_term_reference' => 'taxonomy_term_reference_plain',". The problem is that neither that field type nor that formatter still exists. It's controlled through a setting now. Which means we need to introduce something like default_token_formatter_settings too on that level. We can commit it for now, with a follow-up. In that, we should check all those field types and formatters and make sure that we have all field types in core covered with good defaults.

juampynr’s picture

Maybe that is the reason it doesn't work for @yobottehg? Is your node type translatable? If not, then that's why. If you think that behavior is wrong, then you need to open a core issue. I'm not sure yet, but I think I agree. If you have a french node, then you always want the french term label, even if you don't have translations.

The node type that I played with at #24 was translatable and the node I tested had a translation.

hussainweb’s picture

@Berdir, on #35, yes, that's right. I didn't realize that was why simpletest returned a weird error. I thought simpletest couldn't run these tests at all and did not even think of setting it to FALSE for debugging until after I fixed the test. I had to resort to var_dump debugging and it took hours.

* Coding standards (e.g. spaces around .)
* I don't really like the use of so many variables/properties.

Agree. I just wanted to get it working and also help @yobottehg see the fixed test with minimal changes. Once we are moving ahead, I definitely intend to clean this one.

* The permission is required, we can't do anything about that. Real installations have that by default.

True. I just mentioned it as one of the things I found and was required in this test.

* That we have to configure the view display should be avoidable. token actually tries that but the code for that is outdated.

Agree on follow-up. In fact, I was wondering if we couldn't improve the whole token.tokens.inc file and hooks in it. Maybe a plugin system. That requires a different issue for an indepth discussion but I am just throwing it out there.

Also, agreed on all other points. I will test the changes in the test later today and see if it works now. Of course, this is just the test mirroring a real world failure and I was expecting it to fail but not seeing the correct failure yet. Also, I don't think we explicit set 'Users may translate this field' in the test. If the field is not translatable, addTranslation should not require that particular field, right?

hussainweb’s picture

Just a small note: the second patch in #30 passed all tests because somehow, MultilingualTest was never run. There are only 39 passes when there should be 40. I verified this with the output of run tests. I am not sure why it didn't run.

hussainweb’s picture

I am removing many of the properties that are not needed on the class and moving term and node creation to the actual test method. I am also fixing the code formatting issues.

I am considering sharing helper methods with FieldTest but there is nothing outside Entity::create calls we do. We could create helpers to do that but then there are a lot of other test which could use that and it would bloat the patch here unnecessarily. I think a follow-up is best here. Did you have any other code in mind that could be shared?

I am thinking of removing properties like $nodeType as it is just simpler to use it as literal in the code. I understand that title and others are variables because it helps in asserts later but node type, vocab name, etc... are okay to be literals. Any thoughts?

I will work on adding a text field which is translatable.

yobottehg’s picture

@berdir
Yes the node type was translatable and thanks for all the feedback, i'll take account of it for the future tests.

@hussainweb @juampynr
thanks for the refactoring and all the help

Just one question because i don't know if i'm getting it right.
The Test should now show my usecase and the test passes but it shouldn't? I mean for pathauto this does not work in practice.

is this how token works ?

$this->assertTokens("node", ['node' => $article->getTranslation($this->translateToLangcode)], $tokens);

Or this ?

$this->assertTokens("node",['node' => $this->article], $tokens, ['langcode' => $this->translateToLangcode]);

Because the second assert fails with unable to generate node:title.

Also i don't get why i should pass the same Term to the Translation if the field is not translatable, is this the case for all Entity Reference fields ?

I should mention that the initial code of the issue is missing in the latest patch.

thanks for clarification and sorry for all the questions

juampynr’s picture

@yobottehg, the function signature of assertTokens is the following one:

  function assertTokens($type, array $data, array $tokens, array $options = array()) {

So the second of your code snippets is the right one. I suggest that you have a look at what the function does in order to figure out why that token is not being generated. You will see that what it does is to call token::generate() and inspect the result. In order to replicate this out of a testing environment, I sometimes write a small script like this one to play with. Then, I set a breakpoint and inspect the result.

Berdir’s picture

Strange. I thought I commented already.

* Yes, this test should demonstrate your use case. I'm not sur why it doesn't work for you if it's not the translation configuration. You are on 8.0.0+, correct? This issue was fixed in october.
* Both calls should work, that's what the patch up in #18 should fix. So enable that assertion and combine it with the patch from #18. That's what I meant with this above:

I've also added another assertion now that uses the langcode option. That should work in the same way and the patch above should fix that (and only that), not tested yet.

* I'm not sure why exactly the value is needed for addTranslation(), but when creating a translation in the UI, it basically does the same. And not providing it results in no token value at all, which is also not the same problem as you have.

Berdir’s picture

Status: Needs review » Postponed (maintainer needs more info)

I totally forgot about this issue, I think #2772673: Provide tokens for referenced entities in the active language. provides a lot of the test coverage that we wanted to add here, so we might be able to close this as a duplicate.

Bambell’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)

I carefully verified and, yes, the test provided in this patch is entirely contained in #2772673: Provide tokens for referenced entities in the active language.. Both tests create a term, translate it, create a node with an entity reference field and assign the original term to it, translate the node and assign the translated term and verify the node title and field tokens, for both the original and translated node. #2772673: Provide tokens for referenced entities in the active language. also tests additional tokens. Closing as duplicate.