[D7] Windows Live login

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:

• Drupal Code Sniffer has found some code style issues (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.

Looking at your project's page, the description seems a bit short. You should have a more detailed description of what the module does.

Looking into the code, I see you are using this type of verification

if (@!empty() ...

It's probably better to use the isset() function instead. This way you won't hide any errors that might occur.

Function windows_live_login_form_user_profile_form_alter()

if (@!empty($form['field_user_profile_link']['und'][0]['value']['#default_value'])) {
  $form['field_user_profile_link']['#disabled'] = TRUE;
}

You are using $form['field_user_profile_link']['und'][0]['value']['#default_value']. The problem is that the "und" parameter specifies the language. If the language is undetermined, then the parameter is "und". You should use the field_get_items() function instead.

$profile_link = field_get_items('user', $form_state['user'], 'field_user_profile_link');

Attached file shows result of automated review using drupalcs.
Manual Review:

• windows_live_login.install: I would suggest to implement hook_requirements() for creating directories while installing module if it doesn't exists already & set permissions respectively.
• windows_live_login_menu(): No need of wrapping menu title & description from t(), check -> http://drupal.org/node/140311
• README.txt: a little typo -> line no 19,40 "allready" should be "already"

Ohh yes,

You can check code in .install file in the module, I have implemented it http://drupal.org/sandbox/nmudgal/1311396
And by any luck you if you get any chance/time you can review it too :-) http://drupal.org/node/1457952

Thanks.

1. You should use 2 spaces as TABs not 4 (also counts for JS files)

2. allready => already as stated already by nmudgal , you also misspelled simular (=> similar) ...

3.

The Windows_live_login module is a standard Drupal 7 module. The installation is
simular to other Drupal 7 modules.

4. "standard" could be confused with "core", I recommand rethingking the entire phrase, besides that, people should know how to basically install modules before yours.

5. Your @file doc statements are not very informatif.

* Windows Live login .module file

People should know when they open a .module file that it's actually a .module file.
The same for other files.
PS: I must state that I have my problems with those comments as well, but there are peoplbe becoming very angry about them ;)

6. I don't want to be paranoia, but isn't there a trademark on "Windows Live", I'me not sure one is allowed to use it that way ... but I may be wrong

7. I think windows_live_login_login_account_info() should be put into a theme function.

8. You should also check your project page, check http://drupal.org/node/997024

Manual Review

After having installed the module and setup the oauth keys, I get the following message:
Fatal error: Call to undefined function curl_init() in /media/workspace/testsystems/drupal/1/modules/windows_live_login/windows_live_login.module on line 378

If your module requires cURL you have to check for that in hook_requirements().
See for example http://api.drupal.org/api/drupal/modules!simpletest!simpletest.install/f...

--------------------------------------------

// Open connection.
  $ch = curl_init();

  curl_setopt($ch, CURLOPT_URL, $url);
  curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
  curl_setopt($ch, CURLOPT_TIMEOUT, '3');
  $windows_live_user = curl_exec($ch);

  // Close connection.
  curl_close($ch);
  $windows_live_user = json_decode($windows_live_user);

Do not use json_decode(), use drupal_json_decode() instead.
The error handling should be improved by checking the returned results.

Here an example:

if (($http_data = curl_exec($curl)) !== FALSE) {
      $http_code = curl_getinfo($curl, CURLINFO_HTTP_CODE);
      curl_close($curl);
      if ($http_code == 200))
     {
        $windows_live_user = drupal_json_decode($http_data);
     }
}

--------------------------------------------
function windows_live_login_password_generator($length = NULL) {}

Drupal already has a build-in function to generate passwords:
http://api.drupal.org/api/drupal/modules!user!user.module/function/user_...

--------------------------------------------

Regards,

Hi,

First of all I would recommend you to review other modules to get review bonus so that reviewers can get back to this application sooner.

Now some manual review.
In the windows_live_login.module file, it is good practice to include all the administrative settings in .admin.inc file, so change implementation of hook_menu as follows.

function windows_live_login_menu() {
  $items['admin/config/services/windows-live-login'] = array(
    'title' => 'Windows live login',
    'description' => 'Adjust windows live login options.',
    'position' => 'right',
    'weight' => -5,
    'page callback' => 'system_admin_menu_block_page',
    'access arguments' => array('administer site configuration'),
    'file' => 'system.admin.inc',
    'file path' => drupal_get_path('module', 'system'),
  );
  $items['admin/config/services/windows-live-login/settings'] = array(
    'title' => 'Windows live login settings',
    'description' => 'Set the windows live login settings.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('windows_live_login_menu_settings_form'),
    'access arguments' => array('windows live login settings'),
    'type' => MENU_NORMAL_ITEM,
    'file' => 'windows_live_login.admin.inc',
  );
  return $items;
}

So the function windows_live_login_menu_settings_form() would be placed in windows_live_login.admin.inc file. I have used some example text for title and description, please choose some decriptive text instead of example text in above code.

In the windows_live_login.info, include this line

configure = admin/config/services/windows-live-login

As adding these line would give a configure link on the module page, once the module is enabled. I will spend some time reviewing the whole code, would be great in case you could review other modules too. Would revert back once done.

These are no major issues and therefore no reason the set needs work

Hi,

It appears that there have been multiple project applications opened under your username:
http://drupal.org/node/1301558

As successful completion of the project application process results in the applicant being granted the ´Create Full Projects´ permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as ´closed(duplicate)´, with only the oldest application left open.

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the ´open´ application as a duplicate, and re-open one of the project applications which had been closed.

Thanks,
Ashish.

drupal version