[D6] dlsconnect

Hi,

we are already using this module to connect our community with a DLS server (www.q-online.de). The module works fine, the short time passwords (nearly one time, because it is only valid through one soap request and randomly changed afterwards) help us to protect our Drupal installation, because some DLS Server are not configured for using a SSL connection.

So, I hope, that permission will be granted to create a full dlsconnect project.

Kind regards,
Dirk

* Git release branch missing, see http://drupal.org/node/1015226
* lines in README.txt should not exceed 80 characters
* Remove all old CVS $Id tags, not needed anymore
* "// special client soap user: have to be created manually on DLS server" comments should be on their own line, start capitalized and end with ".", see http://drupal.org/node/1354#inline
* "$parameters = array( 'ApiLogin' => $apilogin," the first array entry should be on a separate line
* dlsconnect_init(): Are you sure you need hook_init()? It is called on literally every page request, even if your module is not needed on that page.

@C_Logemann has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

Please don't assign the issue to your self, only the current reviewer should do this.

Sorry for the delay,
as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

Hi,

There are still some errors in automated review http://ventral.org/pareview/httpgitdrupalorgsandboxclogemann1245466git :

FILE: ...st_candidate/dlsconnect_feature/dlsconnect_feature.features.content.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
195 | ERROR | Array indentation error, expected 4 spaces but found 0
327 | ERROR | Array indentation error, expected 4 spaces but found 0
--------------------------------------------------------------------------------

Manual review:

1) The function dlsconnect_block

case 'list':
      $blocks[0]['info'] = t('DLS client information');
      $blocks[1]['info'] = t('DLS group information');
      $blocks[2]['info'] = t('DLS user links (SSO)');
      return $blocks;

It doesn't look as a good idea to identify these blocks by number. Please, to prevent confilcts define some const like
define('DSLCONNECT_CLIENT_INFO', 'dslconnect_client_info');
and make it like

      $blocks[DSLCONNECT_CLIENT_INFO]['info'] = t('DLS client information');

2) the function dlsconnect_init

 // Activate default function.
  $settings = dlsconnect_dlsconnect_settings();
  foreach (module_implements('dlsconnect_settings') as $module) {

    // Override if hook_ldapimp_db is set other modules,
    // mostly one custom.module.
    if ($module != 'dlsconnect') {
      $settings = module_invoke($module, 'dlsconnect_settings');
    }
  }

It probably makes sence to replace it with drupal_alter(...), don't you think?

Apart from mentioned, it looks ok for me.

I do not think that you have to take care of the errors that the Feature module creates.

I have to agree with @itsekhmistro on dlsconnect_block - here you could get conflicts with other modules.

According to http://api.drupal.org/api/drupal/developer%21hooks%21core.php/function/h...

In hook_block(), each block your module provides is given a unique identifier referred to as "delta" (the array key in the return value for the 'list' operation). Delta values only need to be unique within your module, and they are used in the following ways:

Passed into the other hook_block() operations as an argument to identify the block being configured or viewed.
Used to construct the default HTML ID of "block-MODULE-DELTA" applied to each block when it is rendered (which can then be used for CSS styling or JavaScript programming).
Used to define a theming template suggestion of block__MODULE__DELTA, for advanced theming possibilities.
The values of delta can be strings or numbers, but because of the uses above it is preferable to use descriptive strings whenever possible, and only use a numeric identifier if you have to (for instance if your module allows users to create several similar blocks that you identify within your module code with numeric IDs).

The values of delta can be strings or numbers, but because of the uses above it is preferable to use descriptive strings whenever possible, and only use a numeric identifier if you have to (for instance if your module allows users to create several similar blocks that you identify within your module code with numeric IDs).

It's only a suggestion.

According to http://api.drupal.org/api/drupal/developer%21hooks%21core.php/function/h...

In hook_block(), each block your module provides is given a unique identifier referred to as "delta" (the array key in the return value for the 'list' operation). Delta values only need to be unique within your module, and they are used in the following ways:

Passed into the other hook_block() operations as an argument to identify the block being configured or viewed.
Used to construct the default HTML ID of "block-MODULE-DELTA" applied to each block when it is rendered (which can then be used for CSS styling or JavaScript programming).
Used to define a theming template suggestion of block__MODULE__DELTA, for advanced theming possibilities.
The values of delta can be strings or numbers, but because of the uses above it is preferable to use descriptive strings whenever possible, and only use a numeric identifier if you have to (for instance if your module allows users to create several similar blocks that you identify within your module code with numeric IDs).

The values of delta can be strings or numbers, but because of the uses above it is preferable to use descriptive strings whenever possible, and only use a numeric identifier if you have to (for instance if your module allows users to create several similar blocks that you identify within your module code with numeric IDs).

It's only a suggestion.

I stand corrected.

manual review:

1. if you require nusoap then you should implement hook_requirements() to check if it is available. Also, why don't you rely on the more common PHP SOAP?
2. "variable_del('ldapprov_debug');": all your variables need to be prefixed with the name of your module to avoid name collissions.
3. "$settings['soaplib'] = 'sites/all/libraries/nusoap/lib/nusoap.php';": do not hardcode that library path. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
4. _dlsconnect_pass(): Yep, you should really use drupal_alter(), so that other modules can change what you provide on default.
5. If your module invokes its own hooks you should provide an .api.php file for documentation and usage of those. See http://drupal.org/node/161085#api_php
6. dlsconnect_dlsconnect_actions(): empty return statement at the end of a function is not needed. Also elsewhere.
7. "t('Soapuser exists in DB and LDAP with DLS name: ') . $dlsusercreate": do not concatenate translatable strings, use placeholders instead. Also elsewhere.
8. "drupal_set_message(t('There is a Problem with creating DLS name for Soap User: '), 'warning');": does not contain a user after the ":"?
9. _dlsconnect_page_client_deletegroups_confirm(): All the markup should probably go to a theme function, so that it can be overridden.
10. "$output .= '<li class="dlsconnect-listitem-clientname">' . t('DLS client') . ': ' . $clientdata['title'] . '</li>';": this is vulnerable to XSS exploits if the title contains script tags or what not. You must sanitize user provided input before printing it, see http://drupal.org/node/28984
11. _dlsconnect_page_client_deletegroups_confirm(): use theme('item_list', ...) to build list markup.
12. Instead of using _dlsconnect_page_error_access() you could just use drupal_access_denied().
13. dlsconnect_admin_settings_submit(): the second declaration of $values is never used.
14. dlsconnect_admin_settings(): you could use system_settings_form() here, then you don't need the submit callback.

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

Thanks for your reviews!

manual review:

1. "drupal_set_message($msg, 'warninh');" Should be "warning", right?
2. Usage of check_plain(): Does not always make sense to me. You should only use it if you are printing something to the user and you should do check_plain() as late as possible to not mess up the data while it is passed around. Example: dlsconnect_nodeapi() or _dlsconnect_groupuser_add() where you don't print anything.
3. _dlsconnect_show_clientinfo(): do not create link markup yourself, use l() instead.

But otherwise this looks RTBC to me. Assigning to tim.plunkett as he might have time to finally approve this. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Looks good for me,

Thanks for your contribution, C_Logemann! Welcome to the community of project contributors on drupal.org!! :)

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.