Currently comment support for field language is completely broken as LANGUAGE_NONE
is hardcoded in many many places when directly accessing the comment body. This makes working with translatable comments pretty broken. Moreover the comment form builder function passes the wrong language to field_attach_form()
, thus not respecting the comment language. AAMOF field widgets should always match the entity language, if defined, instead here we are always using the default language. This way if the comment body is made translatable its values will always be in the default langauge no matter which language the comment has.
Comments
Comment #1
plachThis needs to be fixed in D7 too. And will need a D7 > D7 update function :(
Comment #2
andypostSo what language should be used in comment fields and for comment entity itself?
Comment #4
webflo CreditAttribution: webflo commentedComment #5
plach@andypost:
Comments currently store the active content language, when the comment is initially being post, as comment language. This leaves room for intoducing an (optional) language selection widget (ET is likely to provide it in D7), see #1280996: New language_select element type for form API.
As usual translatable fields should follow the entity language, but here they are being created in the site default language. We need to use
entity_language()
as$langcode
parameter offield_attach_form()
in comment_form() (see http://drupal.org/node/1626346).For D7 we will have to write an update function fixing comment field languages.
Comment #6
cweagansFixing tags per http://drupal.org/node/1517250
Comment #7
slowflyer CreditAttribution: slowflyer commentedI spend my weekend to get a solution for this issue, at least for my Drupal projects.
The result is the patch attached.
It replaces the 3 times LANGUAGE_NONE with a function call.
The function tries to get the language of the comment using entity language function.
If the entity_translation module is not installed it falls back to the old LANGUAGE_NONE.
But the most important part is calling field_attach_form in line 2029 with language parameter:
- field_attach_form('comment', $comment, $form, $form_state);
+ field_attach_form('comment', $comment, $form, $form_state,isset($comment->language) ? $comment->language : '');
If the edit form falls back to site default language, edit forms for entity_translation will not work.
Comment #8
webflo CreditAttribution: webflo commentedComment #10
plachBugs are fixed in the development version first. We need to update tests but let's see if nothing breaks first.
Comment #11
andypostI think this require a tests
Comment #12
plachYes, working on that.
Comment #13
plachHere is a patch with updated tests and a test-only version that exposes the bug(s). While trying to make tests pass I found an unrelated bug in
field_language()
, which has been introduced by the Entity API refactorings. Since it's just a wrong copy/paste that is unlikely to be repeated and given that the attached tests actually cover it, I think a separate issue on which this would be postponed is unnecessary.The patch updates the existing comment language tests to check the correct behavior of the comment body field. As a bonus it moves the test class from the Locale namespace to the Comment namespace, where it belongs.
Comment #14
plachComment #15
andypostIt works!
Possibly this could introduce a performance regression. But this change is required
I think it makes sense to fix it here
Comment #16
plachWell, we will need to call
getFormLangcode()
at least once anyway and the result may be cached into$form_state['langcode']
. However this is going to be heavily refactored when porting #1188388: Entity translation UI in core to the new Property API.Let's just quick fix this as comment translation is broken in D7, right now.
Comment #17
plachThis is somehow "blocking" the work in #1188388: Entity translation UI in core, altough we can get around it. If any committer could have a look to this, it would be great...
Comment #18
catchI don't see this causing a performance regression, should be a few extra function calls and not a lot more.
This needs to be updated for the new path. A bit annoying that the @file duplicates like this.
Whoops, but the $id is only used for the static caching and it /looks/ like the new tests catch this.
Committed/pushed to 8.x to unblock the other issue.
Just needs a novice follow-up for the comment being updated before it moves back to 7.x
Comment #19
plachAnd here it is.
Comment #20
catchOK thanks. Committed/pushed the follow-up, moving to 7.x for backport.
Comment #21
plachOk and this was the easy part. The D7 version will be tougher as we need an API change:
field_attach_form()
should use the comment language and not the default language. And we will need an update function changing the language of comment fields to make it match the comment language, at least for original values.The only relieving aspect is that if nobody reported this issue before it's likely that no one ever tried to translate comments, which is a pretty edgy use-case I'd say. Hence the update function should be harmless in practice as most comment fields out there should be non translatable and thus have
LANGUAGE_NONE
.Comment #22
plachOn this.
Comment #23
plachThis was even more broken than I originally thought (see the test-only patch results). I cannot imagine anyone being able to use comments and field language together so far. One of the main problems was that language fallback was not enabled for comments, hence displaying a list of comments with different languages would result in an empty body for all the comments not matching the current language, provided that the body were translatable.
Moreover there was no support for comment field language change. Since field widgets always have
LANGUAGE_NONE
as initial value on new comments (the comment language is altered by Locale afterfield_attach_form()
is called), submitting translatable fields lead to a big bunch of errors on save.That said I don't think it's reasonable to assume there can be any site out there using field language on comments without reporting any error here. I'd be tempted to skip the update function altogether since it would be really hard to make it reliably alter field data. Basically it would need to look for all the field instances tied to every comment bundle, determine whether a field is translatable and change all language values from the site default (that might have changed meanwhile) to the comment language. Please note that the first two might involve hook invocations.
Comment #25
plachLet's try again.
Comment #27
plachNo errors here and the exception in the log appears to be totally unrelated to the changed code.
#25: drupal-comment_field_language-1534674-25.patch queued for re-testing.
Comment #28
plachAssigning to @David to get a feedback about whether it'd be wise to write an update function or not. Also @catch's feedback would be welcome. See #21 and #23.
Comment #29
plachThis is an almost straight port of what was committed in #18: tentatively marking RTBC to get some feedback by David.
Comment #30
plachMinor: should talk about comments.
Cannot reroll this now, to be fixed as soon as a new patch is rolled.
Comment #31
plachFixed #30. This one can help go back under thresholds and allow features to be committed to D8. A feedback would be more than welcome.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedOK, I looked at this, but comparing #13 to #31 it doesn't really look like a straightforward port at all; this area of the codebase is extremely different in Drupal 7 and Drupal 8 and large parts of the patches barely even look alike.
So, I don't feel comfortable being the only reviewer here (or have time, or am qualified :)... it would be really great to see someone else review the Drupal 7 patch also.
I did look at it, though, and overall it seemed pretty straightforward to me. I was somewhat confused by the area of the tests that are doing Preview followed by Save with the comment language switching in between. It wasn't clear to me if that was done to test a workflow that had a bug before (but which is fixed by the patch) or whether it was required for some other reason, but overall the whole thing with "Initially field form widgets have no language.... After the first submit the submitted entity language is taken into account" looked really odd.
As for the update function, I agree we probably don't need one. So is it correct that the worst thing that can happen (if someone actually was using comment field languages on a live site) is that their comments might stop displaying (due to the data being in an unexpected language), but the comment data itself will still be hanging around the field tables in the database? As long as there's no reason to expect actual data loss, I think that's something we could live with for now if we had to.
Comment #33
plachTotally OK for me, my main goal was to understand whether we needed to provide an update function or not. Obviously being this a port of an already committed patch there are chances that this can go in as-is. I wouldn't waste your time by marking it RTBC otherwise :)
Yes, this is exactly as it looks: it's testing also the preview because it was broken too.
This is going to change in D8, but in D7 we have no other ways: the field form element structure includes also the field language and when creating an entity we have no known language, hence we need to initally assign LANGUAGE_NONE and change it afterwards. This handles also a regular language change so it would be needed anyway.
Cool :)
No, the patch introduces language fallback for comments so data previosuly hidden might start showing up :) The real change is that before the patch when creating comments with translatable fields attached the latter always get the default language. With the patch they follow the comment language, which in turn inherits the page language when the comment is created. No risk of data loss, only a slightly different (more correct) behavior.
So it seems this may be ready to go in after all: @andypost (or anyone else feeling bold enough) can you confirm this is good to go?
Comment #34
andypostThis patch is really needed:
1) entity form for comment should use comment language or fallback to content language
2) comment should have ability to hook event of language change in form
3) preview was broken btw because the same hardcoded LANGUAGE_NONE
4) introduced locale_field_entity_form_submit() could be re-used for any entity
5) I agree that update is not needed because if some one has broken data they can see the data returns back
This function looks public, so maybe we should file change notice
Comment #35
plachThanks!
Agreed for the change notice. I guess it's David's turn again :)
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedOK, I just took this patch for a spin in combination with the latest version of the Entity Translation module. And as far as I can tell, it breaks things.
I set up a site with English and Spanish languages (determined via the URL), then enabled entity translation for nodes and comments and specifically for the comment body field.
Before the patch, things seemed to (mostly) work well. If I wrote a comment at node/1 it was recorded as being in English, and if I wrote a comment at es/node/1 it was recorded as being in Spanish. (This worked the same regardless of whether I saved the comment directly or previewed it before saving, although previewing did show some PHP notices.) And if I went back and edited the comment that was originally in English, I got a translation tab which allowed me to enter a Spanish version, and that seemed to work perfectly (once I had two versions of the comment body, the English version was shown at node/1 and the Spanish version was shown at es/node/1). The only thing that didn't work was the opposite; when I started with a comment that was originally written in Spanish, I was unable to use the Translate tab to create an English version.
After applying the patch and clearing caches, very little worked at all. When I entered a comment body (in either English or Spanish), it simply disappeared (i.e., the comment was saved to the database, but with everything I had typed for the comment body gone). Complete data loss. The only way it worked is if I previewed the comment before saving it; in that case it did save correctly. (And after it saved, the comment translation tab did seem to work better than before.)
So... obviously that doesn't sound too good on the surface :) Any thoughts? Am I doing something wrong here?
Comment #37
plachEntity Translation currently works around the current core behavior. It needs to be updated once the patch is committed. I can test this again but we have tests in place that demonstrate that the core behavior is correct (or tests are broken either).
Do you think you could test it again without ET and replicate the test case?
Comment #38
YesCT CreditAttribution: YesCT commented@plach I'm wondering about the status of this issue.
Comment #39
plachI thought it was RTBC but David reports failures with ET (D7). However ET is supposed to be fixed after the patch here gets in so this may not indicate there's something bad with it.
if you could have a look to the comment language test and try to replicate it to check that everything works as expected it would be great. You can manually set field translatability by altering the
translatable
column in thefield_config
table and clearing the caches afterwards.Comment #40
Berdir@plach: Can you provide/reference a patch for ET so that this can be tested and maybe helps to make the implications of this more visible?
Comment #41
plach@Berdir:
There's a patch in #1760270-7: Comment translation broken? that is meant to update ET after this issue is fixed.
Comment #42
plachOk, I finally found some time to test this again. Actually #31 does not work with the latest dev of ET but this was expected: we have already an issue for this: #1760270-9: Comment translation broken?. I repeated @David's tests with the latest patch over there and everything behaved correctly.
Moreover I performed the following tests:
LANGUAGE_NONE
as comment language and comment body language;Setting back to RTBC since it seems no one else will review this one.
Comment #43
YesCT CreditAttribution: YesCT commented#31: drupal-comment_field_language-1534674-31.patch queued for re-testing.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for the pointer to #1760270: Comment translation broken? (and for your persistence in general). That helped a lot, since the main thing I was concerned about here was that the patch seemed to break (mostly) working functionality in Entity Translation, so that made me wonder what else it would break... However, looking at the required code changes in that issue and the surrounding code, it seems pretty clear that it's breaking Entity Translation for a very specific reason that isn't too likely to show up elsewhere.
So, committed to 7.x: http://drupalcode.org/project/drupal.git/commit/dfed194
We definitely need some kind of change notice here, though I'm not 100% sure I understand what it should contain :) In any case, I think you should feel free to mention the relevant Entity Translation changes in the change notice too.
And if it helps in terms of coordinating releases of Entity Translation with the release of this change in core, I'm thinking there's a very good chance the next D7 bugfix release will be February 6 (doesn't seem like there's a realistic chance of one on January 2).
Comment #45
plachThanks, here is the change notice: http://drupal.org/node/1874724.
Good to know :)
Comment #46
David_Rothstein CreditAttribution: David_Rothstein commentedThanks... looks pretty good to me. I made a few small edits (nothing major) and added a link to this in CHANGELOG.txt, so I think we're good here.
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedDrupal 7.19 was a security release only, so this issue is now scheduled for Drupal 7.20 instead.
Fixing tags accordingly.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedDrupal 7.20 was a security release only, so this issue is now scheduled for Drupal 7.21 instead. For real this time... I think :)
Fixing tags accordingly.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedFixing tags since Drupal 7.21 only contained a fix to deal with fallout from the Drupal 7.20 security release. Hopefully this is really the last time I do this and this will be released in Drupal 7.22 for real :)