hosting http basic auth ldap

Hi and welcome to the queue,

only some brief hints, rather minor issues:

• You are working on the master branch; you should work on a major branch instead. See http://drupal.org/node/1127732.
• Although it can be found in the info file, you should add the major core branch to your summary.
• Doxygen comments schould be added to all functions (except hooks). See http://drupal.org/node/1354
• You should also add @file comments.

Besides "formal" review, your Readme says "module may not work". I am not really familiar with aegir stuff, so probably there is no problem. Just wanted to mention it.

Hi, the D6 info should be here on the review appliance page, in the summary.

As soon as you are ready for the second round, don't forget to switch the status to "needs review" again.

Personal note: As I have been struggling with the coding standards myself not many days ago, I really recommend taking one or two hours to read the documents I have linked. They are not completely consistent after all (some things are constantly evolving atm) but give a good overview about what is considered as good documentation here.

If you have further questions feel free to get in direct contact, und dann auch gerne auf deutsch ;)

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/hosting_http_basic_auth_ldap.module:
   +5: [minor] Comment should be read "Implements hook_foo()."
   +39: [minor] Comment should be read "Implements hook_foo()."
   +48: [minor] Comment should be read "Implements hook_foo()."
   +61: [minor] Comment should be read "Implements hook_foo()."
   +68: [minor] Comment should be read "Implements hook_foo()."
   +75: [minor] Comment should be read "Implements hook_foo()."
   +106: [minor] indent secondary line of comment one space 
   +106: [minor] Comment should be read "Implements hook_foo()."
   +107: [minor] indent secondary line of comment one space 
  
  Status Messages:
   Coder found 1 projects, 1 files, 9 minor warnings, 0 warnings were flagged to be ignored
  

• README.txt is missing, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• @file doc block is missing in the install file, see http://drupal.org/node/1354#files .
• ./hosting.feature.http_basic_auth_ldap.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

      // initial status ( HOSTING_FEATURE_DISABLED, HOSTING_FEATURE_ENABLED, HOSTING_FEATURE_REQUIRED )
  

• ./provision/http_basic_auth_ldap.drush.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function http_basic_auth_ldap_provision_services() {
  function http_basic_auth_ldap_provision_apache_vhost_config($uri, $data) {
  function http_basic_auth_ldap_provision_nginx_vhost_config($uri, $data) {
  

• ./hosting.feature.http_basic_auth_ldap.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  15- *  associative array indexed by feature key.
  

• Classes and Interfaces should use UpperCamel naming. See http://drupal.org/node/608152
  

  ./provision/http_basic_auth_ldap.drush.inc:22:class provisionService_http_basic_auth_ldap extends provisionService {
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./hosting_http_basic_auth_ldap.install ./hosting_http_basic_auth_ldap.drush.inc ./hosting.feature.http_basic_auth_ldap.inc ./usr/share/drush/commands/provision/http/apache_ssl/vhost_ssl_disabled.tpl.php ./usr/share/drush/commands/provision/http/apache_ssl/server_ssl.tpl.php ./provision/http_basic_auth_ldap.drush.inc ./hosting_http_basic_auth_ldap.module ./hosting_http_basic_auth_ldap.info
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

You are working on a false named branch in git! See the documentation about release naming conventions in git.

Review of the 6.x-1.1 branch:

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• ./provision/http_basic_auth_ldap.drush.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function http_basic_auth_ldap_provision_services() {
  function http_basic_auth_ldap_provision_apache_vhost_config($uri, $data) {
  function http_basic_auth_ldap_provision_nginx_vhost_config($uri, $data) {
  

• ./hosting.feature.http_basic_auth_ldap.inc: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  15- *  associative array indexed by feature key.
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  usr/share/drush/commands/provision/http/http.ssl.inc:2:// $Id$
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

.. crap .. (this comment was, not the project)

I'm sorry I misunderstood what your point was - I should read more carefully indeed :(

The .x means that drupal.org will treat it as the latest dev version.
The branch 6.x-1.x will be packaged as 6.x-1.x-dev. The stable(or beta or whatever) releases will then be created with tags.

So, while your personal development branch, 6.x.2.x-dev, has a bit misleading name, your branching is technically correct. The 2.x just wont get recognized by the packaging script. It is, however, unclear to me why you wont name the 2.x correctly also? The dev branch is not considered as a recommended, working release, it's completely ok if it does not work, and it would make it easier for the other developers to chip in. You even get to choose if you want it packaged or not.

I spotted some minor coding standard issues, but apart from those, this look ready to me. I lack the expertise in this area, but personally i believe you don't.
It would be good, however, to get some insight also functionality-wise. Maybe you could draw attention to this case from IRC?

if ($node->type == 'site') {
    switch ($op) {
    case 'insert':
        hosting_http_basic_auth_ldap_insert($node);
        break;

Mis-indentation.

 $lines[] = "  <Directory \"$root\">";
    $lines[] = "    AuthType Basic";
    $lines[] = "    AuthName \"$message\"";
    $lines[] = "    AuthBasicProvider ldap";
    $lines[] = "    AuthLDAPURL \"$server\"";
    $lines[] = "    Require $require";
    $lines[] = "    AuthzLDAPAuthoritative on";
    $lines[] = "  </Directory>";

Generally arrays(when longer than 80 charcters) in drupal should be created this way(example from the coding standards doc):

  $form['title'] = array(
    '#type' => 'textfield',
    '#title' => t('Title'),
    '#size' => 60,
    '#maxlength' => 128,
    '#description' => t('The title of your node.'),
  );

	function hosting_http_basic_auth_ldap_hosting_feature() {
	  $features['http_basic_auth_ldap'] = array(
	    // title to display in form
	    'title' => t('HTTP Basic Ldap Authentication'),
	    // description
	    'description' => t('Allows admins to specify HTTP basic ldap authentication for sites.'),
	    'status' => HOSTING_FEATURE_DISABLED,
	    // module to enable/disable alongside feature
	    'module' => 'hosting_http_basic_auth_ldap',
	    // which group to display in ( null , experimental )
	    'group' => 'experimental'
	    );
	  return $features;
	}

Also the last line of the array must end in comma(,).

Actually, since the issues are minor, let's keep this in "needs review" state hoping that someone will jump in with better in-depth review.
Please fix the coding standard things anyway.

If nobody comes, i say let's change it to RTBC(since the code looks ok and you yourself are already using it in production environment) and see what the "upper level" thinks.

Review of the 6.x-1.x branch:

• Remove all old CVS $Id tags, they are not needed anymore.
  

  usr/share/drush/commands/provision/http/http.ssl.inc:2:// $Id$
  

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

• hosting_http_basic_auth_ldap_nodeapi(): a break statement after a return statement is unreachable code, so remove it.
• wrong git tag name, "6.x.1.1" should be "6.x-1.1". Regarding the branch 6.x.2.x-dev see comment from bfr.
• "include_once('/usr/share/drush/commands/provision/provision.service.inc');": hard-coding this path is ugly. The installation instructions should mention that the user needs to change that path if they have drush in a different location.

Otherwise I think this is nearly ready. I know, getting feedback on your application took long, you can speed up the process with #1410826: [META] Review bonus if you don't want to wait another 4 weeks for a review.

please see http://ventral.org/pareview/httpgitdrupalorgsandboxblueprint1331024git for the errors still being generated

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

manual review:

1. The wrong tag "6.x.1.1" still exists, please remove it.
2. Most of the automated coding standard errors still exist for your module file and others, but I consider them minor.

Otherwise this looks RTBC.

Note that comments should begin capital and end with . ! or ?
Also you indentation is quite unstructured (eg. in the switch() in hook_nodeapi)
The $ID$ stuff is also not needed any longer, remove it.

Thanks for your contribution!

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.