Enable use of Mollom for any form
msn - April 12, 2008 - 00:18
| Project: | Mollom |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | sun |
| Status: | needs review |
Description
I would like the integration of the guestbook and tellafriend modules.
But,... maybe the sollution the captcha module has would be nice, the administration mode, giving the admin a "search form and set captcha" option.
#1
IMHO the CAPTCHA module does take the Drupal Way in allowing every form to be CAPTCHA-scanned, not just the preset ones. This feature request seems like a good one to me as it allows Mollom to provide more complete security.
#2
#3
I think that currently this is the #1 reason preventing a more widespread Mollom use (along the 'allow another fallback strategy' issue). Any development on this?
#4
Subscribing
#5
+1.
The mollom module also suggests its as easy as adding a mollom_$form_id variable in the comments for mollom_validate. However even that functionality doesn't work. This is a fairly big bug if you use any other sort of form other than core drupal forms for content entry.
#6
Check this patch for mollom + guestbook.
http://drupal.org/node/302941
#7
@lismail I guess that just solidifies my point. We shouldn't have to patch mollom for every contrib module :-D
#8
I like the way the CATPCHA module allows you to install the challenge on any form you have.
I would not move to Mollon unless this is also possible as it would leave many of my forms exposed to spam.
#9
Ideally, we'd be able to add Mollom to all forms in Drupal. For this to happen it would be ideal, yet not strictly necessary, if:
All that said, we can probably get away with a "less than ideal" solution like the CAPTCHA module uses.
Three days ago, we committed a reasonably big change to the Mollom module which should make it easier to add Mollom to more forms. I'm hopeful that we can add support for more, if not all form, to a future release of Mollom. If anyone wants to help with that, please do. I'm happy to provide direction, assistance and testing.
#10
Just a suggestion: would the form store module take care of Point #1?
#11
Updating the version. Let's try to fix this in CVS HEAD of 6.x first, then consider backporting it to Drupal 5.
#12
I was thinking of just making mollom more configurable by using a hook. In _mollom_protectable_forms use:
$forms = module_invoke_all('mollom')
then implement "on-behalf" hooks for Drupal core modules. And then each module that wanted to expose their form to mollom could implement a hook_mollom.
#13
Here's the patch...
#14
slick
+1
#15
This looks like a good step forward.
This would also require the module to prepare the data ... right now we use:
<?php$function = 'mollom_data_'. $form_id;
if (function_exists($function)) {
$data = $function($form_state['values']);
}
?>
... in the module but I wonder if that needs to be made more solid / integrated in the API. It seems like there should be a field 'data' that points to a data preparation function?
'data' => 'mymodule_mollom_data',Thoughts?
#16
I appreciate that D6 is the start right now, but I have a paying user who needs 5.x support now.
#17
I've added the suggestion from Dries to the patch found in #13 as well as an addition of a drupal_alter so that modules can change what other modules defines.
#18
drupal_alteris not available n 5.x; how necessary is it to this patch?#19
Well - it makes it easier to do modification of added form specifications which is good practice. As I understand a patch is first made against Drupal 6 and then backported to Drupal 5 where either drupal_alter could be removed or replaced with other code.
#20
FWIW, this works for me in D6. I don't know about D5.
In settings.php
<?php// Set a variable to tell Mollom that my form needs protection.
// The key is "mollom_" . $form_id
$conf['mollom_panels_comment_form'] = 2;
?>
#21
Doing that enabled Mollom on a form, but how does the data in the fields of the form get mapped to the fields needed for Mollom to submit the data for analysis?
You need a function mollom_data_form_id($form_state) implemented to handle the data mapping.
#22
Here's the code that I've been working on as a part of #412760: User interface brainstorming to enable Mollom for any form. Since this is going to require at least a few major underlying changes, I'll be making my own little local Mollom fork to post as diffs to the latest version.
#23
What if we could integrate this with the validation api module? IT has the nice feature of being able to validate any form item in drupal.
http://drupal.org/project/validation_api
#24
We could probably supply a validator hook? the users could set on the form items of their choosing.
#25
Anyone want to weigh in on the validation api route?
#26
My preference is to not require other modules if at all possible. Plus keep in mind that there are 5.x sites that use Mollom as well.
#27
yes that's true, I was just looking closer at the two module's code. I don't THINK that validation API has the ability to add form elements, just works through the $form['validate'] hooks. It also seems focused around fields, while mollom deals with the form as a whole. People might want to check the validation api code though anyway.
#28
I'd also prefer not to rely on other modules at this point. Let's bring the discussion back on topic. We can discuss the validation API module in another issue if desired.
#29
Bringing the conversation back on topic, what's wrong with the original patch or the updated patch? I'm using this on several Drupal 5.x sites right now.
#30
The hook is the way to go here.
I am working on integrating webform and mollom. Populist wrote a patch (#259488: Webform support) to do this. However it works by making mollom webform aware. The work done is great, but this is a slippery slop and isn't scalable for contrib modules.
Another issue I ran into, is that webform is can create multistep forms. Right now, _mollom_get_mode($form_id) works solely on the $form_id. It should also be passed the $form_state so the mollom protection can be conditional (i.e. show only on the last page). Perhaps as we declare a protectable form, we can declare a 'check' function in the protectable forms to run before simply passing back the $form[$form_id]['mode'].
so webform would declare:
$form['webform_57'] = array(
'name' => 'webform_57',
'mode' => MOLLOM_MODE_CAPTCHA,
'check callback' => 'webform_mollom_check'
'check arguments' => array(57, $form_state);
);
This would call webform_mollom_check and for example return MOLLOM_MODE_DISABLED unless we are on the last page.
I am proposing:
I am working on a d5 site right now, so I don't want to rollout any patches. We should develop on d6 and then backport. But I wanted to start discussing some ideas.
And for convenience here is the hook patch for drupal5 (there was some formatting that prevented it from applying). I also removed the drupal_alter line.
#31
Hi all,
I'm the maintainer of the CAPTCHA module and I made a proof of concept module to integrate the Mollom CAPTCHA with the CAPTCHA module and its admin UI workflow, so the Mollom CAPTCHA can be added to virtually any Drupal form.
Some notes:
;). If there is interest in captcha_mollom_integration, it's a no-brainer to merge it with cvs.drupal.org.
#32
Subscribe.
#33
Subscribing.
#34
Subscribing
#35
A huge +1 on Mollom addressing the "apply to any form" by integration with CAPTCHA. It would be nice for Mollom to provide integration the other way as well, by allowing different CAPTCHAs to be used when analysis fails. And CAPTCHA module should be able to implement support for Mollom generically enough that other services could potentially be substituted (or even fall back on, say, Akismet if Mollom is down).
#36
I had no idea none of this worked, I kind of just assumed it did, until I found out the opposite about 8 minutes ago. Subscribing!
#37
subscribing
#38
subscribing
#39
Subscribe.
#40
I think this deserves to be critical.
#41
@mcrittenden: I completely agree, but let's leave it at normal for now. I'm devoting the rest of today and tomorrow to actually getting this any-form integration code working. I'll be posting patches for review and screenshot of the new interface.
#42
Excellent! I'll be glad to review when you have something ready.
#43
Initial patch with most of the interface work done. You can select forms to be protected, select fields for analysis. Still bugs to work out, but it also shows up the CAPTCHA correctly on initial quick tests. There is also some extra stuff in the patch that's just me playing with how to work the admin interface for this.
#44
Looking good so far. I like the simplicity of the UI. Haven't had a chance to do a functionality review so far. Hopefully soon.
Thanks for the work on this. I know a lot of people are pretty excited about this getting in there.
#45
Subscribe.
#47
Subscribing.
#48
Subscribe.
#49
Subscribe.
#50
Yep - good stuff! Subscribed.
#51
Subscribing
#52
Will be making the second half push tomorrow through this weekend to get this accomplished. Stay tuned.
#53
subscribe
#54
Subscribed.
I absolutely love Mollom but we need to use Webform for our site-wide contact form! The CAPTCHA module provides a couple of unobtrusive possibilities, but I'd rather have NO captcha using Mollom's text analysis.
#55
Hi Dave, any updates as regards the comment:
http://drupal.org/node/245682#comment-2020772
I have a couple of production sites being attacked by spam. Is the patch attached with the above referenced comment stable enough to be used on a production site??
Also, are there any special steps to be kept in mind while applying this path on a Windows machine??
#56
@r_honey: No it's not ready yet. There's a few minor bugs and I need to update all the tests in the module to be sure I don't find any more.
#57
Subscribing....
#58
I would be waiting eagerly for the stable release.
#59
subscribe
#60
I would also be very interested in this.
Subscribing.
#61
subscribing... I'll have a look at it later on.
#62
Subscribing...
#63
Going by the number of subscribe comments I see on this issue, and on various others, I would seriously like the Drupal team to enable "Post/Comment reply notifications" feature for some parts of Drupal.org (specifically forums/issues).
It might require capability for sending significant amount of mails daily (or even per hour), but it will atleast keep the nodes clean, and focused on the topic at hand.
#64
subscribing
#65
You are basically implementing a custom version of Form controller here.
The particular problem applies to all modules that want to attach something to forms, potentially to all forms, but in a user-configurable manner. The idea of Form controller module is to introduce a unique entry point for all modules to build a UI for doing so, and optionally configuring further options for the particular form alteration of the implementing module. In other words, "Form controller" could have also be named "form_alter_ui" module.
Advancing on that paradigm and abstracting it a bit more, then you can say that Field API in D7 does the same in green: Given a certain entity id, we attach either fields to the object, or form widgets to a form. Form controller does the same, but just not targeting the object behind the form, but the form itself. The "entity" is the form, and the "id" is the $form_id. Contrary to Field API, however, it is an extra conditional controller on top of a form alteration process that may attach itself to _all_ forms by default, depending on the use-case and default configuration values of the implementing module. As mentioned on its project page, Journal module is such a use-case that, by default, attaches itself to all forms on a site, but can be configured per form, whether it will be attached and whether it will be a required field or not. Extra benefit: In case Form controller is not installed, the implementing module just uses its default configuration (just like Mollom does now), so it is a totally optional dependency.
To attach its own UI to configure form attachments, Form controller basically follows the pattern we will introduce with #473268: D7UX: Put edit links on everything in D7, which you already know from Views' admin links. Here, the to-be-configured thing simply isn't a block, node, or view, but a form. Configuring the form means to enable/disable/configure all optional form attachments provided by implementing modules.
Yes, it is still D5 and didn't see much traction so far due to my other obligations. However, instead of implementing yet another custom approach here, I'd really like to see us joining forces to implement a solution that benefits a full range of other contributed modules. It definitely needs a rewrite with less nifty jQuery stuff (which was a bad idea anyway), but overall it shouldn't be hard, and I'd be very happy to grant CVS access to you, Dave, or others.
Please take this into consideration, thank you. You know where to find me. :)
#66
tha_sun: Yeah, this entire approach is very hacky, having to build forms ourself and get fields, etc. Having a default simple interface with the option to be able to control any forms with a generalized solution is a very interesting and attractive approach instead of trying to re-invent stuff. Definitely want to look into this some more.
#67
subscribe
#68
subscribe
#69
Hi Dave, can you specify a timeline in which the updated Mollom project would be made available??
If it would take time, can I request you to give me an idea for how to patch the Guestbook module for integration with Mollom. I have site guestbooks open for Anonymous commenting (that's why they are for, isn't it).
But they are being flooded by Spam heavily!!!
#70
subscribe
#71
Subscribing (interested in using Mollom with the feedback module)
#72
subscribe
#73
#65: nice explanation.
#74
subscribing
#75
subscribe
#76
Hi Dave, it has been quite some time since any update for this issue. Is there any chance of getting this fixed in the near future??
#77
Subscribing. Need to protect webforms.
#78
Subscribing with all my heart. I badly need it for the Guestbook.
#79
Subscribing...
#80
Subscribing
#81
I just tested this with a webform and it doesn't work. I get the error "An illegal choice has been detected. Please contact the site administrator." after selecting the webform field I want to base textual analysis on. I think this is caused by the fact that the checkbox values are like
submitted[contact_us][last_name]and is treated as array? When I add debug print statements to form.inc, I see that the$elements['#value']is structured likeArray ( [submitted[contact_us] => Array ( [name] => submitted[contact_us][name] ) ).Other than that the patch looks like a good step in the right direction. Though perhaps some help text should be added to the "Fields to use for textual analysis" config option to explain what types of fields are good candidates.
Cheers,
Stella
#82
I'm working on a clients site, who is using and more importantly paying for two mollom subscriptions. I've just found a email out form, which is custom written, and we believe is sending out spam. I was thinking, no problem, we've paid for mollom, it will fix this problem. Should be as easy as the captcha module. Right?...
Turns out, there's no easy way to do this. People have been talking for 20 months about adding feature support which is similar to captcha, but still as far as I can see, it still doesn't exist. It really shouldn't take more than a couple days to get it done. Perhaps the profit motive of allowing all the free subscriptions to increase the amount of captcha which are processed, is not allowing this to go through. I can understand that, but what about your paying customers?
This combined with the other issues I've run into on my first project with mollum have been very disappointing and it will be hard to me to recommend it to other paying clients. I've never had a problem with the Captcha module and it seems to get rid of spam pretty well.
So far the only benefit I've seen from mollom has been prettier and easier to read captchas. Hardly worth the couple hundred dollars paid to mollom.
</feet-to-fire-rant>
#83
For those interested in adding mollom to any form programatically, it's pretty easy to do.
<?phpfunction yourmodule_form_alter(&$form, $form_state, $form_id) {
switch($form_id) {
case 'spammy_form':
$form['my_spam_gollum'] = array(
'#type' => 'mollom',
'#mode' => MOLLUM_MODE_CAPTCHA, // or MOLLOM_MODE_DISABLED, MOLLOM_MODE_ANALYSIS
'#weight' => 9999, // sink it to the bottom
);
break;
}
}
?>
Also the solution in #20 should work from the tracing of the code I did.
Or you could simply:
variable_set("mollom_FORMID", 2); // 0 is disabled, 1 is captcha and 2 is mode analysis.
It would be nice is these kind of developer snippets and information was made easily available in the Drupal Tutorials: http://mollom.com/tutorials/drupal
#84
Tip on #43: if you apply it to a site with Mollom installed, you need to uninstall Mollom first, as the module needs to create an extra database table.
I tested it against Mollom 6.x-1.10, patch works, ignore the 1 reject.
Can't add any fields for textual analysis as I get "illegal choice detected". But it displays a captcha.
#85
Any updates from the official Mollom maintainers?? People are having trouble out there man..
#86
Subscribing
#87
Yep we're still actively working on this. Keep posted and I'll try to post a revised patch around this weekend.
#88
+++ mollom.admin.inc 9 Sep 2009 04:27:56 -0000@@ -6,6 +6,215 @@
+function mollom_get_available_forms() {
Right now we have functions that are a bit confusing: mollom_get_available_forms(), mollom_get_forms() and mollom_get_protected_forms(). It was not immediately clear how mollom_get_forms() is different from the two other functions.
+++ mollom.admin.inc 9 Sep 2009 04:27:56 -0000@@ -6,6 +6,215 @@
+ $forms = mollom_get_forms();
+ $query = db_query("SELECT fid FROM {mollom_form}");
- I had to dug around a bit to understand why mollom_get_forms() and the query returned different results. One is all the forms that could be protected with Mollom, the other one is all the forms that are actually protected by Mollom. We might be able to write that a bit more cleanly. Anyway, at first glance, this function was a bit funny.
- This could probably be simplified. There is currently no code inserting rows in the {mollom_form} table. We can probably leave that out for now, and revisit this in the context of form_controller module as a follow-up step?
+++ mollom.admin.inc 9 Sep 2009 04:27:56 -0000@@ -6,6 +6,215 @@
+ l(t('Edit'), 'admin/build/mollom/protect/' . $form->fid),
I'd rename 'Edit' to 'Configure'. It is closer to configuration than to editing, IMO.
+++ mollom.admin.inc 9 Sep 2009 04:27:56 -0000@@ -6,6 +6,215 @@
+ $rows[] = array(array('colspan' => 4, 'data' => l(t('Add form protection'), 'admin/build/mollom/protect')));
Add a @todo to use local tasks in Drupal 7.
+++ mollom.admin.inc 9 Sep 2009 04:27:56 -0000@@ -6,6 +6,215 @@
+function mollom_autocomplete_form_ids($string = '') {
+ $forms = mollom_get_available_forms();
I'm not sure why we need the auto-complete. Doesn't mollom_get_available_forms() returns all forms that are explicitly made available through the new hook_form_info() using human-readable names? We could consider removing this for now, and revisiting this when we discuss form_controller support in a next step.
+++ mollom.admin.inc 9 Sep 2009 04:27:56 -0000@@ -6,6 +6,215 @@
+function mollom_protect_form(&$form_state, $mollom_form = NULL) {
+ if (!isset($form_state['storage']['step'])) {
+ $form_state['storage']['step'] = $mollom_form ? 2 : 1;
+ $form_state['storage']['mollom_form'] = $mollom_form;
+ }
+ else {
+ $mollom_form = $form_state['storage']['mollom_form'];
+ }
Instead of using a multi-page form, it would be nicer if we dynamically refreshed the page using Javascript when selecting a form. The multi-step form experience always feels awkward to me.
+++ mollom.form.inc 9 Sep 2009 04:27:56 -0000@@ -0,0 +1,98 @@
+function node_form_info() {
+ global $user;
+ $account = $user; //drupal_anonymous_user();
+
+ $node = array(
+ 'uid' => $account->uid,
+ 'name' => isset($account->name) ? $user->name : '',
+ 'language' => '',
+ 'vid' => 0,
+ );
+
The fact that you have to specify those parameters (e.g. $node) is somewhat unfortunate. I know it is required for drupal_retrieve_form() but maybe we can solve this differently so hook_form_info() can be a bit more simple/elegant. Making this API simple is valuable because it makes it easier for people to add support for their module(s).
+++ mollom.form.inc 9 Sep 2009 04:27:56 -0000@@ -0,0 +1,98 @@
+ 'parameters' => array(
+ node_load($webform->nid, NULL, TRUE),
+ array(),
+ TRUE,
+ FALSE,
+ ),
See above. Here as well, it would be nice if we could get rid of 'parameters'.
+++ mollom.form.inc 9 Sep 2009 04:27:56 -0000@@ -0,0 +1,98 @@
+ $forms['comment_form'] = array(
+ 'title' => t('Comment'),
+ 'parameters' => array(
+ array('nid' => db_result(db_query_range(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.status = 1"), 0, 1))),
+ ),
I'm not sure I understand the purpose of the SQL query.
+++ mollom.module 9 Sep 2009 04:27:56 -0000@@ -94,6 +94,49 @@ function mollom_menu() {
+ 'title' => 'Edit form protection',
Configure form protection?
+++ mollom.module 9 Sep 2009 04:27:56 -0000@@ -123,6 +176,71 @@ function mollom_menu() {
+ $field = str_replace(']', '', $field);
+ $field = explode('[', $field);
This code looks unnecessary because we implode() the stuff away later. Can probably be simplified.
+++ mollom.module 9 Sep 2009 04:27:56 -0000@@ -123,6 +176,71 @@ function mollom_menu() {
+ $value = isset($value[$key]) ? $value[$key] : NULL;
We need to convert the value to its final HMTL output. That is, if the value comes from a textarea with an input format, we want to run the filter before sending the data to Mollom. Mollom does not understand all possible input formats, and expect the final HTML output. Should be a lot easier in Drupal 7. Might be doable in Drupal 6. At a minimum, there should be a @todo for Drupal 7.
+++ mollom.module 9 Sep 2009 04:27:56 -0000@@ -123,6 +176,71 @@ function mollom_menu() {
+ $data['post_title'] = NULL; // @todo Will this still be ok?
We should try to set 'post_title'.
+++ mollom.module 9 Sep 2009 04:27:56 -0000@@ -123,6 +176,71 @@ function mollom_menu() {
+ $data['post_body'] = $fields_data ? implode("\n", $fields_data) : NULL;
The problem with this code is that it duplicates information in 'post_body'. If an 'author_mail' is provided, it is also set in 'post_body'. Not the end of the world, but it could negatively affect the classifier. It is better to exclude known fields from 'post_body'.
+++ mollom.module 9 Sep 2009 04:27:56 -0000@@ -123,6 +176,71 @@ function mollom_menu() {
+ // Add common data.
+ $data['author_name'] = isset($values['name']) ? $values['name'] : (isset($user->name) ? $user->name : NULL);
+ $data['author_mail'] = isset($values['mail']) ? $values['mail'] : (isset($user->mail) ? $user->mail : NULL);
+ $data['author_url'] = isset($values['homepage']) ? $values['homepage'] : NULL;
+ $data['author_openid'] = $user->uid ? _mollom_get_openid($user) : NULL;
+ $data['author_id'] = $user->uid ? $user->uid : NULL;
+ $data['author_ip'] = ip_address();
This could be incorrect. $values['name'] is not guaranteed to be the 'author_name'. Not sure how to fix that. One possible solution is to manually maintain a mapping in hook_form_info().
#89
Working on this.
#90
First of all: Straight re-roll of Dave's patch (without some unrelated change to the admin settings form).
#91
subscribing.
#92
Hrm. This approach and the overall requirements are more complex than I thought. I spent the last couple of hours to entirely revamp this patch.
First of all, there are only two valid approaches to accomplish the goal:
A mixture of both approaches will #fail. Just to make this clear.
Since we want to go with 1) and leave the generic approach for later, we have to properly define forms + fields in info hooks.
By providing a proper hook architecture (like D7 does in many areas now), all modules are able to hook into anything and enhance any field information provided by whatever else.
Attached patch implements this.
aww. That's quite a patch. :)
Will continue tomorrow.
#93
Thanks sun! Did a quick/initial review:
+++ mollom.admin.inc 20 Nov 2009 00:18:01 -0000@@ -7,7 +7,226 @@
+ $form['mollom']['form_id_custom'] = array(
+ '#type' => 'textfield',
+ '#title' => t('Custom form ID'),
+ '#size' => 25,
+ );
I'm still not sure I fully grok the custom form ID support. It seems to be at odds with the _mollom_form_info() hook.
+++ mollom.api.php 20 Nov 2009 00:16:15 -0000@@ -0,0 +1,58 @@
+ * An associative array containing information about forms that can be
+ * protected. Each key is a $form_id whose value is an associative array:
+ * - title: The human-readable name of the form.
+ * - mode: (optional) The default protection mode for the form. If omitted,
+ * the form will not be configured upon installation of Mollom module.
+ * - parents: (optional) An associative array of elements in the form that are
+ * available and can be configured for Mollom's text analysis. Each key
+ * forms the form API parents of the element in the form and the
+ * corresponding value contains the form element label. For nested elements
+ * a key of "parent][child" can be used. If omitted, Mollom can only provide
+ * a CAPTCHA protection for the form.
- Yes, please. Not all forms can be protected with text analysis, so giving module authors the option to force a CAPTCHA makes total sense. For example, you can't protect the user registration form with text analysis. There just isn't enough data to analyze.
- I think it also worth noting that it allows you to exclude fields. Imagine a site that asked for a social security number or credit card number. They probably don't want to send that to Mollom.
+++ mollom.api.php 20 Nov 2009 00:16:15 -0000@@ -0,0 +1,58 @@
+ 'parents' => array(
I was wondering about the name 'parents'? I was expecting 'fields' or something. I haven't reviewed the entire patch yet, but the term 'parents' doesn't make sense yet. Maybe it will become clear further down.
+++ mollom.form.inc 20 Nov 2009 00:19:33 -0000@@ -0,0 +1,118 @@
+ 'message' => t('Message'),
Shouldn't 'message' be 'body'? Or do we map 'message' to mollom.postBody as well?
+++ mollom.form.inc 20 Nov 2009 00:19:33 -0000@@ -0,0 +1,118 @@
+ 'message' => t('Message'),
Same as above. Shouldn't 'message' be 'body'?
+++ mollom.form.inc 20 Nov 2009 00:19:33 -0000@@ -0,0 +1,118 @@
+ if (module_exists('content')) {
+ $content_info = content_types($type->type);
+ foreach ($content_info['fields'] as $field_name => $field) {
+ $parents[$field_name] = check_plain(t($field['widget']['label']));
+ }
Not sure if we want all fields. We should probably limit it to text fields (i.e. textfields and textareas). We don't necessarily want dates, chexkboxes, radio buttons, etc.
This review is powered by Dreditor.
#94
Alrighty.
- Removed support for custom form ids, because that indeed makes not really sense. (carried over from original patch)
- Reworked hook_mollom_form_info() documentation.
- Limited CCK fields to fields of text.module.
Clarification:
- For Contact module's forms, the posted message lives in a form element 'message' and that is the form element we want to process. All selected fields from the available form elements are processed and concatenated into 'post_body'. However, I now realize that a potential 'title' field will be merged into 'post_body' too, if selected. Hmmm. So we need to extend the form element mapping a bit. :-/
- The key 'parents' maps to form API's key '#parents'. I chose this key name, because we are using the same approach as form_set_error() to handle nested form elements. We cannot use 'fields', because we use that key to store the enabled fields already. I actually like to re-use existing concepts that developers should already know. If you find it totally awkward, then we could rename it to 'elements'.
#95
Forgot to remove the JS for custom form ids as well.
#96
- Re-rolled against HEAD.
- Fixed/removed some first @todos.
#97
I installed the module on a Drupal 6 site from scratch (no upgrade) and I got:
Fatal error: Call to undefined function mollom_get_form_info() in mollom.install on line 98. Haven't been able to debug that yet, so just reporting my error for now.#98
Yup, fixed now.
This one passes all existing tests.
#99
This time for real. Had to borrow myself a testing server to resolve the test failures iteratively...
#100
Desperately trying to find time to test this properly. Thanks SO MUCH for your work on this sun.
#101
Making the installation defaults work properly.
#102
- Renamed 'parents' to 'elements'.
- Introduced new 'mapping' to declare optional 'title', 'name', 'mail', and 'url' (homepage) elements in registered forms.
- Removed old data processing implementations.
- Fixed the entire implementation by adding a test to ensure data processing. Took some hours to figure out the proper shifting around of submitted form values in combination with configured fields, in combination with mapping certain fields to certain data properties and omitting them, and in combination with mapping all remaining form values to eventually further data properties. ;)
The only remaining @todo is:
// @todo How do we get invisible & dependent 'format' support in here?#103
+++ mollom.api.php 20 Nov 2009 22:11:53 -0000@@ -0,0 +1,68 @@
+function hook_mollom_form_info() {
'mapping' is not documented.
+++ mollom.form.inc 21 Nov 2009 03:13:47 -0000@@ -0,0 +1,148 @@
+ 'mapping' => array(
+ 'title' => 'subject',
+ 'name' => 'name',
+ 'mail' => 'mail',
+ ),
(and elsewhere) Keys need to be updated, now using Mollom's data structure keys.
+++ mollom.form.inc 21 Nov 2009 03:13:47 -0000@@ -0,0 +1,148 @@
+ // @todo How do we get invisible & dependent 'format' support in here?
+ // $parents['format'] = t('Input format'),
Tinkering about this some more, I think this is obsolete. We do not want to _alter_ the user input in any way before spam detection, because that would potentially break Mollom's parser by either removing spam content (f.e. links) or adding content that could be considered as spam (f.e. inline macro tags that are processed into images containing 3rd party text contents and/or even links).
+++ mollom.install 20 Nov 2009 22:40:03 -0000@@ -53,6 +53,36 @@ function mollom_schema() {
+ // @todo 'module' required for maintenance.
This would be nice, but technically not strictly required - until D7. Because, without hook_modules_uninstalled(), we have no trigger to delete configured forms. So all we could do here is preparation for D7...
+++ tests/mollom.test 21 Nov 2009 03:03:29 -0000@@ -810,3 +899,93 @@ class MollomResellerTestCase extends Mol
+ $this->assertEqual($data['post_title'], $values['subject'], t("@key: '@data' is equal to '@value'.", array(
+ '@key' => 'post_title',
+ '@data' => $data['post_title'],
+ '@value' => $values['subject'],
+ )));
Add a @todo here to remove those ugly, lengthy $message strings when latest D7 code has been backported to D6 SimpleTest.
This review is powered by Dreditor.
#104
Also, this:
...should be explained in PHPDoc of mollom_form_get_values().
#105
When I upgraded an existing Drupal 6 site with Mollom module, I got some warnings:
* notice: Undefined variable: forms in /Users/dries/Sites/mollom-6/mollom.form.inc on line 146.* warning: Invalid argument supplied for foreach() in /Users/dries/Sites/mollom-6/mollom.module on line 467.
* warning: array_merge_recursive() [function.array-merge-recursive]: Argument #2 is not an array in /Users/dries/Sites/mollom-6/mollom.module on line 476.
* warning: array_merge_recursive() [function.array-merge-recursive]: Argument #1 is not an array in /Users/dries/Sites/mollom-6/mollom.module on line 476.
* warning: array_merge_recursive() [function.array-merge-recursive]: Argument #1 is not an array in /Users/dries/Sites/mollom-6/mollom.module on line 476.
* warning: array_merge_recursive() [function.array-merge-recursive]: Argument #1 is not an array in /Users/dries/Sites/mollom-6/mollom.module on line 476.
#106
Here is a screenshot of the errors.
#107
With a clean install, I got the same errors. After installation, I can't configure any forms so I can't proceed with testing.
#108
+++ mollom.admin.inc 20 Nov 2009 22:37:20 -0000@@ -7,10 +7,227 @@
+ // Add a row to add a form.
+ if (empty($rows)) {
+ $rows[] = array(array('data' => l(t('Add form'), 'admin/settings/mollom/add'), 'colspan' => 4));
+ }
In Drupal 7 we should use a local task. If we do Drupal 7 todo's, this should be one.
+++ mollom.admin.inc 20 Nov 2009 22:37:20 -0000@@ -7,10 +7,227 @@
+ * Helper function to return registered forms suitable for #options.
It is not clear what 'suitable for #options' means. I'm sure it will become clear as I read more code, but reading from top to bottom, it isn't. Could probably be clarified a bit better.
+++ mollom.admin.inc 20 Nov 2009 22:37:20 -0000@@ -7,10 +7,227 @@
+function mollom_admin_unprotect_form(&$form_state, $mollom_form) {
+ $form['#tree'] = TRUE;
+ $form['form'] = array(
+ '#type' => 'item',
+ '#title' => t('Form'),
+ '#value' => $mollom_form['title'],
+ );
+ $form['mollom']['form_id'] = array(
+ '#type' => 'value',
+ '#value' => $mollom_form['form_id'],
+ );
It is unclear why we want to store/use anything else but the form_id here. Might become clear as I read more of the code. It is also unclear why we store it in
$form['mollom']['form_id']instead of$form['form_id'].+++ mollom.admin.inc 20 Nov 2009 22:37:20 -0000@@ -99,7 +274,7 @@ function mollom_admin_settings() {
- '#collapsible' => TRUE,
+ '#collapsible' => $keys,
Now that the keys are on their own settings page, I'd remove '#collapsible' and '#collapsed'.
+++ mollom.admin.inc 20 Nov 2009 22:37:20 -0000@@ -128,12 +303,15 @@ function _mollom_verify_key() {
+ return FALSE;
If we remove the '#collapsible' this might be unnecessary?
+++ mollom.api.php 20 Nov 2009 22:11:53 -0000@@ -0,0 +1,68 @@
+/**
+ * @file
+ * API documentation for Mollom module.
+ */
- 'mapping' does not seem to be documented yet in mollom.api.php. What if something is not in elements but it is in mapping? And vice versa, what if something is in elements but not in mapping? When is something concatenated to the postBody? Some fields like author_ip don't seem to be specified at all. That is, they are outside of mapping and elements.
+++ mollom.form.inc 21 Nov 2009 03:13:47 -0000@@ -0,0 +1,148 @@
+ * @todo Move this into Webform module.
We can probably leave it in mollom.module for now.
+++ mollom.form.inc 21 Nov 2009 03:13:47 -0000@@ -0,0 +1,148 @@
+ // @todo
Stale @todo.
+++ mollom.module 21 Nov 2009 03:13:29 -0000@@ -332,7 +392,9 @@ function mollom_form_alter(&$form, $form
+ '#mode' => $mollom_form['mode'],
+ '#fields' => $mollom_form['fields'],
+ '#mapping' => $mollom_form['mapping'],
It seems like sometimes we call this 'elements' and sometimes we call this 'fields'. I guess 'fields' might be a processed 'elements'?
+++ mollom.module 21 Nov 2009 03:13:29 -0000@@ -373,156 +435,184 @@ function mollom_form_node_admin_content_
+ // Allow modules to react on saved form configurations.
+ if (isset($status) && $status) {
+ if ($status === SAVED_NEW) {
+ module_invoke_all('mollom_form_insert', $mollom_form);
+ }
+ elseif ($status === SAVED_UPDATED) {
+ module_invoke_all('mollom_form_update', $mollom_form);
+ }
+ }
I'm trying to think of a use case for this ... feature creep?
+++ mollom.module 21 Nov 2009 03:13:29 -0000@@ -373,156 +435,184 @@ function mollom_form_node_admin_content_
+ // All optionally specified fields specified in $mapping must be excluded from
+ // $fields, because they are used in other data properties.
What is meant with 'used in other data properties'? Can you clarify in the comments?
+++ mollom.module 21 Nov 2009 03:13:29 -0000@@ -373,156 +435,184 @@ function mollom_form_node_admin_content_
+ // Add user data.
+ $data += array(
+ 'author_name' => isset($user->name) ? $user->name : NULL,
+ 'author_mail' => isset($user->mail) ? $user->mail : NULL,
+ 'author_url' => !empty($user->homepage) ? $user->homepage : NULL,
+ 'author_openid' => $user->uid ? _mollom_get_openid($user) : NULL,
+ 'author_id' => $user->uid ? $user->uid : NULL,
+ 'author_ip' => ip_address(),
+ );
This looks like a problematic change. If a site editor edits a spam comment, we'd send the site editor's user name, user ID, IP address along with the spam comment (instead of the original spammer's information). This would harm the site editor's reputation. We can't just send global user information along with someone else's content.
+++ tests/mollom.test 21 Nov 2009 03:03:29 -0000@@ -127,6 +127,78 @@ class MollomWebTestCase extends DrupalWe
+ protected function setProtection($form_id, $fields = NULL) {
- It looks like setProtection assumes that you are logged with the proper access rights.
- I was expecting setProtection() to give me control over the Mollom protection mode but that doesn't seem to be the case. That might be OK, just recording my expectation mismatch.
+++ tests/mollom.test 21 Nov 2009 03:03:29 -0000@@ -127,6 +127,78 @@ class MollomWebTestCase extends DrupalWe
+ $this->clickLink(t('Add form'));
Maybe that should be called 'Protect new form'.
+++ tests/mollom.test 21 Nov 2009 03:03:29 -0000@@ -127,6 +127,78 @@ class MollomWebTestCase extends DrupalWe
+ protected function delProtection($form_id) {
- Let's write deleteProtection() instead of delProtection().
- It looks like delProtection assumes that you are logged with the proper access rights.
+++ tests/mollom.test 21 Nov 2009 03:03:29 -0000@@ -127,6 +127,78 @@ class MollomWebTestCase extends DrupalWe
+ if ($exists) {
+ $this->drupalGet('admin/settings/mollom/unprotect/' . $form_id);
+ $this->assertText(t('Mollom will no longer protect this form from spam.'), t('Unprotect confirmation form found.'));
+ $this->drupalPost(NULL, array(), t('Confirm'));
+ }
We don't actually test that the protection is remove. We only test the confirmation page.
+++ tests/mollom.test 21 Nov 2009 03:03:29 -0000@@ -397,9 +469,10 @@ class MollomFallbackTestCase extends Mol
function testFailoverMechanism() {
+ $this->drupalLogout();
// Set the fallback strategy to 'blocking mode', so that if the failover
// mechanism does not work, we would expect to get a warning.
Odd change. If that is intentional, please add a line of documentation.
+++ tests/mollom.test 21 Nov 2009 03:03:29 -0000@@ -477,11 +549,13 @@ class MollomUserFormsTestCase extends Mo
+ $this->drupalLogin($this->admin_user);
+ $this->setProtection('user_pass');
+ $this->drupalLogout();
Given that this is a bit of a pattern, have you considered moving the login and logout functionality inside of setProtection()?
+++ tests/mollom.test 21 Nov 2009 03:03:29 -0000@@ -810,3 +899,93 @@ class MollomResellerTestCase extends Mol
+ $this->assertEqual($data['author_id'], $user->uid, t("@key: '@data' is equal to '@value'.", array(
+ '@key' => 'author_id',
+ '@data' => $data['author_id'],
+ '@value' => $user->uid,
+ )));
See above. This assumption is only valid for new submissions, but not for editing existing submissions.
#109
Fixed the installation/update errors. (Only happens with Webform module being installed, which I still did not try yet.)
Also fixed all other issues, which I won't comment on in the following...
It is one already. :) In D7, we actually do both: Local actions (just a 'type' change in hook_menu()), but in D7 we also output a table row like that, containing a link to add a form, in case there are no forms configured yet, so the table does not contain a table header only.
1) We only store the 'form_id'. The other form element is of #type 'item', only to display the form's human-readable title.
2) We cannot use $form['form_id'], because that element is already used by form API internals. For this unprotect confirmation form, $form['form_id'] is 'mollom_admin_unprotect_form'.
I already took some time to optimize that page in terms of UX. The form still contains the fallback method selection, too. Since both the keys and the fallback method selection have lengthy descriptions, it makes sense to use fieldsets here. Initially, only the key configuration is visible, in an uncollapsible fieldset. When the keys are verified and valid, then also the fallback method fieldset is displayed, while the key configuration collapses itself.
That's similar to the previous behavior on that page. As of now, the only difference is that the key configuration fieldset is not collapsible initially. Not sure whether uncollapsing/displaying both of them all of the time is better.
I think that a function like _mollom_verify_key() should return the status either way.
I know that you stated it multiple times already, but ideally, Webform should implement support for Mollom on its own, and Mollom should only implement support on behalf of Drupal core modules. That is, because http://drupal.org/project/webform exists in multiple versions, and contributed modules in general are free to alter their APIs, data structures, and form structures at any time.
Since Drupal's hook system only understands modules, but not module versions, Mollom would never know whether Webform's APIs may have changed, and when they change... fatal errors will be the result. For one, Mollom couldn't provide the same hook implemenation for different versions of Webform. Second, moving the hook to Webform module later on would be a pain of fatal errors for all users, because, unless users update both modules to new releases that contain the hook movement at the same time, the same function will be declared twice.
I have a lot of experience in this area, mainly because I did this mistake in a few of my modules already.
I'm pretty sure that quicksketch will be fine + happy to include this hook implementation.
'elements' was 'parents' before. We need different properties here, because one of them ('elements') holds the generally available form elements in a certain form, as registered via hook_mollom_form_info(). The other one ('fields') is our own property and holds the keys of the 'elements' that have been selected for textual analysis (if any). That is different information, so we need and want to use different properties.
Note that only 'fields' are stored in the database, because 'elements' are supplied by the hook implementation, and can certainly change over time (f.e. by adding a new CCK field).
It's hard to think of something, but let's design this API properly. Saves all of us from pain and work in the future.
Yes, I agree. But to be fair - that's not invented here:
-function mollom_data_comment_form($form_state) {
- global $user;
- $data = array(
- 'post_title' => isset($form_state['subject']) ? $form_state['subject'] : NULL,
- 'post_body' => isset($form_state['comment']) ? $form_state['comment'] : NULL,
- 'author_name' => isset($form_state['name']) ? $form_state['name'] : (isset($user->name) ? $user->name : NULL),
- 'author_mail' => isset($form_state['mail']) ? $form_state['mail'] : (isset($user->mail) ? $user->mail : NULL),
- 'author_url' => isset($form_state['homepage']) ? $form_state['homepage'] : NULL,
- 'author_openid' => $user->uid ? _mollom_get_openid($user) : NULL,
- 'author_id' => $user->uid ? $user->uid : NULL,
- 'author_ip' => isset($form_state['cid']) ? NULL : ip_address(),
- );
I would have ideas to resolve this, but that definitely belongs into a separate issue.
Yes, I considered to put drupalLogin() + drupalLogout() directly into the setProtection() and delProtection() first, and actually had the code like that before. However, after diving into mollom.test a bit more, I quickly figured that we could and should definitely add more centralized helper methods to perform administrative tasks that are common to more than one test. If login + logout would be contained in the helper methods, then we would needlessly login + logout + login + logout all over the time... which is why I figured that we can simply turn it around and let the caller ensure that we have administrative permissions before trying to perform any administrative actions.
That single "odd change" was caused by the structure of the testcase, but I streamlined that now.
All tests pass.
#110
Not sure what happened to my brain, but the @todo I added in the previous patch was nonsense. Sorry.
#111
meh, and I forgot to do the changes to the tests you mentioned, sorry.
#112
The tests I had written initially had a similar setProtection() but doesn't use any interface drupalGet, etc. Just messes with variables/db directly. If we want to test the interface, we can set up a specific test for that, but doing things directly helps keep the test quick and from unnecessary assertions.
#113
I need to dig into this a bit more but the old code tries to use the form values (i.e.
$form_state['name']) when available. So when editing a form from another user, we wouldn't use the global$userobject:- 'author_name' => isset($form_state['name']) ? $form_state['name'] : (isset($user->name) ? $user->name : NULL),- 'author_mail' => isset($form_state['mail']) ? $form_state['mail'] : (isset($user->mail) ? $user->mail : NULL),
At first glance, the new code always uses the global
$userobject and discards existing form values when editing something:+++ mollom.module 21 Nov 2009 19:23:08 -0000@@ -373,156 +437,224 @@ function mollom_form_node_admin_content_
+ $data += array(
+ 'author_name' => isset($user->name) ? $user->name : NULL,
+ 'author_mail' => isset($user->mail) ? $user->mail : NULL,
Ditto with ip_address(). In the old code we pass NULL in case we're editing an existing form:
- 'author_ip' => isset($form_state['nid']) ? NULL : ip_address(),In the new code we always pass ip_address():
+ 'author_ip' => ip_address(),In other words, it looks like a regression introduced by this patch. As I said, I need to dig a bit deeper into the code to make sure I'm not overlooking something, but at first glance it looks like a regression that should be tackled in this patch.
#114
As far as 'name', 'mail', etc. are concerned, the new code does the same as the old, due to:
$data = array();// Add the post body.
$data += array(
// Add data of mapped fields.
$data += $mapping;
// Add user data.
$data += array(
...as long as there is a field mapping. Otherwise, it falls back to $user, just like the old code did.
I think the entire fallback on $user might be wrong. Instead, we probably want to ensure that the form contains at least #type => value elements for data mapping purposes.
But then there is also the special case of author_ip... that's a bit hard. I currently can't think of any logic that could be applied to all forms. If some piece of content was posted by an anonymous user, then the only place where the IP is stored is in {sessions}, but {sessions} is cleaned over time, and well, we wouldn't have any way to get from uid 0 to the visitor's session id at a later time. (note, that's basically what http://drupal.org/project/visitor is about)
#115
As far as 'name', 'mail', etc. are concerned, the new code does the same as the old, as long as there is a field mapping.
Ah, in that case the mappings are still incomplete compared to what we have in DRUPAL-6 HEAD now. User IDs, OpenIDs and IPs are not properly mapped yet. User IDs, Open IDs and IPs provide critical information for Mollom -- it is more important than some other fields.
The problem only occurs when editing content. We could make the Mollom API and the backend such that when editing existing content, Mollom does not need to be passed all the fields. When a field is omitted, Mollom could maintain the old value of the field in the backend (i.e. without overwriting what was previously submitted). For example, unless you specify a new author IP when editing a comment, Mollom would maintain the old author IP on the backend. Of course, we need to send those updates with the proper Mollom session ID -- which we used to do, and hopefully still do. We'll want to double check -- I believe we have tests for that.
That would still require some changes to the patch because currently we're sending the wrong IP address instead of sending no IP address (or an empty IP address). However, it simplifies the problem. We'd need to figure out if we're editing a previously submitted entity (existing comment/node ID + existing Mollom ID), or whether creating a new entity (no comment/node ID yet + no Mollom ID yet). In case of the latter, we can probably rely on the global
$userobject. In case of the former, we should simply omit the fields that can't be modified through the UI.It would be good if we had an
assertXmlrpc()test function similar to theassertMail()test function. It is important to continuously verify that this all works correctly. Being able to inspect the actual XML-RPC messages would be invaluable to achieve this. I've created an issue for that at #639608: Provide assertXmlrpc() function to inspect and verify XML-RPC messages.#116
Ben correctly pointed out that sometimes, you do want the behavior in this patch; e.g. on a wiki page. sun, I'll want to thinker about this some more with Ben. Hold on. :)
#117
It would be an option for the Mollom-backend to preserve data in an existing session, and it would be technically feasible. This would however create the idea that Mollom always stores all session information for an indefinite time. This is both due to technical and privacy reasons not possible.
When giving some though on the possible edit-scenarios, these are the possibilities:
There as some things Mollom should do to take care of these scenarios:
The last scenario is toughest. Ideally the legitimate editor would, or, have an role which does not need Mollom checking, or, edit the content so it is not spammy anymore. Preserving the author information from the original post kind of conflicts with the other scenarios.
Food for thought...
#118
Just quickly posting my gut reaction. This means that we probably want
a) each form to specify a pointer to another potentially existing submitted form value (f.e. 'nid') to determine whether we're dealing with new or existing content.
b) based on a), we want to store the session data (as currently being done for nodes and comments) in {mollom}, and send this session data in case it exists.
c) for the (rather unusual) editing case of a wiki page, we want the form definition to be able to specify that each submission must be validated ('always validate' => TRUE).
d) to copy/move the user permission to post without validation into a user account setting, so it can be applied to individual, trustworthy users.
#119
subscribing
#120
Looking forward to it. Subscribing!
#121
The currently released version of the module does resubmit content to Mollom once it is edited. If the current patch does the same (it should or we have a regression), I think behavior of resubmitting the content using the global
$userobject is OK. It would work for people editing their own content, and it would work when people are making edits to a wiki page.The big question is what happens when an administrator upublishes some spam -- if a
statuschange submits the content to Mollom, then using the global$userobject is problematic. Mollom would think that user has just submitted some spam.One way to work around that is to extend
_mollom_form_info()to return a 'access' attribute. The 'access' attribute would be an array of permissions. If the users has one or more of the permissions set, we'd skip Mollom checking. Thus, we could instruct the module to by-pass permission checking for users with the 'administer comments' permission, or for users with the 'administer nodes' permissions. Like that, we'd never send something to Mollom when edited by an administrator.Either way, when a non-administrator edits a post, we should resubmit the content with the matching Mollom session ID. The current module does this, and this patch can't break that functionality.
Thoughts?
#122
- Removed 'mapping' column from {mollom_form}. Re-install required, if previous patches were tested.
- Added XML-RPC server communication tests through a fake Mollom server implementation.
- Added more tests.
And 3 assertions of the new tests are failing. That is, because Drupal core's Comment module sucks. I need to revamp Mollom's form validation process within this patch to properly support the dynamically defined form elements in comment_form().
Hence, this patch is just for reference.
#123
Created #642702: Form validation handlers cannot alter $form structure
#124
Posting something in between. I really wonder why Mollom doesn't use $form_state['storage'] to trigger automated caching + rebuilds of forms until validation passed?
Aside from that, I can confirm that the simple fix in #642702: Form validation handlers cannot alter $form structure eliminates a lot of trouble, so we should at least get that into D7.
#125
Getting closer.
Don't try this at home.
#126
Next one: #644150: $form_state is stateless, even with enabled caching
#127
Wow, that is a big rewrite. I thought we were relatively close so I'm curious what made you rewrite large parts of the form handling. Here are my initial comments after a first skim.
+++ mollom.api.php 21 Nov 2009 17:55:44 -0000@@ -0,0 +1,103 @@
+ // Mymodule's user registration form.
+ $forms['mymodule_user_register'] = array(
+ 'title' => t('User registration form'),
+ 'mode' => MOLLOM_MODE_CAPTCHA,
+ 'mapping' => array(
+ 'author_name' => 'name',
+ 'author_mail' => 'mail',
+ ),
+ );
When you use a CAPTCHA, you don't need to send Mollom any information. That said, it is OK to send information.
It might be worth mentioning that some data like author IP, user ID are also sent regardless of the presence of a mapping (unless I'm mistaken).
+++ mollom.form.inc 25 Nov 2009 13:53:11 -0000@@ -0,0 +1,153 @@
+ // @todo comment_form() dynamically uses different form elements for
+ // anonymous and authenticated users. This makes it impossible for
+ // Mollom to define a valid mapping. Drupal core sucks again?
Wow, major WTF. I didn't know this was the case. Mmm, I wonder if that triggers a bug in the current version of the Mollom module. Good catch.
+++ mollom.module 26 Nov 2009 17:53:58 -0000@@ -332,12 +399,60 @@ function mollom_form_alter(&$form, $form
+ echo "<pre>"; var_dump('form_alter'); echo "</pre>\n";
+ krumo($form_state);
Keekaboo! :)
+++ mollom.module 26 Nov 2009 17:53:58 -0000@@ -332,12 +399,60 @@ function mollom_form_alter(&$form, $form
+ '#session_id' => NULL,
+ '#form_id' => $form_id,
+ '#require_analysis' => $mollom_form['mode'] == MOLLOM_MODE_ANALYSIS,
+ '#require_captcha' => $mollom_form['mode'] == MOLLOM_MODE_CAPTCHA,
+ '#passed_captcha' => FALSE,
+ '#user_session_id' => session_id(),
#session_id is #mollom_session_id?
+++ mollom.module 26 Nov 2009 17:53:58 -0000@@ -567,47 +744,99 @@ function mollom_elements() {
+ * Format the Mollom form element.
+ */
+function theme_mollom($element) {
+ return isset($element['#children']) ? $element['#children'] : '';
+}
Eh? Odd.
+++ mollom.module 26 Nov 2009 17:53:58 -0000@@ -567,47 +744,99 @@ function mollom_elements() {
+ echo "<pre>"; var_dump(__FUNCTION__); echo "</pre>\n";
+ krumo($form_state['storage']['mollom']);
keekaboo!
+++ tests/mollom_test.info 25 Nov 2009 11:16:09 -0000@@ -0,0 +1,5 @@
+name = Mollom Test
'Mollom test'?
#128
Next one: #644222: comment_form() is incompatible with form caching *cough*...Mollom invalidates Form API...
Since all of those core bugs won't be fixed anytime soon in D6, I now need to post another fallback patch, before revamping it once more to work around the bugs.
However, when porting Mollom to D7, this patch should give some clues about how it should work, i.e. leveraging proper form caching, proper form storage, proper form validation, etc. :P
#129
Alrighty, after working a couple of days on this, I finally made it work (and finally crossed the 100K mark :P). Contains big phat block of @defgroup that (hopefully) explains all the nightmare.
Ten minutes ago, all tests were passing. However, I suddenly get 3 fails in fallback mechanism tests, which I can't explain (perhaps my host got blocked due to massive test requests? ;-)
Also note that some minor and one major review issues raised above still need to be incorporated.
The major one is to add
'entity_id' => 'parent][child'to the form information returned by hook_mollom_form_info(). Given that additional mapping, we can trigger the sending of an IP address. ...although I'm no longer sure whether it is still wanted to not send an IP address when editing stuff...?#130
Next one: #644648: comment_form() structure is inconsistent / $form['#token'] WTF
#131
Alright, my testing server indeed seems to be blocked or may have exceeded the limit of submissions or whatever...
To work around this, I've additionally implemented mollom.getImageCaptcha and mollom.checkCaptcha XML-RPC methods into the fake testing server, which makes the previously failing fallback mechanism test pass.
#132
One of the remaining issues is the question whether we want/need to fully restore the previous behavior regarding 'author_ip', which was only populated with the current user's IP address for _new_ posts, i.e. not having a nid, cid, etc. If it is absolutely required, then I would suggest to add a new 'post_id' key to the existing mapping fields. Thus, for example, the form definition for the node form would specify 'nid' here. And depending on whether this mapping key exists, and the value for the specified form element is non-empty, we would not send the current user's IP address.
Since hook_mollom_form_info() is invoked for all protected forms and all users, the form definition would even be able to conditionally define the additional mapping. E.g., only return it in the definition, if the current user does not have the 'administer comments' permission.
That, however, also triggers the question whether any data should be sent and validated for content administrators at all...?
...
Partially related to that is: Currently, the module implements hook_nodeapi() and hook_comment() to store the (successful) validation result received from Mollom, which is later on used for administrative "Report to Mollom" links. It needs to do that, because form submit handlers do not have access to the inserted or updated content object from the database. However, this means that for each form and module that should be protected, there also needs to be a corresponding mollom_$object_insert() implementation... Without the stored information, "Report to Mollom" is not possible.
I'm entirely not sure how this could be solved.
#133
Summary of Drupal core issues, which badly need support ASAP, because some of them are not back-portable:
#642702: Form validation handlers cannot alter $form structure
#644150: $form_state is stateless, even with enabled caching
#644222: comment_form() is incompatible with form caching
#644648: comment_form() structure and elements are inconsistent
I've already submitted first or even already complete patches to most of them. Please help to get them in.
But also, though less critical:
#639608: Provide assertXmlrpc() function to inspect and verify XML-RPC messages
#134
One of the remaining issues is the question whether we want/need to fully restore the previous behavior regarding 'author_ip', which was only populated with the current user's IP address for _new_ posts, i.e. not having a nid, cid, etc.
I think we want to implement it as discussed in #121: the big question is what happens when an administrator upublishes some spam -- if a status change submits the content to Mollom, then using the global $user object is problematic. To work around that is to extend _mollom_form_info() to return a 'access' attribute. The 'access' attribute would be an array of permissions. If the users has one or more of the permissions set, we'd skip Mollom checking. Thus, we could instruct the module to by-pass permission checking for users with the 'administer comments' permission though comment_mollom_form_info(), or for users with the 'administer nodes' permissions though node_mollom_form_info(). Like that, we'd never send something to Mollom when edited by an administrator, and we would send things to Mollom if a non-administrator user has edit own content rights. Sounds do-able?
#135
Partially related to that is: Currently, the module implements hook_nodeapi() and hook_comment() to store the (successful) validation result received from Mollom, which is later on used for administrative "Report to Mollom" links. It needs to do that, because form submit handlers do not have access to the inserted or updated content object from the database. However, this means that for each form and module that should be protected, there also needs to be a corresponding mollom_$object_insert() implementation... Without the stored information, "Report to Mollom" is not possible.
We store the Mollom session ID for two reasons: (i) for the administrative "Report to Mollom" links but also (ii) for when the content is edited. When content is edited, we need to send the session ID to Mollom so Mollom understands the old session was edited. Can you confirm that (ii) still works?
I don't think we can avoid the mollom_$object_insert() implementation unless Form APIs _submit handlers would be required to capture that data. I think it might be worth adding a sentence to mollom.api.php with regard to the need for a mollom_$object_insert() function.
#136
Next one: #645374: Make saved objects available to form submit handlers
#137
I'm reviewing all of the patches mentioned in #245682-133: Enable use of Mollom for any form to help push things forward. It is obvious that the Form API needs some tough love in Drupal 8.