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
Comment | File | Size | Author |
---|---|---|---|
#76 | extra-fields-localized-1810178-76-D7.patch | 2.15 KB | paranojik |
#71 | drupal_1810178_71.patch | 5.96 KB | Xano |
#62 | extra-fields-localized-1810178-62.patch | 4.3 KB | swentel |
#59 | extra-fields-localized-reroll-1810178-59.patch | 4.29 KB | Rajesh Ashok |
#56 | extra-fields-localized-1810178-56.patch | 4.49 KB | David Hernández |
Comments
Comment #1
guy_schneerson CreditAttribution: guy_schneerson commentedHow I recreated the issue:
Comment #2
guy_schneerson CreditAttribution: guy_schneerson commentedattached patch for above
Comment #3
swentel CreditAttribution: swentel commentedShould be 8.x first, and in case that's ok, we can move this into #1040790: _field_info_collate_fields() memory usage
Comment #4
guy_schneerson CreditAttribution: guy_schneerson commentedthanks @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
Comment #5
podaroktagging
Comment #6
czigor CreditAttribution: czigor commentedThe patch in #2 works for me on D7. Thanks!
Comment #7
yched CreditAttribution: yched commentedPatch for D8.
Comment #8
yched CreditAttribution: yched commentedAdd "needs backport" tag
Comment #9
Berdir#7: extra-fields-localized-1810178-7.patch queued for re-testing.
Comment #10
BerdirThis looks good to me.
Comment #11
BerdirThat said, this will probably not get in without test coverage.
Comment #12
YesCT CreditAttribution: YesCT commented#7: extra-fields-localized-1810178-7.patch queued for re-testing.
Comment #13
BerdirI've set it to needs work because it needs tests :)
Comment #14
BerdirTo 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.
Comment #15
sidharthapI 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.
Comment #16
BerdirThat is not the same string, there's a "User name and Password" once and the other is "Username and Password"
Comment #17
sidharthapYes , It works. thanks @Berdir.
Comment #18
sidharthap@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.
Comment #19
BerdirYou 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.
Comment #20
sidharthapI 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
Comment #21
BerdirRemember to set an issue to needs review if you upload a patch.
Comment #22
BerdirTest 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.
Comment #24
sidharthapThanks @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.
Comment #25
sidharthapThanks @berdir,
Here is a new test only patch. is it looks good ?
Comment #26
BerdirThe source shouldn't be random, this should be the actual string that we want to translate.
Given that we're using the API now, why still use the UI to add the translation?
Comment #28
sidharthapUpdated patch file.
Comment #29
sidharthapThank you @Berdir
Corrected source and translated string.
Comment #30
BerdirOk, getting there :)
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.
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.
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.
Comment #32
sidharthapThank 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.
Comment #33
swentel CreditAttribution: swentel commentedThis should be 'administer user fields'
Comment #34
BerdirShould 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.
Comment #36
BerdirStrange, 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.
Comment #37
sidharthapHere is the two files.
Comment #39
sidharthapstill it is failing near line 506 "Translated string found". @berdir - is it reuired any other role?
Comment #40
BerdirTrailing spaces.
Context needs to be an empty string otherwise the translation would only be used if that context is specified.
Comment #41
sidharthapCorrected the cotext to empty string and removed the trailing spaces as mentioned in #40
Comment #43
YesCT CreditAttribution: YesCT commented@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.
Comment #44
BerdirIf 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.
Comment #45
YesCT CreditAttribution: YesCT commentedOh! sorry. :)
Comment #46
BerdirOk, 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.
Comment #47
BerdirOk, 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 :(
Comment #48
jair CreditAttribution: jair commentedNeeds reroll
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedNow a reroll, still relevant #dcprague
Comment #50
durifal CreditAttribution: durifal commentedworking on it
Comment #51
durifal CreditAttribution: durifal commentedRe-rolled
Comment #53
durifal CreditAttribution: durifal commentedworking on it
Comment #54
yched CreditAttribution: yched commentedThanks for reviving this !
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)
That's awkward, _field_info_collate_fields() doesn't exist anymore :-)
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.
- Misses a whitespace and capital letter after //
- Language: "Check *that* translated strings are..."
- typo "avialable"
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.
Comment #55
David Hernández CreditAttribution: David Hernández commentedWorking on this.
Comment #56
David Hernández CreditAttribution: David Hernández commentedOk, 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.
Comment #57
mcrittenden CreditAttribution: mcrittenden commentedTags
Comment #58
swentel CreditAttribution: swentel commentedI would check on the $this->randomString() you are inserting above, we're still not sure if this is an actual translation.
Can be removed
Comment #59
Rajesh Ashok CreditAttribution: Rajesh Ashok commentedHere is the reroll for patch in #56
Comment #60
Gábor HojtsyAdd sprint weekend tag.
Comment #62
swentel CreditAttribution: swentel commentedRerolled
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 ?
Comment #64
swentel CreditAttribution: swentel commentedNevermind, got some hints from berdir on IRC - this works fine now.
Comment #66
swentel CreditAttribution: swentel commentedTotally wrong patch, one second
Comment #67
swentel CreditAttribution: swentel commentedAnd now with injection too
Comment #68
alexkb CreditAttribution: alexkb commentedAs 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
Comment #69
swentel CreditAttribution: swentel commentedConcentrate 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.
Comment #70
Gábor HojtsyElevating 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!
Comment #71
XanoRe-roll.
Reject:
Comment #72
XanoRTBC, under the condition that the tests pass.
Comment #73
catchThis 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.
Comment #74
yched CreditAttribution: yched commented#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)
Comment #75
catchOK that works for me too :)
Committed/pushed to 8.x, thanks! Moving to 7.x, for backport.
Comment #76
paranojik CreditAttribution: paranojik at Cando commentedRerolled for D7. No test yet.