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;
}
Comment | File | Size | Author |
---|---|---|---|
#9 | 913532-new-comments-forms-8.patch | 1.82 KB | David_Rothstein |
#1 | 913532-new-comments-forms-1.patch | 1.54 KB | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedsimplest patch to start.
Comment #3
sunNice catch! I guess we need to clear that static before and after invoking the hook then. Will you create a core patch?
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.
Comment #4
sunSo 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.
Comment #5
pwolanin CreditAttribution: pwolanin commentedWell, I think we need to do something to offer this kind of functionality in some fashion.
Comment #6
sunIt'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.
Comment #7
Dave ReidSeems like it might make a good 'Mollom auto-protect' module.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedHere'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.
Comment #11
sunSorry, trying to ensure a sane queue.
Comment #12
Alan D. CreditAttribution: Alan D. commentedSorry 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...
Comment #13
sunGood 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?
Comment #14
Alan D. CreditAttribution: Alan D. commentedBig kudos from me if you can find the time!
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.
Comment #15
andrewkillen CreditAttribution: andrewkillen commentedany 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....)
Comment #16
eshta CreditAttribution: eshta at Acquia commentedMollom 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.