jchin1968 [jchin1968]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

What are the differences between the proposed module, and http://drupal.org/project/yubikey?

Thank you for your reply.

I have a small debt to repay for getting a CVS account quickly (http://drupal.org/node/904138), so I'm trying to help real coders in reviewing applications more efficiently.
The following comments are *not* ground for approving or declining an application.

The Coder module gives some warnings concerning formatting and coding standards, mostly concerning tabs and spaces but also a few true/false that should be TRUE/FALSE. It also says "@file block missing" for otp_settings.inc and otp.install.

I do not dare code review something that has to do with passwords and login, but I'd recommend that you install Coder and get rid of the code standard warnings. Cheers!

I am changing the status as per Itangalo's comment.

• The points reported in this review are not in order of importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. version = "6.x-1.0"
   core = "6.x"
   project = "otp"
   

   Those lines need to be removed.

2.   variable_set('otp_length', 8);
     variable_set('otp_expiry', 10);
     variable_set('otp_port', 465);
   
   

   Drupal variables are not initialized in hook_install().

3. Menu descriptions and titles, as well as schema descriptions, should not be passed to t().

4.   // Remove all otp variables
     $results =  db_query("SELECT name FROM {variable} WHERE name LIKE 'otp_%'");
     while ($row = db_fetch_object($results)) {
       variable_del($row->name);
     }
   
   

   Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules (there could be a otp_menu.module module, for example).

5. /**
    * Implementation of hook_perm().
    * Create permission for administering OTP settings.
    */  
   function otp_perm() {
     $perms = array(
       'otp settings',
     );
     return $perms;   
   }
   
   

   The permissions follow the schema <verb> <object> where the verb is written in lowercase.

   Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

   As reported in Documenting hook implementations:

   If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

   /**
    * Implements hook_help().
    */
   function blog_help($section) {
     // ...
   }
   

   This generates a link to the hook reference, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook. Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

   In the case of hooks that have variables in the names, such as hook_form_FORM_ID_alter(), a slightly expanded syntax should be used:

   /**
    * Implements hook_form_FORM_ID_alter() for node_type_form().
    */
   function mymodule_form_node_type_form_alter(&$form, &$form_state) {
     // ...
   }
   

   This generates a link to the hook reference, as well as to the particular form that is being altered. Again, optionally you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

6.   if (eregi("^[_a-z0-9-]+(\.[_a-z0-9-]+)*@[a-z0-9-]+(\.[a-z0-9-]+)*(\.[a-z]{2,3})$", $email)) {
       $valid_email = TRUE;
     }
   
   

   That function is considered deprecated, from PHP 5.3. Is there any reason the code is not using the PCRE functions?

7. The JavaScript code should use Drupal behaviors.

As far as I can see it's looking good. Very good README.txt too!

I like this module very much. What I am looking for now is to add a field to the create account form that asks for a phone number. Then i want the $user->otpSendAddress variable to use that number to send the password to the user by default. Can anyone give pointers on how to do this?

Glad to see someone working on this module, looking good so far.

Just a few small issues:
- Drupal has its own valid_email_address() function
- You use PHPmailer library without having mention that anywhere. Include a minimal version (if it is GPLed) or instruct user to put it in a libraries folder
- Don't use a global variable just so save a path
- Use hook_init() to include a .css file (or I think you only need it on hook_form_alter())
- Avoid $_SERVER['DOCUMENT_ROOT']

About the module logic, it looks good after a quick verification.

Libraries available from third-party sites should not be committed in Drupal.org, even if they are licensed under GPL License.

Please read all the following and the links provided as this is very important information about your CVS Application.

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:Migrating from CVS Applications to (Git) Full Project Applications.

• If your application has been "needs work" (or "postponed (maintainer needs more info)"), your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.
• if the status of this application is a different one, it will be changed to "postponed"; you will be able to reopen it by following the instructions in the above link.

@jchin1968.. has this module been finalized.. or can you give a new link to GIT

As per my previous comment, I am setting this issue as Won't fix.
Since new users can now create full projects, applications have a different purpose and they are handled on a different issue queue. See Apply for permission to opt into security advisory coverage for more information.