Store language for comments [#605862]

Comment	File	Size	Author
#16	605862_comment_lang.patch	3.25 KB	andypost

#13	605862_comment_lang.patch	3.03 KB	andypost

#11	605862_comment_lang.patch	2.92 KB	andypost

#9	605862_comment_lang.patch	2.49 KB	andypost

#6	605862_comment_lang-2.patch	2.56 KB	peximo

#3	605862_comment_lang.patch	2.21 KB	andypost

	drupal.comment-store-language.patch	1.77 KB	sun

Comment #1

andypost

he/him

Russian

CreditAttribution: andypost commented 15 October 2009 at 21:19

Status:

Needs review

» Reviewed & tested by the community

Tested with 2 languages and it works

Suppose therу should be follow-up after #533924: Allow different language types to be enabled separately to set language of the content not the interface language

Log in or register to post comments

Comment #2

Dries CreditAttribution: Dries commented 15 October 2009 at 21:36

Status:

Reviewed & tested by the community

» Needs review

On my personal blog, some people will post comments in Dutch although I have none of the i18n/locale stuff enabled. So, Dutch comments would be stored as English afaik. At some point, it might be nice to be able to correct that. It seems like that could happen a lot though -- it is not because the interface is in English, that the comments will be in English. Commenters are not trained/educated content editors so I'd expect there to be a lot of clean-up work needed to be done by moderators.

Log in or register to post comments

Comment #3

andypost

he/him

Russian

CreditAttribution: andypost commented 15 October 2009 at 22:34

File	Size
605862_comment_lang.patch	2.21 KB

This patch just parents language from node->language

I think to solve this we need language-changer for comments in admin somewhere but this task is for contrib.
Default behavior should check node->language then fallback to interface language.
It's not good idea to give user ability to choose a language of the comment form any kind of visual control.

Log in or register to post comments

Comment #4

peximo CreditAttribution: peximo commented 15 October 2009 at 23:42

In #605630: Comment language administration UI sun say:

Hm, this actually filters by the current global content language, and not by the assigned language of the node.

And I reply:

Yes, because with translatable fields without translation.module the node language is always only one but we can have fields translated; with this patch we can show a node with translated fields depending on the selected content language and filtering the comments for this language.

IMO if a user view a node that "appears" in one language (with translated fields) it's a good idea to set the comment language like the content language and not the node language.
EG. a node with nid 1 has node->language = en and a set of fields translated to italian. If i get /it/node/1 the content of the node (the fields) is showed in italian. So IMO the comments are italian too more easily than english.

Log in or register to post comments

Comment #5

andypost

he/him

Russian

CreditAttribution: andypost commented 15 October 2009 at 23:52

Anyway the $comment->language shell be assigned at comment_form() stage and not in comment_save() because comment_save() could be used in triggers\actions staff.

So in my patch I use $comment->language in both insert and update

Log in or register to post comments

Comment #6

peximo CreditAttribution: peximo commented 16 October 2009 at 00:03

File	Size
605862_comment_lang-2.patch	2.56 KB

Ok, with this we have $comment->language like in previous (andypost) patch and the content language instead of node->language.

Log in or register to post comments

Comment #7

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 00:06

+++ modules/comment/comment.module	15 Oct 2009 22:33:04 -0000
@@ -1852,6 +1854,10 @@ function comment_form($form, &$form_stat
+  $form['language'] = array(
+    '#type' => 'value',
+    '#value' => isset($comment->language) ? $comment->language : $node->language,
+  );

If we go this route, then we should use the global $language->language here, I guess, or not?

I'm on crack. Are you, too?

Log in or register to post comments

Comment #8

peximo CreditAttribution: peximo commented 16 October 2009 at 00:09

I agree with sun. The last patch use $language->language.

Log in or register to post comments

Comment #9

andypost

he/him

Russian

CreditAttribution: andypost commented 16 October 2009 at 00:29

File	Size
605862_comment_lang.patch	2.49 KB

If you trying to use global $language so it should be defined

Log in or register to post comments

Comment #10

plach

he/him

Italian

Venezia

CreditAttribution: plach commented 16 October 2009 at 00:38

Issue tags:

+translatable fields

Log in or register to post comments

Comment #11

andypost

he/him

Russian

CreditAttribution: andypost commented 16 October 2009 at 01:10

File	Size
605862_comment_lang.patch	2.92 KB

Now with upgrade path

Log in or register to post comments

Comment #12

catch

he/him

English

CreditAttribution: catch commented 16 October 2009 at 01:24

Status:

Needs review

» Reviewed & tested by the community

This looks great.

I think we should evaluate whether we want to mass-update the comments table on update with the language of the node they were posted on, if that's already set. However that requires a lot of thought and might not be desirable. I tested the upgrade path and it worked fine, did a describe on comments to verify everything was in place.

We need an index on language - but we need to have a query using it first to see if it's nid, status, language or just language or whatever, so let's discuss that in a (critical) followup issue.

Log in or register to post comments

Comment #13

andypost

he/him

Russian

CreditAttribution: andypost commented 16 October 2009 at 01:40

File	Size
605862_comment_lang.patch	3.03 KB

This is a patch with index, because we dont really know what kind of queries will be run I use nid+language for #605630: Comment language administration UI

Suppose we can add more indexes latter as follow-ups

Log in or register to post comments

Comment #14

sun

German

Karlsruhe

CreditAttribution: sun commented 16 October 2009 at 02:34

I agree.

Log in or register to post comments

Comment #15

catch

he/him

English

CreditAttribution: catch commented 16 October 2009 at 02:34

Status:

Reviewed & tested by the community

» Needs work

Need to add the index in comment_schema() too.

Log in or register to post comments

Comment #16

andypost

he/him

Russian

CreditAttribution: andypost commented 16 October 2009 at 02:38

Status:

Needs work

» Needs review

File	Size
605862_comment_lang.patch	3.25 KB

Oh, changed schema

Log in or register to post comments

Comment #17

catch

he/him

English

CreditAttribution: catch commented 16 October 2009 at 02:44

Status:

Needs review

» Reviewed & tested by the community

That's it. Please wait for green from test bot before committing.

Log in or register to post comments

Comment #18

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 16 October 2009 at 03:40

Status:

Reviewed & tested by the community

» Fixed

I understand Dries's concerns at #2, but I'm not sure what we can do about this, really... at least in core. Short of presenting a language selector to the user to choose which language their comment is posted in, which strikes me as a UX nightmare to figure out.

On IRC catch pointed out two things:

1. A contrib module could also hook into google translation API or whatever for assigning languages once the storage property is there.
2. Comment module currently deletes all child comments without warning, even if flat comments, and lets users click any reply link they want. Posting in a different language is the least of our worries.

;)

I'm inclined to agree. This patch looks pretty straight-forward to me.

Committed to HEAD. Though Dries, if you feel strongly about this, obviously feel free to roll-back.