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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Issue tags: +D8MI

This needs to be fixed in D7 too. And will need a D7 > D7 update function :(

andypost’s picture

So what language should be used in comment fields and for comment entity itself?

webflo’s picture

Issue tags: +language-content
plach’s picture

@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 of field_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.

cweagans’s picture

slowflyer’s picture

Version: 8.x-dev » 7.15
FileSize
3.7 KB

I 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.

webflo’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, comment-entity_translation_workaround-1534674.patch, failed testing.

plach’s picture

Version: 7.15 » 8.x-dev
Status: Needs work » Needs review
FileSize
3.26 KB

Bugs are fixed in the development version first. We need to update tests but let's see if nothing breaks first.

andypost’s picture

Issue tags: +Needs tests

I think this require a tests

plach’s picture

Assigned: Unassigned » plach

Yes, working on that.

plach’s picture

Here 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.

plach’s picture

Issue tags: -Needs tests
andypost’s picture

Status: Needs review » Reviewed & tested by the community

It works!

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.phpundefined
@@ -294,7 +293,9 @@ class CommentFormController extends EntityFormController {
-      $comment_body = $comment->comment_body[LANGUAGE_NOT_SPECIFIED][0];
+      $field = field_info_field('comment_body');
+      $langcode = field_is_translatable('comment', $field) ? $this->getFormLangcode($form_state) : LANGUAGE_NOT_SPECIFIED;

Possibly this could introduce a performance regression. But this change is required

+++ b/core/modules/field/field.multilingual.incundefined
@@ -301,7 +301,7 @@ function field_valid_language($langcode, $default = TRUE) {
-  $id = $entity->bundle();
+  $id = $entity->id();

I think it makes sense to fix it here

plach’s picture

Status: Reviewed & tested by the community » Needs review

Possibly this could introduce a performance regression. But this change is required

Well, 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.

plach’s picture

Status: Needs review » Reviewed & tested by the community

This 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...

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice
+      $field = field_info_field('comment_body');
+      $langcode = field_is_translatable('comment', $field) ? $this->getFormLangcode($form_state) : LANGUAGE_NOT_SPECIFIED;

I don't see this causing a performance regression, should be a few extra function calls and not a lot more.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentLanguageTest.phpundefined
@@ -2,17 +2,17 @@
- * Definition of Drupal\locale\Tests\LocaleCommentLanguageTest.

This needs to be updated for the new path. A bit annoying that the @file duplicates like this.

ion field_valid_language($langcode, $default = TRUE) {
  */
 function field_language($entity_type, $entity, $field_name = NULL, $langcode = NULL) {
   $display_langcodes = &drupal_static(__FUNCTION__, array());
-  $id = $entity->bundle();

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

plach’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
529 bytes

And here it is.

catch’s picture

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

OK thanks. Committed/pushed the follow-up, moving to 7.x for backport.

plach’s picture

Ok 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.

plach’s picture

Assigned: Unassigned » plach
Issue tags: -Novice

On this.

plach’s picture

Status: Patch (to be ported) » Needs review
Issue tags: +API change
FileSize
3.74 KB
10.7 KB

This 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 after field_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.

Status: Needs review » Needs work

The last submitted patch, drupal-comment_field_language-1534674-23.test.patch, failed testing.

plach’s picture

Let's try again.

Status: Needs review » Needs work
Issue tags: -translatable fields, -API change, -Needs backport to D7, -D8MI, -language-content

The last submitted patch, drupal-comment_field_language-1534674-25.patch, failed testing.

plach’s picture

Status: Needs work » Needs review
Issue tags: +translatable fields, +API change, +Needs backport to D7, +D8MI, +language-content

No 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.

plach’s picture

Assigned: plach » David_Rothstein

Assigning 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.

plach’s picture

Status: Needs review » Reviewed & tested by the community

This is an almost straight port of what was committed in #18: tentatively marking RTBC to get some feedback by David.

plach’s picture

+++ b/modules/locale/locale.module
@@ -386,20 +386,53 @@ function locale_form_node_form_alter(&$form, &$form_state) {
+ * Form submit handler for node_form().
...
+ * is invoked by node_form_submit_build_node(), because it alters the values of
+ * attached fields. Therefore, it cannot be a hook_node_submit() implementation.

Minor: should talk about comments.

Cannot reroll this now, to be fixed as soon as a new patch is rolled.

plach’s picture

Fixed #30. This one can help go back under thresholds and allow features to be committed to D8. A feedback would be more than welcome.

David_Rothstein’s picture

Assigned: David_Rothstein » Unassigned
Status: Reviewed & tested by the community » Needs review

OK, 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.

plach’s picture

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.

Totally 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 :)

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,

Yes, this is exactly as it looks: it's testing also the preview because it was broken too.

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.

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.

As for the update function, I agree we probably don't need one.

Cool :)

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.

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?

andypost’s picture

Status: Needs review » Reviewed & tested by the community

This 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

+++ b/modules/locale/locale.module
@@ -386,20 +386,53 @@ function locale_form_node_form_alter(&$form, &$form_state) {
+/**
+ * Handles field language on submit for the given entity type.
+ *
+ * Checks if Locale is registered as a translation handler and handle possible
+ * language changes.
+ */
+function locale_field_entity_form_submit($entity_type, $form, &$form_state ) {

This function looks public, so maybe we should file change notice

plach’s picture

Thanks!

Agreed for the change notice. I guess it's David's turn again :)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

OK, 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?

plach’s picture

Entity 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?

YesCT’s picture

@plach I'm wondering about the status of this issue.

plach’s picture

I 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 the field_config table and clearing the caches afterwards.

Berdir’s picture

@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?

plach’s picture

@Berdir:

There's a patch in #1760270-7: Comment translation broken? that is meant to update ET after this issue is fixed.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Ok, 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:

  1. I uninstalled ET but left comment bodies translatable;
  2. I posted a couple of comments and both were created with LANGUAGE_NONE as comment language and comment body language;
  3. I enabled plain multilingual supports for articles and posted a couple of comments, switching page langauge in between; in both cases that comment and comment body language matched the page language;
  4. I previewed a new comment and no notice appeared;
  5. I visited the comment administration page and no notice appeared.

Setting back to RTBC since it seems no one else will review this one.

YesCT’s picture

David_Rothstein’s picture

Title: Comment field language is completely broken » Change notice: Comment field language is completely broken
Category: bug » task
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

Thanks 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).

plach’s picture

Status: Active » Needs review

Thanks, here is the change notice: http://drupal.org/node/1874724.

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).

Good to know :)

David_Rothstein’s picture

Title: Change notice: Comment field language is completely broken » Comment field language is completely broken
Category: task » bug
Priority: Critical » Major
Status: Needs review » Fixed

Thanks... 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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

David_Rothstein’s picture

Drupal 7.19 was a security release only, so this issue is now scheduled for Drupal 7.20 instead.

Fixing tags accordingly.

David_Rothstein’s picture

Drupal 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.

David_Rothstein’s picture

Issue tags: +7.22 release notes

Fixing 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 :)