Drupal 7 Module Development

Hey everyone,

I'm very excited to see this gathering momentum. With Janrain itself as the driving force behind this effort (and a lot of community involvement, too), I think we can create a fantastic version of the module for Drupal 7. I look forward to helping out in any way I can! :-)

Cheers,
Ben Kaplan

Great news. Really looking forward to following this project as well as assist with testing and any other way I can.

Hello fellow Drupalers,

I have completed most of the core module. The RPX token handler and
all related logic is there, short of the Engage-to-Profile data
mapping.

The code is heavily based on the Drupal 6.x RPX module. I ported it to
the new API and fixed some bugs (hopefully without introducing any new
ones :)).

As I am not an experienced Drupaler, I'm sure the code would benefit
from a peer review. I would be thankful for any comments and/or
patches.

Thanks,
George

Awesome, George. I'm looking forward to testing. I plan on doing so later this week.

Cheers,
Ben

Ben,

I was thinking more of a code review, since it doesn't have any
administration interface yet.

But you can actually test it, if you set the rpx_apikey variable
manually (using drush, perhaps).

I'm planning to upload the admin module in a day or so; it will be much
easier to test then.

Thanks,
George

Hey George,

Did you get the Acquia folks to do a code review for you? And can you upload the admin module? Then I can do some initial testing.

Thanks,
Ben

Hi Ben,

I plan to upload a complete Beta this weekend. I'll ask the Aquia
guys to take a look then.

Thanks,
George

Hi guys,

Here's the Beta, sorry for the delay.

Today I was surprised to learn that the Profile module (which is used
by RPX for the Janrain to Drupal user profile data mapping) was
deprecated a couple of weeks ago. It won't even show in the list of
available modules, unless you have already used it before. Please see
http://drupal.org/node/608894 for more information.

Since we still need to test, for those of you who are going to install
the latest D7 from scratch, I included a "fix" that resurrects the
Profile module in the module list (so that it can be enabled). In the
meantime, I'll update the Engage (RPX) code to also map to the default
user fields.

The PHP notices will go, I'm working on that. Other than that, please
test and let me know what you think.

Thanks,
George

Thanks for posting, George. I'm looking forward to testing. And it sounds like the Acquia guys should be very helpful with a code review.

One thought on the conversion from Profile module to user fields: In addition to supporting fields on the user entity, the Janrain Engage module should also support the Profile 2 module (http://drupal.org/project/profile2). This module creates a new fieldable 'profile' entity in Drupal 7.

I think the growing consensus is that for just a few fields that are very closely associated with the user account, you may want to store them as fields on the user entity. But if you will have more than just a few fields (with user information that isn't closely related to the Drupal user account), you'd probably want to store them via Profile 2's 'profile' entity.

Because Janrain Engage can potentially supply a lot of data about a user from a user's social networks (such as Facebook), it is important that Profile 2 fields be supported in addition to user fields. This way, if you want to store, say, 20 different fields about a user, you don't have to store all of them on the user entity.

The good news is that both the user entity and Profile 2's 'profile' entity are using the new Field API in Drupal 7. So supporting both options for field storage shouldn't be too difficult.

Thoughts?

--Ben

Ben,

I did not hard code the Engage profile data paths in the Beta. What I
did instead was allow the admin to enter the "path" to a data field
within the Engage dataset. This means that the extended profile data
offered by the Janrain Engage premium plans are automatically
supported, as well as any other data fields that Janrain may add in
the future.

So, in the same spirit, I could add both the user entity and Profile2
configured fields to the Engage-to-Drupal mapping configuration page
and let the admin decide which Engage fields go where. Please let me
know what you think.

Also, do we need to support the old Profile module? There could be Drupal
installations that still depend on it. I think we should, if only to
be consistent with the D7 upgrade path (i.e. if the module is already
used, leave it be, otherwise hide it).

Thanks,
George

I found that my previous Profile module resurrection "fix" could break
the D7 installation.

Now I just enable the Profile module when the Engage UI module is
installed. Of course, this hack is just for testing, and it won't be in the
final release.

Cheers,
George

Hi guys,

If you want to give it a try over the weekend, here is the latest
version (fixed some bugs).

I will check the module into the CVS soon, should be easier to
deploy/update.

Thanks,
George

Hi guys,

Committed the module to HEAD. A downloadable package should become
available within 12 hours.

Cheers,
George

d'oh, I just spent about 30 minutes reviewing janrain from HEAD, and then realized I was looking at the 6.x version.

So whatever you did, it didn't make it into HEAD.

oh, the CVS repo moved :( it's now called rpx, even though the name of the product has again be renamed?

I recommend naming the module what the actual name of the product is to not confuse people.

Jacob,

The janrain module (http://drupal.org/project/janrain) is a
different project (see http://drupal.org/node/919880).
And the Janrain's RPX product has been renamed
Janrain Engage a while ago, hence the confusion.

Cheers,
George

Overall, this is really stellar. It's a lot of functionality, and on the whole very well coded. Nice work. We're mid-sprint and under-the-gun so I don't have enough time right now to review it in entirety, but I went through most of the rpx_ui module at least and noted a few minor things.

General coding convention is to not add a linebreak at the top of every function. I noted a couple places with bad indentation too. Not really important, but check
out the coding conventions doc (http://drupal.org/coding-standards). Also, use the coder module (http://drupal.org/projects/coder) to help detect style errors.

Here's some more detail on my findings in 45 minutes or so of fooling around:

  $items['admin/config/people/rpx'] = array(
    'title' => 'Janrain Engage',
    'description' => 'Configure the settings for Janrain Engage',
    'page callback' => 'rpx_admin_settings',
    'access arguments' => array('administer site configuration'),
    'weight' => -4,
  );

Move this to a rpx_ui.admin.inc include file.

function rpx_admin_settings() {

This function seems like it could just be called from the form builder. So you could use drupal_get_form() and then from there call rpx_ui_check_apikey(). Or something. But I don't understand exactly what the function is doing, a line on the purpose of the function would help for those of us who don't know the RPX api intimately.

$form['setup'] = array('#markup' => t($text, array(
      '!create_rp' => RPX::create_api . '?' . http_build_query(array(
        'base' => $base_url . base_path(),
        'return' => url('admin/config/people/rpx', array('absolute' => TRUE)),
        'requestId' => $request_id,
      ))
    )));

Consider making a couple temporary variables instead of nesting all the parens together. IIRC, Drupal coding standards call for one paren per line if you start spanning lines.

$form['api']['rpx_realm'] = array(
      '#type' => 'item',
      '#title' => t('Engage Realm'),
      '#markup' => variable_get('rpx_realm', ''),
      '#description' => t('The Engage realm for this site (set automatically based on your API Key).'),
  );

Indentation

$path = variable_get('rpx_apikey', '') ? variable_get('rpx_admin_url', '') : '';

Redundant, $api_key is defined above.

$form['providers'] = array(
        '#type' => 'fieldset',
        '#title' => 'Identity Providers',
        '#collapsible' => TRUE,
        '#collapsed' => FALSE,
        '#description' => t('<a href="@setup_url" target="_blank">Configure/add providers here.</a> <a href="@update_url">Click here</a> to update your list of enabled providers from the Engage servers.', array('@update_url' => url('admin/config/people/rpx', array('query' => array('update-providers' => ''))), '@setup_url' => variable_get('rpx_admin_url', '') . '/setup_providers'))
    );

Indents.

rpx_admin_settings_submit()

Why not use system_settings_form() to simplify life here?

On the "Profile" tab I got this:

"Notice: Undefined variable: rows in theme_rpx_profile_settings() (line 483 of /Users/jacob/work/drupal-7/sites/all/modules/d7_modules/rpx/rpx_ui.module)." because I had no fields defined.

Also ['profile'][''] in the second text box. I think it makes sense to hide this UI until it can be made into a full-fledged GUI. I don't really understand how to use it.

Got this:

Notice: Undefined index: 1 in rpx_profile_settings() (line 406 of /Users/jacob/work/drupal-7/sites/all/modules/d7_modules/rpx/rpx_ui.module).

After saving a profile field.

When I set the "Drupal field" it went away, but then when I hit edit, I got an empty form.

Hi Jacob,

Thanks for your review and feedback.

One note about the "Profile" tab configuration UI: this is now very
flexible in the sense that users get access to all of the user profile
data fields that can be returned by Engage (and there are lots of
them). I don't see any other way to do it, short of hardcoding all of
them into the module (and of course I'm open to suggestions).

To help the user understand how to use it, I link to the Engage
dashboard tools, which are intuitive. I think we need some more beta
testing to see how it sticks with the users.

Thanks,
George

Hi George,

I've completed a first round of testing. As a first draft, it's looking good!

In this note, I'll address error messages, broken functionality, and other obvious bugs. Then, in a follow-up comment I'll address UI and usability. So here are my comments (I've added a letter to each bug so that they are easier to discuss):

A. When visiting the module settings page (admin/config/people/rpx), I'm getting the following error message:

Notice: Undefined variable: rows in theme_rpx_profile_settings() (line 483 of /Users/benkaplan/git/drupal-beta3/sites/all/modules/rpx-HEAD/rpx_ui.module).

This may be because there are no fields defined yet.

B. After actually creating a new mapping field, I get the following error message:

Notice: Undefined index: 1 in rpx_profile_settings() (line 406 of /Users/benkaplan/git/drupal-beta3/sites/all/modules/rpx-HEAD/rpx_ui.module).

C. Clicking the "edit" link next to the newly created field does not let you actually edit the field. Instead, it takes you to a page to add a new field.

D. The Profile field that was defined in field mapping did not successfully map. To test, I kept it very simple and followed your example. I tried to map ['profile']['verifiedEmail'] to a single-line textfield named "Profile Email". Nothing was saved in that field upon new user registration. Either the mapping isn't working right or it isn't clear how to properly configure the mapping.

E. I agree with JacobSingh that how to enter the Janrain Engage fields (['profile']['']) is very confusing. Is there any way to check a site admin's Janrain Engage account to see what fields he has access to? Other options include listing all of the possibilities in the help text. I also don't think it's terrible to hard code these options and then allow users a field to create their own if an option isn't listed. I'm guessing that the bulk of these fields wouldn't change their machine names.

F. I tried to link a Myspace account (that had been properly configured using a Janrain Engage free account) and got the following error message:

Notice: Undefined variable: token in rpx_token_handler() (line 54 of /Users/benkaplan/git/drupal-beta3/sites/all/modules/rpx-HEAD/rpx_core.module).
You need a token to be here!

In addition to this error message, I also received an "Access denied" on the page that loaded. This appears to be only a problem when linking a Myspace account.

G. When viewing a page that includes the Drupal login block (with Janrain Engage icons appended to it), I'm getting the following error message:

Notice: Undefined variable: icons in _rpx_user_login_form_alter() (line 68 of /Users/benkaplan/git/drupal-beta3/sites/all/modules/rpx-HEAD/rpx_widgets.module).

Note that the problem only occurs if the "Attach Engage Sign-In link to login forms" option is seleted.

H. On the new user registration page, I'm getting the following error message:

Notice: Undefined variable: _SESSION in rpx_core_form_user_register_form_alter() (line 183 of /Users/benkaplan/git/drupal-beta3/sites/all/modules/rpx-HEAD/rpx_core.module).

I. I tried to login via Facebook (when no account on the Drupal site yet) and got the following error message:

Notice: Undefined index: add_to_account in rpx_token_handler() (line 52 of /Users/benkaplan/git/drupal-beta3/sites/all/modules/rpx-HEAD/rpx_core.module).
You must validate your email address for this account before logging in with it.

J. When trying to connect two different Drupal site accounts to the same Facebook ID, I got the following fatal error:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'http://www.facebook.com/profile.php?id=1191339856' for key 'authname': INSERT INTO {authmap} (uid, module, authname) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => 43 [:db_insert_placeholder_1] => rpx_core [:db_insert_placeholder_2] => http://www.facebook.com/profile.php?id=1191339856 ) inuser_set_authmaps() (line 1951 of /Users/benkaplan/git/drupal-beta3/modules/user/user.module).

There should be some kind of error handling here because it is entirely possible that someone would try to register twice with the same Facebook ID. It also appears that the Facebook ID is being retained on the new user registration page for the registrant even after the registrant logs out of the Drupal site.

K. If the module settings are configured to force user registration, then I'm getting this error:

Notice: Undefined index: add_to_account in rpx_token_handler() (line 52 of /Users/benkaplan/git/drupal-beta3/sites/all/modules/rpx-HEAD/rpx_core.module).

L. When trying to use the "share" widget and clicking "Preview" on a comment, I'm getting the following error message:

Notice: Undefined index: HTTPS in rpx_widgets_comment_view_alter() (line 98 of /Users/benkaplan/git/drupal-beta3/sites/all/modules/rpx-HEAD/rpx_widgets.module).

I get this same error message when actually saving the comment.

Let me know if any of this is unclear or if you need further testing on any item.

Cheers,
Ben

Hey Ben,

Thanks for your feedback.

A. and B. Those are notices, they do not affect the functionality, but they shouldn't be there, so I'm getting rid of them in the next commit.

C. Oops. Noted.

D. What sign-in provider did you use? For example, Google and Facebook do return verified emails for 3rd party sign-ins, while Twitter doesn't. You can check what information they return using the sign-in test and provider configuration tool I link to on the "Add new field" page.

E. I agree that entering the paths is confusing. I think the best way would be to have an Engage server-side API that would return the information on all the fields currently available to the user, so that the module is always up to date. Or maybe we will have to make do with hard-coding them. I'll check with the Engage dev team.

F. That's weird. Thanks for the feedback.

G. and H. Notices again. Getting rid of them in the next commit.

I. That should only happen if you've already tried to login with Facebook and Facebook did not return verifiedEmail for your account. Can you login as admin and check if an account has been created for that login and there's a Facebook identifier in the "Janrain Engage Identities" tab for that account?

J. This is weird, as this is already being handled. The module should have just returned an error stating that the identifier has already been mapped to an account. The problem is that I can't reproduce the error. Can you give me the exact sequence of steps you took before you encountered it? Thanks.

K. Thanks, I'll fix these notices, too.

Thanks a lot for your input.

George

Hey George,

One additional possibility for doing the field mapping is to integrate with Token module. Most of the Token module is already in Drupal 7 core and the contributed version of Token module (http://drupal.org/project/token) adds some additional functionality like a Token browser (which allows you to see all available tokens and click on any token that you want to insert).

Additionally, take a look at the 'Fields token' patch at this thread: http://drupal.org/node/691078. This allows any field added through the Fields UI to be available as a token. So instead of having to integrate directly with User entity fields or Profile2 fields, it might be possible to integrate with Token and have any field in a Drupal 7 site (whether attached to a User, a Profile2 'Profile', or any other entity) available for data mapping.

To see how this might work from a simple UI perspective, check out the Drupal 7 version of RealName module (http://drupal.org/project/realname). It's using tokens (including Field tokens) to define a user's "realname".

Anyway, just an idea I wanted to bring to your attention since I wasn't sure if you're familiar with Tokens in Drupal.

--Ben

Hi Ben,

I will surely take a look at the Token module. I didn't know about it;
thanks for bringing it up.

BTW, I see that #501408: Display user fields on registration form
has been committed to Drupal core. Now it should be easy to add
support for User fields as well. I plan to add this in the next
commit.

Thanks,
George

Hi guys,

I added a path configuration tool based on the jQuery treeTable plugin (thanks Ben). Now creating/editing Engage profile fields should be a breeze.

Hey George,

Earlier today, I did some more testing of the latest -dev branch. In the notes below, I'm continuing my alphabetical to-do list from comment #21:

M. The "Janrain Engage identities" tab is showing up to users even when an API key has not yet been entered by the site admin. As a result, if a user clicks the link to "Add identity," they get a "Page Not Found" error on the Janrain site. The "Janrain Engage identities" tab should really not be appearing to users at all unless an API key is entered at admin/config/people/rpx.

N. A site admin may not want all users to see the "Janrain Engage identities" tab. There should be a permission for this at admin/people/permissions. I'd probably label the permission something like "Manage own identities".

O. While you are adding the permission in N. above, you may also want to add an "Administer Janrain Engage" permission as well. Having this permission would be required to visit the admin/config/people/rpx configuration page. Many modules have this type of administrative permission.

P. When visiting admin/config/people/rpx/profile, I'm getting the following notice at the top of the page:

Notice: Undefined index: post in rpx_profile_settings() (line 304 of /Users/benkaplan/git/drupal-rc1/sites/all/modules/rpx-HEAD/rpx_ui.admin.inc).

Q. At the top of the admin/config/people/rpx/profile page, the following message is printing: "At least one of the Profile (deprecated) or Profile2 modules must be enabled to map Engage data to Drupal profile fields." But I don't think this is fully accurate... If the site admin adds fields to the user entity, then they should be able to map fields even without those two modules installed. So some type of message is good, but we should be supporting profile, profile2, and user fields, right?

R. The new token browser is nice, but can you explain on the page the difference between "Profile data," "Merged Portable Contacts Data," and "OpenID Simple Registration Data"? I wasn't clear on this. There are so many available fields that I think we need a little better explanatory text.

S. On the module settings page, the tab labeled "Profile Fields" should probably be "Field Mapping". Otherwise, it sounds like it is only meant for the old (deprecated) profile module. The help/explanatory text on this page also needs to be updated since it only refers to profile module fields.

T. The label on the "Janrain Engage Identities" tab should be configurable on the module settings page (admin/config/people/rpx) since this is user-facing text. A lot of sites won't want a tab label that long because it could break a lot of site layouts that don't have that much width available. Also, I recommend that, by default, the tab be labeled "Linked accounts". I don't think most end users understand what an "identity" is. Likewise, Drupal best practices would not normally place a third-party service name like "Janrain Engage" in a user-facing UI tab. Most Drupal site admins will want to change this so we're better off making it more usable from the start.

U. The link to the "Janrain Engage Identities" tab is currently user/%user/rpx. In follow-up to R. above, I'd change this to: user/%user/linked-accounts

V. At user/%user/rpx, I'd change the "Add identity" link to "Add linked account". For the table column header, change "Janrain Engage Identifier" to "Account ID". The confirmation messages when accounts are added or removed will also need to be updated to use the "linked accounts" terminology (instead of "Janrain Engage ID").

W. Also at user/%user/rpx, I'd love to see a third column in the table (with the column header labeled "Account type"). This new column would be the first column on the left. In the column, we could display a logo for the particular service provider (like a Facebook logo for a linked facebook account or a Twitter logo for a linked Twitter account). This would make things much more user-friendly to the end user who isn't used to seeing the long identifier link code.

X. The help/explanatory text at user/%user/rpx should be configurable on the module settings page. A lot of sites may want to edit or otherwise shorten this text because it's pretty long.

Y. When a user adds a new identity and receives the confirmation message, he is incorrectly being taken out of the overlay (which correctly launched when the tab was clicked). The user should also be staying within the overlay when deleting an identity... but currently this is working incorrectly, too.

Anyway, that's it for now... let me know your thoughts. Once some of this UI stuff is fixed/updated, I also plan to do more functional testing, too (including more testing of field mapping).

Thanks again for all of your hard work on this... we're making good progress! :-) Since Drupal 7.0 is officially being released on Jan. 5, we should try to work toward a beta release of the module by then or shortly thereafter.

--Ben

Hi Ben,

First of all thanks for all your great feedback. I am already
implementing all of your suggestions right now, except:

U. The link to the "Janrain Engage Identities" tab is currently
user/%user/rpx. In follow-up to R. above, I'd change this to:
user/%user/linked-accounts

I believe we should not be doing this because:

1. It's modeled after the Core OpenID module (user/%user/openid) and I
think we should do "as the core does".

2. There may be modules out there that would like to use the
"linked-accounts" endpoint; we don't want to clash with them. So I
think it is a good idea to name the endpoint after the module.

BTW, by the same reasoning we might want to pass on (T) (because the
core OpenID uses "OpenID identities" in its user-facing UI tab). But I
have no problem changing it as you suggested. Please let me know what
you think.

Thanks,
George

Hi George,

U. I agree with your points about the path. So we can keep it as is (user/%user/rpx).

T. As for the tab, I'd still recommend that we use "Linked accounts" as the default.

I think OpenID is a different use case: In my experience, people using OpenID generally already know what it is and are familiar with its "identity" terminology. But for the run-of-the-mill end user who just knows they have a Facebook account, we should try to avoid technical sounding jargon that seems obvious to us developers (but is not so obvious to a random user). So the advantage of words like "linked" and "accounts" is that they are pretty universal website terms that an end-user is likely to have encountered before.

Cheers,
Ben

Ben, I think you are right about the tab name. I made it customizable (already in CVS).

Thanks,
George

Now that we have a working 7.x-1.x-dev branch, we should probably create individual issue threads for each bug. So I'm marking this issue thread as "fixed."

I've also gone through and tested the fixes to A. through Y. above... most are fixed but a few still remain. So I've created individual issue threads for each when follow up is necessary. Check the module issue queue for the latest.

Cheers,
Ben