Simple LDAP

Hi,

here are my responses to your manual review done:-
1) You are using variable_get in your module without mentioning its default value. Found it in simple_ldap_role.module line 78. Default value should also be set while using variable_get
2) simple_ldap_test.module file is empty

Also make sure to fix your ventrail issues here : http://ventral.org/pareview/httpgitdrupalorgsandboxbchavet1845170git

Thanks
Parwan

README.txt

The README on this project is actually really useful, the only thing I'd suggest is adding a module description to it.

project page

Check.

Master Branch

Nope, not using it.

Major coding standards / best practice issues

A run through of the module and submodules in this project via Coder returned the following: 266 passes, 124 fails, 99 exceptions, and 92 debug messages.
A run through of the module and submodules in this project via PAReview.sh returned: Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards.
See attachments for more details.

License

Not present, Check!

access administration vs. administer site configuration

Permissions make sense.

files[] without classes or interfaces

Files present in simple_ldap.info declare classes, no issues.

PAReview: 3rd party code

No 3rd party stuff here.

Multiple Applications

Nope, you're all set.

Already Approved

Nope, but I bet you will be soon.

Idle Application

Nope, you're all set.

Code too short

Nope, you're all set.

The Coder results are gigantic, so you'd probably be better off just running it through yourself. Most of it seems related to the tests failing. I'm attaching a link to the screenshot because it's too big for d.o: https://docs.google.com/open?id=0B2ENsfpXRywVU3JSWlFjWDNGQ0U

The PAReview.sh / ventral results have been addressed in your last comment, but I'm attaching screenshots here to keep things all in one place as much as possible. Coder and similar dev tools aren't always perfect.

I scanned the files pretty quickly and didn't notice any outstanding coding style errors. This could probably use a security check from someone more qualified than I.

I can comment on the first section of the report, though. These functions are overridden from password.inc as described here: http://api.drupal.org/api/drupal/includes%21password.inc/7, so the function names cannot be changed. simple_ldap_user needs to handle passwords in plain-text in order to properly encrypt and send to the LDAP server. Password.inc is the only location that this can be intercepted.

Yes, what you have done there is perfect, blc. Also, these are the same findings that @carwin posted in his first attachment, so they can be disregarded there as well.

1) You are using variable_get in your module without mentioning its default value. Found it in simple_ldap_role.module line 78. Default value should also be set while using variable_get

As @blc mentions, the default value is NULL in Drupal 7, and it isn't necessary to explicitly specify. That was only the case in Drupal <= 6.

A couple of other things to note.

1. It looks like you are exposing your own hook, called hook_sync_to_ldap. It's a common practice to add a mymodule.api.php file that documents how this hook might be used. For an example, you might look at Views module.
2. The README file documents the class structure, which also might be good to go in the api file. README's typically are instructions on how to get the module set up, things to know, etc. You might document how to get a testing environment set up in there, how to specify the user map in settings.php, etc.
3. I mentioned this before, but I don't see a lot of error detection going on. I would try to find where the best places are to wrap things in try {} catch () {}, and then log caught exceptions to the watchdog. One way to do that is to use the __call() magic method, and make all of your real class methods private and prefixed with an underscore. Then, in __call(), you can do:
   

     public function __call($name, $arguments) {
       $function_name = "_$name";
       if (method_exists($this, $function_name)) {
         try {
           return call_user_func_array(array($this, $function_name), $arguments);
         }
         catch (SimpleLdapException $e) {
           watchdog(__FILE__, (string) $e, array(), WATCHDOG_ERROR);
         }
       }
     }
   

4. This is by no means necessary to get this module approved, but I would love to have a debug mode for this module, that prints things like the query made to ldap, the raw response, etc. to drupal_set_message, or watchdog or something.
5. Again, by no means necessary, but you might consider adding hook_requirements implementation if certain required variables are not set properly, such as the password_inc variable, the user map variable, and basic things like checking to make sure the site can connect to the LDAP server successfully.

Overall, this module is a fantastic alternative to the existing LDAP module. Whereas the current LDAP module errs on the side of flexibility, this module, true to its name, errs on the side of simplicity. In my opinion, there need be no discussions in this issue of trying to combine these modules, or work together as maintainers, as they have fundamentally different goals, and trying to make the one marry to the other would end up losing what is good about either project.

The code in this module is immaculate, and speaks to the care and attention to detail that @blc has given this module. In addition to having a solid object oriented API, there are simpletests for every critical aspect of the module, such as user authentication, role creation/deletion, password security, and the list goes on.

We are using this module in a production environment that has 7000+ users with success. In addition, @blc has been very responsive and quick to fix issues as they've arisen.

All in all, this project gets one HUGE +1 from me. I don't think any of the above issues need to be addressed before this module gets approved. There are modules currently in contrib that don't hold a candle to the quality of work already present in this module (including my own), and everything mentioned above is just icing on the cake. Great work, @blc!

Agree with @q0rban about the REAME.txt contents. These should be moved to simple_ldap.api.php. There are some tips about what should README.txt files have at http://drupal.org/node/447604.

The simple_ldap_role README.txt is empty, while the contents of simple_ldap_user's README.txt should be moved to simple_ldap_user.api.php.

Aren't here any libraries or extensions that this module needs? If so, implementing hook_requirements() would ensure they are met.

There is a small typo at SimpleLdapSchema

  /**
   * Return a lis tof attributes defined for the objectclass.
   */

simple_ldap_role.test misses some docblocks at the top of some of the test classes.

Looking at simple_ldap_user.password.inc, I see several functions that do not follow the modulename_function pattern, which could cause collisions. Is there a reason for that? Besides, aren't Drupal's password and hashing functions useful for this scenario?

These are very simple fixes and questions. The module looks very robust overall. Nice work!

Based on the above reviews and fixes, I am marking this as RTBC.

Bump for RTBC, I can't find anything wrong with this.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Thanks for your contribution, blc!

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.