Related to: #908316: Upgrade path for changed Comment form ids

Since the comment form on each node type has a different ID in Drupal 7, we need some way to set a default that is applied to new node types. Currently if I create a new node type, its comment form will not be protected by mollom even if all existing node type comment forms are.

Perhaps mollom should implement http://api.drupal.org/api/function/hook_form_BASE_FORM_ID_alter/7 for comment and node forms, so that there is a mechanism to protect all comment forms or all node forms with a single configuration entry?

Alternately, we need a new interface in mollom to define the defaults for newly created node types and their comment forms?

In comment.module we now have:


/**
 * Implements hook_forms().
 */
function comment_forms() {
  $forms = array();
  foreach (node_type_get_types() as $type) {
    $forms["comment_node_{$type->type}_form"]['callback'] = 'comment_form';
  }
  return $forms;
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
1.54 KB

simplest patch to start.

Status: Needs review » Needs work

The last submitted patch, 913532-new-comments-forms-1.patch, failed testing.

sun’s picture

Title: Mollom will not protect the comment form on new content types » Automatically protect the comment form of new content types
Category: bug » feature
Priority: Critical » Normal
+++ mollom.module	(working copy)
@@ -2046,6 +2046,21 @@ function mollom_form_node_multiple_delete_confirm_
+    // Clear the node type cache. ¶
+    // @todo - core patch so we don't need this.
+    drupal_static_reset('_node_types_build');

Nice catch! I guess we need to clear that static before and after invoking the hook then. Will you create a core patch?

+++ mollom.module	(working copy)
@@ -2046,6 +2046,21 @@ function mollom_form_node_multiple_delete_confirm_
+    $mollom_form = mollom_form_new("comment_node_{$type->type}_form");
+    mollom_form_save($mollom_form);

+++ mollom.admin.inc	(working copy)
@@ -549,6 +549,13 @@ function mollom_admin_settings($form, &$form_state
+    '#type' => 'checkbox',
+    '#title' => t('Protect comments for new content types'),
+    '#default_value' =>  variable_get('mollom_protect_new_comment_types', 0),
+    '#description' => t('Causes Mollom to automatically add text analysis for spam for the subject and body for comments on any newly created content type.'),

So to also transfer my considerations into this issue:

I'm not sure whether this is a good idea, because:

- it only auto-protects the comment form of the new node type, but not the new node form itself.

- only the comment subject and body field are automatically protected. If you add further fields, they are not protected. Quite confusing, and needs extra help text to TRY TO make sure that users understand that they have to update the form protection after adding new fields.

- I'd somehow expect that checkbox to sit on the node type form, instead of Mollom's admin settings form.

- we purposively removed the auto-protection feature from D6 (which I think happened during module installation back then), as it resulted in some UX or even technical issues, which I'm currently unable to recall. (need to look up that issue)

Powered by Dreditor.

sun’s picture

So one of the issues was #672788: Do not enable protection for all forms on install, but apparently, it's not the one I mean (can't find it), as we are no longer auto-protecting any forms upon module installation either.

Given the consequences/problems I mentioned in #3, I'm not sure whether it's worth to work on this.

pwolanin’s picture

Well, I think we need to do something to offer this kind of functionality in some fashion.

sun’s picture

It's quite a challenge to do this right.

- Automatically analyzing all fields imposes a security issue, if fields happen to contain sensitive user data. Although we only support text fields, we never know what users are doing with text fields.

- If the body field is removed from a comment form, then there might be no (protected) field left for text analysis.

- Automatically protecting forms can lead to protected forms that were not intended to be protected.

- Users should be aware of and not forget about Mollom's form configuration.

Dave Reid’s picture

Seems like it might make a good 'Mollom auto-protect' module.

David_Rothstein’s picture

Here's an updated patch. In current D7 HEAD, drupal_static_reset() no longer works for clearing the node type cache; node_type_cache_reset() needs to be used instead.

Agreed this probably makes more sense as a separate module, though.

sun’s picture

Status: Needs work » Closed (won't fix)

Sorry, trying to ensure a sane queue.

Alan D.’s picture

Issue summary: View changes
Status: Closed (won't fix) » Active

Sorry to reopen, but wondering if your thoughts on this have changed?

Even if it is a doco page that tells us what to do to enable this by default via code would be very cool, for both node_form and comment_form.

Field protection on most sites is rare, < 10%. Content types that slip through getting protection added on sites is high resulting, in unhappy clients...

sun’s picture

Good timing. I thought a lot about this just recently.

I'm considering a major rewrite of the module to be no longer based on forms, but on entity types (and bundles/subtypes) instead.

Already in D7, the vast majority of protected forms are entity forms. And for entity forms, the module is able to automatically discover all text fields that are available on the entity.

The >90% use-case is to protect everything that Mollom is able to protect.

I'd like to turn the entire Mollom configuration upside down: Instead of having to individually configure every single thing that you want to protect, you should only have to configure which protections should be "skipped."

Likewise, all forms should be protected with the recommended options by default. You should be able to change the defaults globally (for all protected forms), and perhaps, you should be able to override the options for a particular form.

In short, I believe the Mollom module is too hard and too complicated to set up right now. There are too many manual steps, too many options, and you borderline need a Mollom Configuration Specialist™ certification to get it right.

I'm very tempted to work on such a new 3.x-ish incarnation, and considered to begin with D8, so as to start with an ideal architecture first and only deal with a (probably painful) D7- backport later.

What do you think?

Alan D.’s picture

Big kudos from me if you can find the time!

Mollom Configuration Specialist™ certification

lol, I don't think it is that bad! It is just easy to let forms skip through the process.

Was contact module ported to D8? This is the only form that comes to mind that may not be entity based that is in core. It is one module we never enable on client sites. And in contrib, webforms are not entity based.

There are some entities types that you would want to skip by default, actually you would want to skip most. I have a CRUD logging module in development that I am hitting the same issue with, and I am thinking that a single settings page where you select the entity types to track is going to be the best way to handle these. This is a list of entity types from one of our installs

bean
comment
country
field_collection_item
file
flagging
node
profile2
profile2_type
redirect
rules_config
search_api_index
search_api_server
user
wysiwyg_profile

Of these, on a standard site, only comments would get the protection although there are a few sites certain node types could need protection. So maybe protect all comments / nodes but add a configure link to all forms to the configuration page to enable / disable protection. I think the captcha module does something similar... been years since I have used that one.

andrewkillen’s picture

any idea when this will happen, the change to mollom to make it more administrator friendly? i.e. enable all forms with mollom unless specifically disabled.

I help to manage 18 sites and we are constantly adding content types and other forms, and could really do with this as on some db's we're running into 6+GB of form cache from spam.

Yes I should just implement it better, but this is a background that should really be automatically done by the system, not left till there is an issue that forces the check of configuration...

I would not feel so desperate for this need if mollom were free, but it is a paid service and as such should show continued support and development,

(sorry to rant....)

eshta’s picture

Status: Active » Postponed

Mollom is currently in the process of being ported to Drupal 8. That's keeping the maintainers pretty busy.... but once that is complete there will be time for these types of large feature requests again. In the meantime - feel free to contribute ideas or code towards new features that you'd like to see or to take ownership entirely of a change. Maintainers are still around to review and help out as these progress.