I came I came across the issue Product label translations show up in incorrect language on a Drupal commerce site.

I tracked down the problem to the function field_info_extra_fields() and the FieldInfo class.
The problem is that the method FieldInfo::getBundleExtraFields() calls $extra = module_invoke_all('field_extra_fields'); and in turn hook_field_extra_fields() is called returning localized data however.
The extra field info is then cached into the cache_field table as 'field_info:bundle_extra': cache_set("field_info:bundle_extra:$entity_type:$bundle", $info, 'cache_field');.

This leads to one language getting stuck in cache and showing for all other languages

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

guy_schneerson’s picture

How I recreated the issue:

  • Install a clean Kickstart 1
  • Change the display of the product display to show the "product:sku" field
  • Enable the locale module
  • Add french to your languages and import the commerce french translation file (it adds translation to "SKU")
  • now view a product node page and change languages it will get stuck on french
guy_schneerson’s picture

Status: Active » Needs review
FileSize
1 KB

attached patch for above

swentel’s picture

Version: 7.x-dev » 8.x-dev

Should be 8.x first, and in case that's ok, we can move this into #1040790: _field_info_collate_fields() memory usage

guy_schneerson’s picture

Status: Needs review » Needs work

thanks @swentel
Posted a link on the related issue, thanks for pointing it out as my patch will have no relevance as needs to be moved into the #1040790 or rewritten after it is committed.
Marking it as needs work for now

podarok’s picture

Issue tags: +D8MI

tagging

czigor’s picture

Status: Needs work » Needs review

The patch in #2 works for me on D7. Thanks!

yched’s picture

Patch for D8.

yched’s picture

Issue tags: +Needs backport to D7

Add "needs backport" tag

Berdir’s picture

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

That said, this will probably not get in without test coverage.

YesCT’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Needs work

I've set it to needs work because it needs tests :)

Berdir’s picture

To write tests for this, set up two languages, add a translation for e.g. one of the strings in user_field_extra_fields() then vist the manage fields page for those two languages, it should not display the correct translation for the second language.

If you know a bit about testing, then this should be a relatively easy test to write.

sidharthap’s picture

FileSize
79.5 KB
24.32 KB

I apply this patch on my D8. After applying the patch i can not see the translated language. I tried the steps mentioned #14 to replicate. Please correct me if i am doing something wrong here. Here is my attached screen.

1810178-15-1
1810178-15-1

Berdir’s picture

That is not the same string, there's a "User name and Password" once and the other is "Username and Password"

sidharthap’s picture

Yes , It works. thanks @Berdir.

sidharthap’s picture

@Berdir, I am in the middle of it. This needs a fully functional test. Please comment on this.
1 - Is it required to write test to check the exact filed User name and password OR we can take a random field name.
2 - The module already has a translation test file, can we use that file to write test for this function getBundleExtraFields() ?
3 - If we are going write the test in FieldInfoTest file, should we follow below steps to cover:
1 - looged in as Admin.
2 - Check languages if not activated, Activate one language (Considering Default language is there)
3 - Translate one specific string (User name and password ) for account.
5 - Navigate to "admin/config/people/accounts/fields" in both the languages and check the expected string.

Berdir’s picture

You need to use an extra field, so you can't create a random field but use something that has been defined by a module. So either implement that hook in field_test.module or use an existing one.

You don't need to check languages, this is a test, it will always only have the default language by default, so just create a second language. But otherwise, those test steps look good to me. Note that a test that involves the UI should be in field_ui.module, not field.module. Maybe it's also possible to test this by directly calling the API functions and switching global $language, I'm not sure.

sidharthap’s picture

I tried to create a initial patch what i did in browser. Please correct me if i am wrong, This is my first test code for drupal8.

Thank you

Berdir’s picture

Status: Needs work » Needs review

Remember to set an issue to needs review if you upload a patch.

Berdir’s picture

Test looks nice! Can you also upload a test-only patch so that it fails and we have prove that the test covers this bug?

I suggest you use the API to add a language, see e.g. Drupal\block\Tests\BlockLanguageCache::setUp() for an example and use a dummy language code that doesn't actually exist. Going through the UI currently tries to download translations from d.o.

- Same for the translation, as nice as that looks, I would suggest to use a dummy string with only ascii characters to avoid unecessary problems with weird non-utf8 operating systems/editors. Also, instead of $string1 and 2, name them something like $en_string and $translated_string or something like that.

Status: Needs review » Needs work

The last submitted patch, extra-fields-localized-1810178-19.patch, failed testing.

sidharthap’s picture

Thanks @berdir,
i can use a dummy string, language add through API but i am not sure how can i test exact tranlated string in the page. mean in my previous patch, i am navigating to manage field section (hi/admin/config/people/accounts/fields) to test that hindi translated string exist on the page.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
2.02 KB

Thanks @berdir,
Here is a new test only patch. is it looks good ?

Berdir’s picture

+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.phpundefined
@@ -106,4 +106,36 @@ private function checkTranslationRevisions($eid, $evid, $available_langcodes) {
+      'source' => $this->randomName(16),

The source shouldn't be random, this should be the actual string that we want to translate.

+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.phpundefined
@@ -106,4 +106,36 @@ private function checkTranslationRevisions($eid, $evid, $available_langcodes) {
+    // Submit the translations.
+    $textarea = current($this->xpath('//textarea'));
+    $lid = (string) $textarea[0]['name'];
+    $edit = array(
+      $lid => $translation->getString(),
+    );
+    $this->drupalPost('admin/config/regional/translate/translate', $edit, t('Save translations'));

Given that we're using the API now, why still use the UI to add the translation?

Status: Needs review » Needs work

The last submitted patch, extra-fields-localized-test-only-1810178-25.patch, failed testing.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
2.08 KB

Updated patch file.

sidharthap’s picture

Thank you @Berdir
Corrected source and translated string.

Berdir’s picture

Ok, getting there :)

+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.phpundefined
+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.phpundefined
@@ -19,7 +19,7 @@ class TranslationWebTest extends FieldTestBase {

@@ -19,7 +19,7 @@ class TranslationWebTest extends FieldTestBase {
-  public static $modules = array('language', 'field_test');
+  public static $modules = array('language', 'field_test', 'locale');

You need field_ui module for this test, which means that your function should be in a test class within field_ui/lib/Drupal/field_ui/Tests, not the field module.

+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.phpundefined
@@ -106,4 +106,29 @@ private function checkTranslationRevisions($eid, $evid, $available_langcodes) {
+   * Test that _field_info_collate_fields() is not localized
+   */
+  function testFieldTranslation() {

TestS and we *are* testing that its translated. We just need a failing test-only patch together with the patch that works to make sure that the test covers the bug.

Also, the method should be named testExtraFieldsTranslation() or something like that.

+++ b/core/modules/field/lib/Drupal/field/Tests/TranslationWebTest.phpundefined
@@ -106,4 +106,29 @@ private function checkTranslationRevisions($eid, $evid, $available_langcodes) {
+    $this->drupalGet('xx/admin/config/people/accounts/fields');
+    $this->assertText($translation->getString(), 'Translated string found.');
+    $this->drupalGet('admin/config/people/accounts/fields');
+    $this->assertText($en_string->getString(), 'English string found.');

This now returns an access denied page if you look at it when you run the test locally.

That's because that page needs the 'administer user fields' permission. I don't think it needs those other permissions that you currently defined for the user.

Just start doing those changes (move to field_ui, correct permissions) and look at the debug output from the test, there you can see what is returned.

Status: Needs review » Needs work

The last submitted patch, extra-fields-localized-test-only-1810178-29.patch, failed testing.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
2.06 KB

Thank you @berdir
The test function is now moved to Manage fields (Drupal\field_ui\Tests\ManageFieldsTest) test. still it fails the simple test in local. before making changes i ran test for this and found 132 passes, 170 fails, 61 exceptions, and 40 debug messages. The test fails first at user logged in.

swentel’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.phpundefined
@@ -472,5 +479,30 @@ function testDeleteTaxonomyField() {
+    $admin_user = $this->drupalCreateUser(array('administer user display', 'access administration pages'));

This should be 'administer user fields'

Berdir’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.phpundefined
@@ -6,11 +6,18 @@
+  public static $modules = array('language', 'locale');
   public static function getInfo() {

Should have a empty line between these two lines.

As @swentel said, the permission name should be fields, not display.

I think those local failures are caused by a misconfiguration of your environment, let's see what the testbot has to say. Should have two fails because of the wrong permission.

Status: Needs review » Needs work

The last submitted patch, extra-fields-localized-test-only-1810178-32.patch, failed testing.

Berdir’s picture

Strange, not sure why those assertions have failed twice. But yes, if you fix that permission name then it should work in that it correctly identifies the fail.

What you need to do then is upload two patches, one with a --test-only.patch suffix that shows "This test without the fix is red" and another patch that contains both the fix and the test to show that it is green and actually commit.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
3.62 KB

Here is the two files.

Status: Needs review » Needs work

The last submitted patch, extra-fields-localized-test-only-1810178-37.patch, failed testing.

sidharthap’s picture

still it is failing near line 506 "Translated string found". @berdir - is it reuired any other role?

Berdir’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.phpundefined
@@ -7,10 +7,19 @@
+  public static $modules = array('language', 'locale');

@@ -472,5 +481,31 @@ function testDeleteTaxonomyField() {
   }

Trailing spaces.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.phpundefined
@@ -472,5 +481,31 @@ function testDeleteTaxonomyField() {
+    $en_string = locale_storage()->createString(array(
+      'source' => 'User name and password',
+      'context' => $this->randomName(10),
+    ))->save();

Context needs to be an empty string otherwise the translation would only be used if that context is specified.

sidharthap’s picture

Corrected the cotext to empty string and removed the trailing spaces as mentioned in #40

Status: Needs review » Needs work

The last submitted patch, extra-fields-localized-1810178-41.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

@sidharthap Looks like progress is going well.

You will find that it's nice to upload the tests only patch first (which will fail) and then second the complete actual patch (which hopefully will pass).

That way the testbot will leave the issue set at needs review.

Berdir’s picture

Status: Needs review » Needs work

If you look closer, that is what he did :)

Interestingly, the test only patch passed, but the one with the fix did not. So something is... wrong. I'll try to look into it.

YesCT’s picture

Oh! sorry. :)

Berdir’s picture

Ok, there are two problems here.

1) The test uses randomString(), which includes special characters like > and < which are escaped in HTML and cause assertText() to fail. That's the reason it randomly fails if there is one of those in the generated string.

2) Apart from that however is the test correctly passing because it actually works right now in HEAD. The reason it's working is because of a bug in the cache usage of the FieldInfo class, which causes it to rebuild on every page load and therefore ignore written cache anyway. Opened #1971418: Wrong use of cache tags in FieldInfo class as that is a separate, major bug that needs to be fixed first.

Once that referenced issue is fixed, you can re-roll this with using randomName() instead of randomString() and it will fail/pass as expected.

Berdir’s picture

Ok, I was wrong with that issue.

The manage fields page always forces a cache clear, that's why the bug is not visible there, changing the cache tags just broke that ;)

Which means we need a different way to test this here. Possibly just use the API functions directly and change global $language. Which would mean that it could go back to a field.module test. Sorry for the confusion :(

jair’s picture

Issue tags: +Needs reroll

Needs reroll

Anonymous’s picture

Now a reroll, still relevant #dcprague

durifal’s picture

Assigned: Unassigned » durifal

working on it

durifal’s picture

Assigned: durifal » Unassigned
Status: Needs work » Needs review
FileSize
3.81 KB

Re-rolled

Status: Needs review » Needs work

The last submitted patch, extra-fields-localized-1810178-51.patch, failed testing.

durifal’s picture

Assigned: Unassigned » durifal

working on it

yched’s picture

Assigned: durifal » Unassigned

Thanks for reviving this !

  1. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
    @@ -13,7 +13,14 @@
     class ManageFieldsTest extends FieldUiTestBase {
    -  public static function getInfo() {
    +  /**
    

    Should have a whiteline betwen the class statement and the 1st phpdoc (already wrong in current HEAD, but let's fix it while we're in there)

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
    @@ -499,6 +506,32 @@ function testDeleteTaxonomyField() {
    +   * Test that _field_info_collate_fields() is not localized
    

    That's awkward, _field_info_collate_fields() doesn't exist anymore :-)

  3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
    @@ -499,6 +506,32 @@ function testDeleteTaxonomyField() {
    +  function testFieldTranslation() {
    

    This test has nothing to do in field_ui/ManageFieldsTest, the behavior we are testing is not provided by field_ui.

    \Drupal\field\Tests\FieldInfoTest looks like a better place for this.

  4. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
    @@ -499,6 +506,32 @@ function testDeleteTaxonomyField() {
    +     //check translated strings are avialable in pages.
    

    - Misses a whitespace and capital letter after //
    - Language: "Check *that* translated strings are..."
    - typo "avialable"

  5. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
    @@ -499,6 +506,32 @@ function testDeleteTaxonomyField() {
    +     //check translated strings are avialable in pages.
    +     $this->drupalGet('xx/admin/config/people/accounts/fields');
    +     $this->assertText($translation->getString(), 'Translated string found.');
    +     $this->drupalGet('admin/config/people/accounts/fields');
    +     $this->assertText($en_string->getString(), 'English string found.');
    

    We don't need to test this through the UI (and thus we shouldn't to avoid adding unneeded burden on the test suite).
    Kust call FieldInfo:: getBundleExtraFields() and check that the 'label' is the expected translated string.

David Hernández’s picture

Assigned: Unassigned » David Hernández

Working on this.

David Hernández’s picture

Assigned: David Hernández » Unassigned
Status: Needs work » Needs review
FileSize
4.49 KB

Ok, here it's the new patch. I've moved the test to the test file recommended by @Yched at #54. This caused several problems, as it is an unit test case and the original test was a web test case. I think I've updated it correctly, but will be better if someone with more experience in testing reviews it.

There were also some deprecated functions and classes. I think I've fixed all of them.

mcrittenden’s picture

Issue tags: -Needs reroll

Tags

swentel’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
  1. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php
    @@ -327,4 +329,33 @@ function testWidgetDefinition() {
    +    $this->assertNotNull($translation[0]->translation, 'Translated string found.');
    +    $this->assertNotNull($label, 'User name and password');
    +  }
    

    I would check on the $this->randomString() you are inserting above, we're still not sure if this is an actual translation.

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
    @@ -13,6 +13,7 @@
    +
    

    Can be removed

Rajesh Ashok’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.29 KB

Here is the reroll for patch in #56

Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2014

Add sprint weekend tag.

Status: Needs review » Needs work

The last submitted patch, 59: extra-fields-localized-reroll-1810178-59.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
4.3 KB
3.55 KB

Rerolled

I changed the test to actually call extraBundleFields and compare the generated translation, however I'm struggling to 'trick' DUBT tests to change the interface language.
Anyone knows how to trick that ?

Status: Needs review » Needs work

The last submitted patch, 62: extra-fields-localized-1810178-62.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
3.55 KB

Nevermind, got some hints from berdir on IRC - this works fine now.

Status: Needs review » Needs work

The last submitted patch, 64: extra-fields-localized-1810178-64.patch, failed testing.

swentel’s picture

Totally wrong patch, one second

swentel’s picture

Status: Needs work » Needs review
FileSize
5.97 KB
4.52 KB

And now with injection too

alexkb’s picture

As a 8.x version of Commerce/Kickstart doesn't exist, could we please get instructions for manually reproducing the bug? I tried what seemed obvious - enabling Content Translation + Language modules, and then enabled the translation setting on the page content type (and fields), enabled the french translation, created some test content in both languages, but could still see both fields updating without applying the patch. #drupalsouth

swentel’s picture

Concentrate on a field that is registred as an extra field. If you would translate the label in the translate interface, it should be cached and switching the language in either field ui or on the page where it's printed should give the same label. The test is using the 'Username and password' label on the user register screen (and also fails without the patch) so that is the fastest way to reproduce.

Gábor Hojtsy’s picture

Title: _field_info_collate_fields() is not localized » _field_info_collate_fields() is not language-aware, may return wrong values
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community
Issue tags: +language-config

Elevating to major bug. The patch looks good. If field information is language dependent, it makes total sense to cache it per language :) Test looks good too!

Xano’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.96 KB

Re-roll.

Reject:

diff a/core/modules/field/lib/Drupal/field/FieldInfo.php b/core/modules/field/lib/Drupal/field/FieldInfo.php	(rejected hunks)
@@ -12,8 +12,7 @@
 use Drupal\Core\Config\ConfigFactory;
 use Drupal\Core\Field\FieldTypePluginManager;
 use Drupal\Core\Extension\ModuleHandlerInterface;
-use Drupal\field\FieldInterface;
-use Drupal\field\FieldInstanceInterface;
+use Drupal\Core\Language\LanguageManagerInterface;
 
 /**
  * Provides field and instance definitions for the current runtime environment.
@@ -136,12 +142,15 @@ class FieldInfo {
    *   The module handler class to use for invoking hooks.
    * @param \Drupal\Core\Field\FieldTypePluginManager $field_type_manager
    *   The 'field type' plugin manager.
+   * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
+   *   The language manager to use.
    */
-  public function __construct(CacheBackendInterface $cache_backend, ConfigFactory $config, ModuleHandlerInterface $module_handler, FieldTypePluginManager $field_type_manager) {
+  public function __construct(CacheBackendInterface $cache_backend, ConfigFactory $config, ModuleHandlerInterface $module_handler, FieldTypePluginManager $field_type_manager, LanguageManagerInterface $language_manager) {
     $this->cacheBackend = $cache_backend;
     $this->moduleHandler = $module_handler;
     $this->config = $config;
     $this->fieldTypeManager = $field_type_manager;
+    $this->languageManager = $language_manager;
   }
 
   /**

Xano’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, under the condition that the tests pass.

catch’s picture

Assigned: Unassigned » yched

This directly conflicts with #2073297: Remove cache of Field / FieldInstance objects to the point where I'd be tempted to bump that one to major for 8.x, and move this to 7.x directly. Assigning to yched to see what he thinks.

yched’s picture

Assigned: yched » Unassigned

#2073297: Remove cache of Field / FieldInstance objects is actually probably a duplicate, or at least a subtask of #2116363: Unified repository of field definitions (cache + API), which is not ready yet.
I'll mark it as postponed.

Meanwhile, we can safely commit this one (which is only about the registry of "extra fields", so it's pretty much isolated from the field registry now)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

OK that works for me too :)

Committed/pushed to 8.x, thanks! Moving to 7.x, for backport.

paranojik’s picture

Title: _field_info_collate_fields() is not language-aware, may return wrong values » field_info_extra_fields() is not language-aware, may return wrong values
Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
2.15 KB

Rerolled for D7. No test yet.

  • catch committed 91542ec on 8.3.x
    Issue #1810178 by sidharthap, swentel, Xano, Rajesh Ashok, David...

  • catch committed 91542ec on 8.3.x
    Issue #1810178 by sidharthap, swentel, Xano, Rajesh Ashok, David...

  • catch committed 91542ec on 8.4.x
    Issue #1810178 by sidharthap, swentel, Xano, Rajesh Ashok, David...

  • catch committed 91542ec on 8.4.x
    Issue #1810178 by sidharthap, swentel, Xano, Rajesh Ashok, David...