Attached patch adds support for Profile module fields that are exposed on the user registration form.

User form tests are passing for me locally.

CommentFileSizeAuthor
#8 mollom-HEAD.profile.8.patch1.59 KBsun
#7 mollom-DRUPAL-6--1.profile.7.patch3.12 KBsun
#4 mollom-HEAD.profile.4.patch4.94 KBsun
#2 mollom-DRUPAL-6--1.profile.2.patch3.71 KBsun
mollom-DRUPAL-6--1.profile.0.patch4.1 KBsun

Comments

dries’s picture

dries commented

Nice and simple! I'm on a plane so I haven't been able to test this patch. However, the ease of integration is great validation for the new Mollom API.
The tests look good too, minus:

 +    // Enable Profile module.
+    require_once './includes/install.inc';
+    drupal_install_modules(array('profile'));
+    drupal_flush_all_caches();

Instead of doing that, let's do $modules[] = 'profile' in setUp please.

Two unrelated asides that might be worth creating issues for:

  1. It seems like we're approaching a point where it might be valuable to have mollom.user.inc, mollom.comment.inc, mollom.contact.inc and maybe mollom.node.inc. Of those, mollom.comment.inc is probably going to be the biggest one in the near time. Food for thought -- best left for another issue.
  2. For Drupal 7, it is weird that the form ID for the user registration form is user_register_form but that the form ID for the password request form is just user_pass (no _form suffix). At least, that is what it looks like from looking at user_mollom_form_info().
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom-DRUPAL-6--1.profile.2.patch3.71 KB

Alright -- if we want to use setUp(), then we should not make the existing User module integration tests slower. Therefore, attached patch adds a separate test case for the Profile module integration. That's clean and mean.

On the other, technically unrelated issues:

  1. Right now (even in Drupal 7), splitting the integration code for core modules would only lead to additional include files being loaded; i.e., a negative performance impact. I fully agree that in an ideal world, all of the integration code for a particular module should live in a dedicated include file, and that file should only be loaded if the module is actually installed. However, although Drupal 7 provides the new hook_hook_info() facility for such kind of optional module integration code, we cannot use it, because we are implementing the hooks on behalf of core modules, so $module.mollom.inc does not exist in the respective Drupal core modules (i.e., modules/comment/comment.mollom.inc).

    However, implementing hook_hook_info() to allow other contributed modules to put their optional support code for Mollom module into $module.mollom.inc would definitely make sense to do. Still unrelated to this patch though, so let's do that in a follow-up.

  2. Yes, the are still various inconsistent form constructor function names in Drupal 7. Actually, I think there has been an effort to rename all form constructors that do not end in _form, but it was blocked by people stating that the _form suffix is technically unnecessary and API changes cannot be reasoned by name consistency only; especially since this naming convention is neither documented nor enforced anywhere. I disagreed, but yeah, those counter-arguments are valid, too.
dries’s picture

dries commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks great now. Committed to DRUPAL-6--1. Thanks sun!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom-HEAD.profile.4.patch4.94 KB

Ported to HEAD. I couldn't believe it, but there are actually changes in D7's Profile module... LOL ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed to HEAD.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

dries’s picture

dries commented

Profile module had 'changes' or 'improvements'? :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.profile.7.patch3.12 KB

heh, "changes to merely make it compatible" probably cuts it. ;)

However, two issues:

1) I've learned that Profile in D6 supports a "list" field type, too.

2) The added tests are too vague and do not test each of the Profile fields separately.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom-HEAD.profile.8.patch1.59 KB

Profile tests pass. Same patch for D7.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Fixed

Committed those follow-ups to HEAD and D6.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit 4a47c6f on master, fai6, 8.x-2.x, fbajs, actions by sun: 
    #942834 by sun: Added Profile module support for user registration form...
  • Commit fc76aec on master, fai6, 8.x-2.x, fbajs, actions by sun:
    #942834 by sun: Added Profile List field type support and hardened tests...

  • Commit 4a47c6f on master, fai6, 8.x-2.x, fbajs, actions by sun: 
    #942834 by sun: Added Profile module support for user registration form...
  • Commit fc76aec on master, fai6, 8.x-2.x, fbajs, actions by sun:
    #942834 by sun: Added Profile List field type support and hardened tests...