Comment language administration UI
peximo - October 15, 2009 - 16:53
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | Needs text review, Needs usability review |
Description
With translatable fields in core we might have multiple language versions of the same node. In this scenatio we might meed to filter out comments per language.
#1
This patch implements a content language filter on node comments.
The patch add a language column in {comment}; in this column we save the content language code negotiated while viewing the comment post page.
Comment, for each module that implements hook_comment_filter_info(), invoke hook_comment_filter(); each module that implements this hook can alter the $query object.
The patch also implements this hook in locale that filter the node comments by node content language.
#2
#3
+++ 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.
#4
replaced, thx.
#5
+++ 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?
#6
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.
#7
Done: #605862: Store language for comments
#8
+++ 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
#9
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.
For the documentation, I don't write (and speak also :)) english very well, plach will help me tomorrow.
The others problems/comments are fixed.
#10
Sorry, wrong indentation on previous patch.
#11
@Dries:
something for a seperate UI patch?
#12
Patch with documentation.
#13
Suppose we should care about admin/content/comment as Dries pointed at #605862: Store language for comments
Something like admin/content filters for moderation
#14
@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.#15
Why not a query tag and hook_query_alter() here?
#16
suppose, count query should be affected too
#17
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
#18
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.
#19
Realized query_alter aproach only one thing todo - make a permission for Filtering comments
#20
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
#21
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.
#22
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...
#23
+++ 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.
#24
@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.
#25
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)
#26
Screen-shot attached
#27
The last submitted patch failed testing.
#28
IMO one module should be able to add multiple filters (locale can add one for filtering comments on content language and another one for filtering by user prefered language etc.). With current approach this seem to me impossible.
IMO the comment language is a useful information so it should be showed in comments_overview. I don't know wich is the correct distinction to show this field: locale is enabled? Is there at least one node with a language?
I have add another features that IMO is useful: the comment language selection for users with 'change comments language' permission in both comment post and edit.
#29
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
#30
@andypost: it's a good solution, but remains that one module can implement only one filter.
#31
There's another problem with this approach: there isn't a description of what the filter does. I know that for a content type I have enabled a filter given by a module but via UI I do not know what this filter does.
#32
@peximo Agree exactly with all your points but main problem is api-freeze
#33
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$multilanguageinto$multilingualalso.+++ 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 usesmodule_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?
#34
Because api-freeze it's impossible to have here a patch that handle comments filters. The previous solution that implements one filter per module with the name of the module as description in the UI is IMO not acceptable.
Rather it is preferable that each module implements its filters for the comments, eg. user.module can filtering the comments by the $user->language etc.
In the previous patch there is a features for the comment language administration; this features is IMO important, so a repost the patch without the filter system but with this features.
#35
FYI: a comment filter has been re-introduced in #539110: TF #4: Translatable fields UI
#36
The last submitted patch failed testing.
#37
rerolled
#38
#39
The patch looks good, works fine and responds to the requirement Dries expressed in #605862-2: Store language for comments and in #6:
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?
#40
#41
For the node form is locale in locale_form_alter() that add the language selector controlling if a content type has multilingual support. So if we don't control this support also in comment we might have a node without the language selector but with associated comments with this features.
And this behavior is IMO not consistent.
Perhaps the language selector and the permission should be set by locale?
I have fixed the others things.
#42
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.
#43
#44
Ok, with this patch comment adds only the administration language column, the language selector and the 'change comments' permission is added by locale.
Because to do this we must have the node->type property, I have added $node object to the comment form.
#45
added tests.
#46
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.
#47
Fixed with plach suggestion.
#48
The last submitted patch failed testing.
#49
rerolled
#50
#51
I have forgot to replace "comments language" with "comment language".
#52
+++ 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?
#53
I wouldn't use a single permission because you might not wish to give a user the ability to select the language either for a node and for a comment.
IMO comment permissions should be separated from node ones.
Plach has added this TOC after reading this in Guidelines - Test template:
Fixed the trailing comma.
#54
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:
#55
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
#56
The patch add a language selector when a comment is posted or edited, not a filter. Attached there is a screenshot.
#57
The patch implements also the language column in the comments admin UI. See #34 for details.
#58
Here is a screenshot
#59
:o the user has to select the language he is commenting in?
#60
@bojhan: user with proper permission (change comment language) can select the comment language. If not, you set a default comment language #624290: Wrong default comment language.
#61
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?
#62
#63
Without the UI part in comment.module (the language column in the comment administration) we would have a mismatch between the default behavior of administration of the nodes (which shows a language column in the administration page) and comments. The part in locale.module (the language selector) I think may be in a contributed module.
I think we should have a new permission for distinguish users like translators and users, also anonymous, that doesn't translate contents but that can select their comment language.
#64
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.
#65
I agree but IMO at least the language column in comments administration should be in core.
#66
Can you explain why, please? I'm not sure I see the use-case for it.
#67
I think that without the comment language column we miss an useful information: obviously if we have a default behavior (node translated via node translation and comments related uniquely to one node) this column can be superfluous, but with a node translated via translatable fields and comments filtered by language (#641034: Make comment lists filterable) this information seems to me important.
If later then it will be implemented a language selector with a contribuited module this information will be much more important.
I don't think that this column should be added by the same contributed module that add the selector: for node is node module that add the language column, not locale nor translation, although the visualization of this column depends on the enabling of translation module.