mydigipass

Your code seems very good and follow Drupal standards :-)

A few things I found :

• callback.inc.php, line 18: variable $client_id is unused
• mydigipass.module, line 103: variable $user is unused
• mydigipass.module, line 634 & 659: $output is defined and immediately overwritten.

Thank you for you reviews! When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

manual review:

1. mydigipass_admin_settings_user_profile_fields_form(): $all_user_data_fields is third party provided input and needs to be sanitized before it can be use in #options on checkboxes form elements in Drupal 6. See http://drupal.org/node/28984 . I would say this is a security issue, correct me if I'm wrong.
2. mydigipass_admin_settings_button_style_form(): all the user facing options should run through t() for translation.
3. mydigipass_init(): please add a comment why you cannot use drupal_add_js().
4. mydigipass_form_alter(): use l() to generate link markup, it is capable of attributes. Also elsewhere.

Otherwise this looks nearly ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I didn't find any real problems with your modules, these are mostly little hints and some realy small tweaks.

Manual review:
For your include files, there is no need to add .php behind the .inc. You could name then callback.inc and mydigipass.admin.inc (no need to change it everywhere, just saying).

When adding the validation handlers to the forms:

$form['#validate'] = array_merge(array('mydigipass_enforce_authentication_mode_during_login'), $form['#validate']);

You could also use array_unshift().

array_unshift($form['#validate'], 'mydigipass_enforce_authentication_mode_during_login');

In you comments on line 181-.. you mention that you need the hidden value, because the validation handler expects it. You know you can also use the type "value" which is not send to the browser at all?
http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht...

On line 219/290/353/459/... you do a SELECT COUNT(*) to check if a result exists. SELECT 1 would have the same result, but a tiny bit faster.

In a couple of SQL queries you select from multiple tables and join them with a where statement. I am not sure which version is faster, but since Drupal mostly uses JOINs I'd advice using those since it would make them easier readable.
$sql = "SELECT UD.attribute_value FROM {mydigipass_user_link} UL, {mydigipass_user_data} UD WHERE UL.drupal_uid = %d AND UL.mdp_uuid = UD.mdp_uuid AND UD.attribute_key = 'email'";
vs
$sql = "SELECT UD.attribute_value FROM {mydigipass_user_link} UL LEFT JOIN {mydigipass_user_data} UD ON UL.mdp_uuid = UD.mdp_uuid WHERE UL.drupal_uid = %d AND UD.attribute_key = 'email'";

on line 239 you have a subselect, which I think is not needed. I am not 100% sure, but I think this would do the same (a tiny bit) faster:

SELECT * 
FROM {mydigipass_user_link} UL
LEFT JOIN {mydigipass_user_data} UD ON UL.mdp_uuid = UD.mdp_uuid
LEFT JOIN {mydigipass_profile_fields} PF ON PF.name = UD.attribute_key
WHERE UL.drupal_uid = %d AND PF.name IS NOT NULL
ORDER BY UD.attribute_key";

On line 302/423/... I don't think drupal_goto(); works that way (with ). Just give it an empty string to redirect to the frontpage: drupal_goto('');

On line 577 you might want to add a watchdog call so an admin at least gets a message that something is wrong.

In your hook_schema in the .install file:
{mydigipass_user_link} probably also needs a unique index on drupal_uid (this is unique right?) because I noticed some queries on it.

In mydigipass.admin.inc.php
On line 189 you delete all rows. Truncate is faster and also works in all D6 (and D7) supported database engines.

On line 198 you have this line:

$success &= db_query($sql, $value);

1. Why is this by reference?
2. By overwriting this value every time you only save the success of the last query.
I'd make it something like:

$success = db_query($sql, $value) && $success;

Callback.inc.php
On line 115 COUNT() is not all-capitals.

Overall nice module! Setting it to RTBC since there are no real errors. If the module gets approved before you have the chance to look at these things, just copy them to your issue queue and put it on the lowest priority.

manual review:

• "$form['mail']['#default_value'] = check_plain($mdp_email);": do not use check_plain() on #default_value, the form API will sanitize that for you. See http://drupal.org/node/28984

But that is not an application blocker, so ...

Thanks for your contribution, lva!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.