[D7] Deezer

1. Please use unix-style line endings (all files);
2. deezer.theme.inc - line 48 - add comma at the end of array;
3. deezer.js - line 31 - add semicolon;
4. deezer.pages.inc - line 225 - i recommend to make redirection customizable or overridable (drupal_goto('') is hardcoded);

Add empty line to the end:
README.txt, deezer.info

deezer.install

add hook_install
variable_set - deezer_app_id, deezer_secret_key etc

deezer.module

make DEEZER_ACCESS_TOKEN_URL, DEEZER_API_URL and DEEZER_USERPIC_SIDE variables.

Be sure to test through pareview: http://ventral.org/pareview/httpgitdrupalorgsandboxvgrotov1993038git You can get a lot of hints there.
Also, run JS through a tool like JSHint (http://www.jshint.com/) which will help identify some potential issues and save you time here.
~R~

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

removed automated reviews.

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

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

  
  FILE: /home/klausi/pareview_temp/deezer.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   352 | WARNING | Code after RETURN statement cannot be executed
  --------------------------------------------------------------------------------
  

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. deezer.module: why do you have to include deezer.tools.inc globally on every single page request? You should only include files when you actually need them, i.e. in function bodies. And if you provide an important API function it should live directly in the module file.
2. deezer_init(): writing to the global session on every single page request is bad, because this will break page caching. As soon as a client has a session it will not receive cached pages and will be passed through to Drupal. So your module could have a severe performance impact on larger sites that rely on page caching.
3. deezer_menu_alter(): why do you need hook_menu_alter() if you are altering your own paths? Please add a comment.
4. deezer_user_profile_form(): '#markup' => '<br /><b>' . $deezer_user['name'] . '</b>',: this is vulnerable to XSS exploit, since $deezer_user['name'] stems from an untrusted third party source. You need sanitize all untrusted user provided input before printing. Make sure to read https://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
5. Your are not using user_external_login_register() anywhere in your code, which is the proper way to handle external authentication systems.
6. Why do you add deezer.js on every single page request? Shouldn't that only be added when your block is displayed?

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

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

1. "isset($_SESSION['deezer_goto']) && !empty($_SESSION['deezer_goto'])": the isset() check is not needed here, since empty() will not throw notices if the array key does not exist.
2. deezer_record_save(): user_external_login_register() should do that for you, no? So you would not have to copy that?
3. _deezer_request(): do not use file_get_contents() for remote files, that fail on many security protected Drupal sites. Use drupal_http_request() instead.
4. deezer_login_callback(): why do you set $_SESSION['deezer_goto']? Just call drupal_goto() directly here?

That are not application blockers, otherwise looks RTBC to me.

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

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

Thanks for your contribution, Vasiliy Grotov!

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.