[D7] Xing Connect

Please correct git instructions git clone --branch 7.x-1.x http://git.drupal.org/sandbox/ajayNimbolkar/2441787.git so that other can review your module,

Also please add review bonus.

Why your project directory is 2441787 and not xing_connect?

Automated Review
No pareview errors found

Manual Review

Individual user account
Yes follow the guideline

No duplication
Yes follow the guideline

Master Branch
Yes follow the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template.
- There are no some sections.
- Headings don't correspond to the template.

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Yes: Meets the security requirements

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxajayNimbolkar2441787git

I'm a robot and this is an automated message from Project Applications Scraper.

Hi ajayNimbolkar,

Please use the same style across README.txt file.
For example, INSTALLATION has semicolon after it and is not underlined like others.

Please also put correct link for git clone (You can find the correct git clone command for your sandbox by clicking on the Version control tab, removing the checkbox in front of "Maintainer", and clicking Show. You can then copy-paste the git clone command from the codeblock below "Setting up repository for the first time".).

Thanks,

Review of the 7.x-1.x branch (commit 364ea79):

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

  FILE: /home/klausi/pareview_temp/README.txt
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
  ----------------------------------------------------------------------
    9 | WARNING | Line exceeds 80 characters; contains 109 characters
   13 | WARNING | Line exceeds 80 characters; contains 85 characters
   34 | WARNING | Line exceeds 80 characters; contains 174 characters
  ----------------------------------------------------------------------
  
  FILE: /home/klausi/pareview_temp/xing_connect.install
  -------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  -------------------------------------------------------------------------
   5 | ERROR | [x] Doc comment short description must be on the first line
  -------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  -------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

manual review:

1. What are the differences to existing projects such as https://www.drupal.org/project/hybridauth ? I can already use XING with that and have many more login providers? Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please add information about existing projects to the project page.
2. xing_connect_menu(): "'access arguments' => array('administer')," is invalid, that permission name does not exist?
3. xing_connect_menu(): why does the xing/auth path require the 'administer xing' permission? So nobody will be able to login? And people should surely not give the 'administer xing' permission to anonymous users?
4. xing_connect_unique_user_name(): doc block_: "fb username"? You mean XING username?
5. "drupal_set_message(check_plain(t('Welcome !!! You have been logged in with the username !username', array('!username' => $account->name))));": instead of using check_plain() on the translated message you should use "@" or "%" placeholders with t() instead which will run check_plain() for you. Make sure to read https://www.drupal.org/node/28984 again. Also elsewhere.
6. "$drupal_username_generated = xing_connect_unique_user_name(check_plain($xing_username));": if you are not printing anything to HTML then you don't need the check_plain() call.
7. "'mail' => check_plain($xing_candidate_info['data']['users'][0]['active_email']),": "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984 , so the check_plain() is wrong here.
8. xing_connect_authentication(): the check_plain() for the user picture file name is also wrong. Use file_munge_filename() instead.
9. "drupal_alter("xing_connect_register", $fields, $xing_candidate_info['data']);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php

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

Removing review bonus tag, you have not added any additional reviews to the issue summary? Make sure that you have at least 6 reviews listed before you add the tag again.

I read only your module, didn't debug it, but what's line 156:

    return (xing_unique_user_name($trimmed_name, $i));

What is xing_unique_user_name?

Best wishes,

maen

Hi Ajay,

the very first link in your description should be dev.xing.com, not developer.xing.com!

Automated Review
No pareview errors found

Manual Review

Individual user account
Yes follow the guideline

No duplication
Yes follow the guideline

Master Branch
Yes follow the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md
Yes follow the guideline

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Yes: Meets the security requirements

So in short:
As I already stated, the function xing_unique_user_name() was the wrong name, but this is corrected.
The link for the xing app is wrong, should be dev.xing.com.
Then after setting it up, and configuring who can administer the xing login, I tested it.

But I got always an access denied while clicking on "login with xing" with url "http://immo-maen.rhcloud.com/de/xing/auth?destination=xing/auth".

For me as reviewer is not clear if I have to debug the whole code now to find the problem???

In my pov this module should be tested by creator again, not by me!

After looking a bit more detailed you have to add
'access callback' => TRUE, to your menu.

Then the app works as designed!

manual review:

1. xing_connect_form_alter(): why do you have #prefix and #suffix markuo here? Can't you move all markup to the template? And you should not use #markup here, use #theme with the name of your template/theme functions. See https://www.drupal.org/node/930760
2. "drupal_set_message(check_plain(t('Xing API keys/secreat key is not configured correctly.')), 'warning');": the check_plain() is wrong here, this is just a staic string and no user provided content is involved, so it should be removed.
3. xing_connect_submit(): that function does not seem to be used? Is that a form submit callback? In what form is it used? Please improve the doc block, see https://www.drupal.org/coding-standards/docs#forms
4. "drupal_alter("xing_connect_register", $fields, $xing_candidate_info['data']);": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
5. "drupal_set_message(check_plain(t('You have been registered with the username !username', array('!username' => $account->name))));": instead of using check_plain() on the translated message you should use "@" or "%" placeholders with t() instead which will run check_plain() for you. Make sure to read https://www.drupal.org/node/28984 again.

But otherwise looks good to me.

Assigning to Naveen as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, ajayNimbolkar!

I updated your account so you can 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 stay 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.