Comment language administration UI

+++ modules/comment/comment.module	15 Oct 2009 16:37:20 -0000
@@ -720,6 +720,11 @@
+  foreach ($filters as $module) {
+     $function = $module . '_comment_filter';
+     $function($query);
+  }

module_invoke()?

This review is powered by Dreditor.

+++ modules/locale/locale.module	15 Oct 2009 17:31:32 -0000
@@ -1015,3 +1015,11 @@
+function locale_comment_filter_info($node_type) {
+  return array('locale' => t('Filter by content language')); 
+}
+
+function locale_comment_filter(&$query) {
+  global $language;
+  $query->where("language = :name", array(':name' => $language->language));
+}

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

Missing PHPDoc.

Furthermore, both hooks need to be documented in comment.api.php.

+++ modules/comment/comment.module	15 Oct 2009 17:31:31 -0000
@@ -720,6 +720,10 @@
   $query->setCountQuery($count_query);
+  $filters = variable_get('comment_filters_' . $node->type, array());
+  foreach ($filters as $module) {
+    module_invoke($module, 'comment_filter', $query);
+  }
   $cids = $query->execute()->fetchCol();

I'm not yet 100% familiar with the new entity controller... this looks a bit like a query builder to me. I think that we prepare and pass the query conditions elsewhere initially instead.

Now I figure that this snippet probably doesn't belong to the entity controller, but to comment's thread builder function... so that makes a bit more sense - although I just rolled a patch that moved query conditions into the function signature ($mode, $pages), so that the caller can decide on that.

Instead of module_invoke(), we need to build the target $function, test whether function_exists($function) and directly invoke $function($query) instead.

+++ modules/comment/comment.module	15 Oct 2009 17:31:31 -0000
@@ -1011,6 +1015,20 @@
+    // Get all defined filters

Missing trailing period (full-stop).

In general, the inline documentation/comments are a bit lacking in this patch.

+++ modules/comment/comment.module	15 Oct 2009 17:31:31 -0000
@@ -1011,6 +1015,20 @@
+    $filters = array();
+    foreach (module_implements('comment_filter_info') as $module) {
+      $function = $module . '_comment_filter_info';
+      $filters += $function($form['#node_type']->type);
+    }

This makes no sense.

1) The hook returns an array and you are accessing an object.

2) We keep the array and just use module_invoke_all() here (a single line).

3) With m_i_a(), the initial declaration of $filters can go, because it always returns an array.

+++ modules/comment/comment.module	15 Oct 2009 17:31:31 -0000
@@ -1011,6 +1015,20 @@
+    if (count($filters) > 0) {
+      $form['comment']['comment_filters'] = array(
+        '#type' => 'checkboxes',
+        '#title' => t('Comment filters'),
+        '#options' => $filters,
+        '#default_value' => variable_get('comment_filters_' . $form['#node_type']->type, array()),
+      );
+    }

Instead of the surrounding if() statement, you take the expression of the condition and place it into #access.

+++ modules/comment/comment.module	15 Oct 2009 17:31:31 -0000
@@ -1365,6 +1382,7 @@
         'name' => $comment->name,
         'mail' => $comment->mail,
         'homepage' => $comment->homepage,
+        'language' => $language->language
       ))
       ->execute();

Missing trailing comma here.

I guess that this is the hunk where a comment is saved?

Why do we store the current global content language and not the displayed language of the node?

Please roll your patches with diff -up, so we can see what is changed.

+++ modules/comment/comment.install	15 Oct 2009 17:31:30 -0000
@@ -263,6 +263,12 @@
+      'language' => array(
+        'type' => 'varchar',
+        'length' => 12,
+        'not null' => FALSE,
+        'description' => "The languages.language of this comment.",
       )

Why not null?

Please use single quotes here.

Missing trailing comma.

I'm on crack. Are you, too?

I'd recommend that we split this patch in two halfs. One patch that stores the language information with the comments, and one patch that provides the filter. The part that stores the language information is a no-brainer, except that it doesn't look like one can edit the language afterwards.

Done: #605862: Store language for comments

+++ modules/comment/comment.module	15 Oct 2009 17:31:31 -0000
@@ -1263,7 +1281,6 @@
   // Make sure we have a bundle name.
   if (!isset($comment->node_type)) {
-    $node = node_load($comment->nid);
     $comment->node_type = 'comment_node_' . $node->type;
   }

It's a good idea to remove node_load() but next line will give exception about non-existent object $node

@Dries:

The part that stores the language information is a no-brainer, except that it doesn't look like one can edit the language afterwards.

something for a seperate UI patch?

Suppose we should care about admin/content/comment as Dries pointed at #605862: Store language for comments

Something like admin/content filters for moderation

@andypost: We might want to introduce a language selector in the comment form just as in the node form. Only users with the appropriate permission would see it, the others would get language based on global $language.

Why not a query tag and hook_query_alter() here?

suppose, count query should be affected too

Apart from query tag vs. hook_comment_filter_info() - if for some reason we do need a specific hook, we also need hook docs in comment.api.php

Hm. Sorry, this approach is not going to fly. We do not want to start defining hooks of modules that implement hooks of other modules all over the place. Would become a spaghetti factory. I also don't like the coupling between locale and comment.

Realized query_alter aproach only one thing todo - make a permission for Filtering comments

Forget to add comment module patch, now it's full

But now there's no permission check for altering comment filters

Need a review on comments, oh my english

Just discussed this briefly with andypost. I was wondering about the node type variable, the reason that's needed is to stop locale.module altering the query just because it's enabled, even if the feature isn't needed (you can have locale module enabled, but not comments posted in multiple languages). That makes sense now. My only concern here is that the UI text doesn't really explain what the feature is (and I'm not sure how much it needs to be exposed) - makes me think about filtering in comment admin, not a module-provided filter. So not marking RTBC just yet, but not CNW either.

The old hook_comment_info() collects possible filters mostly for settings page to allow enable some filters for comments

To re-implement this functionality:
1) gather modules by module_implements('query_comment_filter_alter')
2) change settings for node to checkboxes with a list of modules
3) http://drupal.org/node/310077 pass metadata for query so every alter could check it's permissions
4) write an example alter for locale module

This patch is working example...

+++ modules/comment/comment.module	16 Oct 2009 05:20:46 -0000
@@ -705,6 +705,15 @@ function comment_get_thread($node, $mode
+  // Pass all comment filters allowed for this node type.
+  $comment_filters = variable_get('comment_filters_' . $node->type, array());
+  if (!empty($comment_filters)) {

Query tags don't hurt, you can unconditionally add them, so this can go.

+++ modules/comment/comment.module	16 Oct 2009 05:20:46 -0000
@@ -705,6 +705,15 @@ function comment_get_thread($node, $mode
+    $query->addTag('comment_filter');
+    $query->addMetaData('comment_filters', $comment_filters);
+    $count_query->addTag('comment_filter');
+    $count_query->addMetaData('comment_filters', $comment_filters);

Why don't you add the tag to the query before it is cloned?

+++ modules/comment/comment.module	16 Oct 2009 05:20:46 -0000
@@ -1014,6 +1023,16 @@ function comment_form_node_type_form_alt
+    // Collect all modules that define comment filters.
+    $filters = module_implements('query_comment_filter_alter');
+    $filters = array_intersect(module_list(), $filters);
+    $form['comment']['comment_filters'] = array(
+      '#type' => 'checkboxes',
+      '#title' => t('Comment filters'),
+      '#options' => $filters,

This will display a list of checkboxes with module names... "locale" (lowercase; the internal module name) being one of them. Not sure whether that's grokable.

module_list() isn't required at all here. you could use drupal_map_assoc() instead, but that doesn't solve the underlying issue explained above.

This review is powered by Dreditor.

@sun agree, tags and meta could be added unconditional
I dont know when the query is cloned... so tags and meta are added just after other tags added.

I know that module_list produce a lowercase but I found no ability to get the Real module-name.

This is just an example - maybe for contrib to add more granular permission for comment filters.

Added a tags because this needs some UI

1) Ability to enable filters for comments (basically core allows comments to be filtered by language by switching language if locale module enabled and checkbox allows it for {node-type} at admin/structure/types/manage/{node-type})

screenshot attached - last patch provides ugly t('Comment filters') checkboxes at node-type-edit form admin/structure/types/manage/{node-type}

peximo's patch gathers filter names with a additional hook which give ability to show - Filter by content language instead of plain module name, maybe it's a good idea to add this hook or maybe just form-alter aproach?

2) admin/contant/comment screen - should we add language column when locale module enabled? like /admin/content does for nodes

3) Ability to filter comments in moderation queue at admin/content/comment (suppose this task for contrib, if so we could make this alterable in code and describe in docs)

Screen-shot attached

Let's change module_list() with new http://api.drupal.org/api/function/system_get_info/7 from #561452: Add API function to get module/theme info without rebuilding it.
This give us a real module name with internal module name as option value

@peximo Agree exactly with all your points but main problem is api-freeze

Besides what you said above there is something to polish here:

+++ modules/comment/comment.admin.inc	24 Oct 2009 10:13:01 -0000
@@ -63,8 +63,15 @@ function comment_admin_overview($form, &
+  $multilanguage = (module_exists('locale'));

You need drupal_multilingual() here. You might want to change $multilanguage into $multilingual also.

+++ modules/comment/comment.module	24 Oct 2009 10:57:23 -0000
@@ -706,6 +710,15 @@ function comment_get_thread($node, $mode
+    

trailing whitespaces

+++ modules/comment/comment.module	24 Oct 2009 10:57:23 -0000
@@ -1023,6 +1036,16 @@ function comment_form_node_type_form_alt
+    $filters = array_intersect(module_list(), $filters);

module_implements() already uses module_list(), IMO this line is useless.

+++ modules/comment/comment.module	24 Oct 2009 10:57:23 -0000
@@ -1878,6 +1901,16 @@ function comment_form($form, &$form_stat
+  

trailing whitespaces

+++ modules/comment/comment.module	24 Oct 2009 10:57:23 -0000
@@ -1878,6 +1901,16 @@ function comment_form($form, &$form_stat
+  if (module_exists('locale')) {

drupal_multilingual()

+++ modules/comment/comment.module	24 Oct 2009 10:57:23 -0000
@@ -1878,6 +1901,16 @@ function comment_form($form, &$form_stat
+      '#access' => user_access('change comments language') 

trailing whitespace

+++ modules/locale/locale.module	24 Oct 2009 09:52:40 -0000
@@ -1068,6 +1068,18 @@ function locale_date_format_reset_form_s
+    $query->where("language = :name", array(':name' => $language->language));

This should be

+    $query->where("language = :name OR language = ''", array(':name' => $language->language));

otherwise language neutral comments will disappear.

I'm on crack. Are you, too?

FYI: a comment filter has been re-introduced in #539110: TF #4: Translatable fields UI

The patch looks good, works fine and responds to the requirement Dries expressed in #605862-2: Store language for comments and in #6:

The part that stores the language information is a no-brainer, except that it doesn't look like one can edit the language afterwards.

This might need some tests, primarily check if language conditions (multilingual site, multilingual content type) are respected.
There are also some small things to fix:

+++ modules/comment/comment.admin.inc	4 Nov 2009 16:59:03 -0000
@@ -63,8 +63,14 @@ function comment_admin_overview($form, &
+  
+  // Enable language column if there are at least two languages enabled.

Should be "Enable the language column ...". Watch out for the trailing whitespaces in the first line.

+++ modules/comment/comment.module	4 Nov 2009 16:56:17 -0000
@@ -1874,6 +1878,22 @@ function comment_form($form, &$form_stat
+  // Add the language selector if there are at least two languages enabled, the 
+  // content type has multilingual support and user has 'change comments 

Shoud be "... and the user has the 'change comments permission'". Watch out for the trailing whitespaces.

+++ modules/comment/comment.module	4 Nov 2009 16:56:17 -0000
@@ -1874,6 +1878,22 @@ function comment_form($form, &$form_stat
+  if (drupal_multilingual() && variable_get('language_content_type_' . $node->type, FALSE)) {

Locale defines locale_multilingual_node_type() to check if a content type is multilingual, but using it would introduce an explicit dependency from Locale. However we are already implicitly doing it by checking a configuration variable (and a logic) defined by Locale.

+++ modules/comment/comment.module	4 Nov 2009 16:56:17 -0000
@@ -1874,6 +1878,22 @@ function comment_form($form, &$form_stat
+      '#access' => user_access('change comments language') 

Trailing whitespace.

I'm on crack. Are you, too?

Yes, probably the changes to comment.module should be straightforwardly moved to locale.module.

+++ modules/comment/comment.module	5 Nov 2009 12:52:22 -0000
@@ -1874,6 +1878,23 @@ function comment_form($form, &$form_stat
+  //if (drupal_multilingual() && variable_get('language_content_type_' . $node->type, FALSE)) {
+  if (drupal_multilingual()) {

These lines must be fixed.

This review is powered by Dreditor.

I am happy with this one, except for some minor issue:

+++ modules/comment/comment.test	6 Nov 2009 17:50:55 -0000
@@ -914,3 +915,40 @@ class CommentRdfaTestCase extends Commen
+ * Functional tests for comments language administration.

I just realized "comments language" should be replaced by "comment language" everywhere in this patch.

+++ modules/comment/comment.test	6 Nov 2009 17:50:55 -0000
@@ -914,3 +915,40 @@ class CommentRdfaTestCase extends Commen
+  

Trailing whitespaces ;)

+++ modules/comment/comment.test	6 Nov 2009 17:50:55 -0000
@@ -914,3 +915,40 @@ class CommentRdfaTestCase extends Commen
+    // Test that there isn't the language column with only one language
+    // ennabled.

"Test that the language column is not present with only one language enabled." sounds better.

+++ modules/comment/comment.test	6 Nov 2009 17:50:55 -0000
@@ -914,3 +915,40 @@ class CommentRdfaTestCase extends Commen
+    $this->assertNoText(t('Language'), t('There isn\'t the language column.'));

According to the change above: "The language column is not present."

+++ modules/comment/comment.test	6 Nov 2009 17:50:55 -0000
@@ -914,3 +915,40 @@ class CommentRdfaTestCase extends Commen
+    // Test that there is the language column with more than one language
+    // ennabled.

As above: "Test that the language column is present with more than one language enabled."

+++ modules/comment/comment.test	6 Nov 2009 17:50:55 -0000
@@ -914,3 +915,40 @@ class CommentRdfaTestCase extends Commen
+    $this->assertText(t('Language'), t('There is the language column.'));

"The language column is present."

+++ modules/locale/locale.module	5 Nov 2009 15:58:25 -0000
@@ -1105,3 +1109,20 @@ function locale_url_outbound_alter(&$pat
+  // the user has the 'change comments permission'.

The permission name is wrong.

+++ modules/locale/locale.test	6 Nov 2009 17:49:56 -0000
@@ -1799,3 +1799,45 @@ class LocalizeDateFormatsFunctionalTest 
+ * Functional tests for comments language selector.

Should be "Functional tests for the comment language selector." Moreover on the top of locale.test there is a summary of tests which should be integrated.

+++ modules/locale/locale.test	6 Nov 2009 17:49:56 -0000
@@ -1799,3 +1799,45 @@ class LocalizeDateFormatsFunctionalTest 
+    // Post a comment to the previous created article.

Should be "Post a comment to the previously created article."

+++ modules/locale/locale.test	6 Nov 2009 17:49:56 -0000
@@ -1799,3 +1799,45 @@ class LocalizeDateFormatsFunctionalTest 
+    $this->web_user = $this->drupalCreateUser(array('administer comments', 'change comments language'));

Why the user has also the 'administer comments" permission? Shouldn't be enough the 'change comment language' one?

+++ modules/locale/locale.test	6 Nov 2009 17:49:56 -0000
@@ -1799,3 +1799,45 @@ class LocalizeDateFormatsFunctionalTest 
+    // Test that there is the language selector.

Again: "Test that the language selector is present." sounds better.

+++ modules/simpletest/drupal_web_test_case.php	6 Nov 2009 16:16:45 -0000
@@ -1071,6 +1071,7 @@ class DrupalWebTestCase extends DrupalTe
+

Do we really need this change?

This review is powered by Dreditor.

+++ modules/locale/locale.module	7 Nov 2009 12:04:57 -0000
@@ -307,6 +307,10 @@ function locale_permission() {
     ),
+    'change comment language' => array(
+      'title' => t('Change comment language'),
+      'description' => t('Change comment language on post and edit.'),
+    )
   );

Hmmm.... shouldn't this be a more generic permission that applies to nodes + stuff as well?

Minor: Missing trailing comma.

+++ modules/locale/locale.test	7 Nov 2009 13:33:27 -0000
@@ -17,6 +17,7 @@
  *  - a functional test for configuring a different path alias per language;
  *  - a functional test for multilingual support by content type and on nodes.
  *  - a functional test for multilingual fields.
+ *  - a functional test for the comment language selector.

errr... WTF? Do me a favor and remove this entire TOC instead, please. ;)

I'm on crack. Are you, too?

I am not sure this can still go in, but I think the current patch takes care of the issue and responds to the requirement Dries expressed in #605862-2: Store language for comments and in #6:

The part that stores the language information is a no-brainer, except that it doesn't look like one can edit the language afterwards.

Could we get some updated screenshots for this issue? I'm really hoping it looks different than http://drupal.org/files/issues/comment_filters.png because that makes absolutely no sense. :P

The patch implements also the language column in the comments admin UI. See #34 for details.

Here is a screenshot

:o the user has to select the language he is commenting in?

Mmm, personally, I'm not really keen on the UI changes here. At the same time, I don't see how else to deal with this.

Given that we already have #624290: Wrong default comment language in core, is the UI best exposed through a contributed module (for D7)? I think we should have covered the 95% case with #624290.

Either way, should this be a new permission or should we leverage the existing i18n permissions?

I also feel uneasy about this patch.

The system should be smart enough to figure out the language of a comment automatically. I.e., yes, we want to store a language for comments.

But displaying and editing a comment's language does not really make sense for 99% of all use-cases. That should be left for contrib.

Can you explain why, please? I'm not sure I see the use-case for it.

Have a language select on comment forms makes a whole lot of sense. Plus it would be super useful for textual analysis (like in Mollom) to be able to better analyze text based on its language.

subscribing

#53: language_comments-605630-53.patch queued for re-testing.

So maybe proceed with admin interface?

Updated test

What needs review? Did we fix the part that everyone feels uncomfortable about?

#74: 605630-comment-admin-d7.patch queued for re-testing.