[D7] Commerce Multicurrency For Yahoo Finance

@th_tushar, first of all thanks for contribution!

Your module is so small, as per guidelines for project length and complexity I am tagging "PAReview: Single project promote" tag with this.

Here is my assumptions on your code after manual review:
1. $rates variable is not declared outside 'for loop' as it should be. In any case if $target_currencies is empty, it would produce notice.

2. In following code you have explode the $record variable in $currency_data variable. Are you sure every time it contains three values in array, if this is not the case then it would produce notice. Best practices to check array count of $currency_data variable. Also this variable is set through third party response ($record = $http_result->data;), so it would be better you should check response type and use sanitize function before final passing.

$currency_data = explode(',', $record);
$rate = $currency_data[1];
$date = $currency_data[2];
$time = $currency_data[3];

3. Where you are using $timestamp variable, please add comment.

$timestamp = strtotime(str_replace('"', '', $date) . ' ' . str_replace('"', '', $time));  

4. Also add hook_help() in your module.

Thanks Again!

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

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

Assigning to myself for next review.

Automated Review

Git default branch is not set, see the documentation on setting a default branch.
Review of the 7.x-1.x branch (commit aa088a4):

• Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting
  

  ./README.txt:                             ASCII English text, with CRLF line terminators
  commerce_multicurrency_yah.module
  README.txt
  

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/matt/PAR/pareview_temp/commerce_multicurrency_yah.module
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   325 | WARNING | Unused variable $timestamp.
  --------------------------------------------------------------------------------
  
  Time: 159ms; Memory: 6.75Mb
  

• 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.

FILE: /home/matt/PAR/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 23 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: /home/matt/PAR/pareview_temp/commerce_multicurrency_yah.css
--------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
  2 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  6 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
 10 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
 14 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
--------------------------------------------------------------------------------


FILE: /home/matt/PAR/pareview_temp/commerce_multicurrency_yah.module
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
   1 | ERROR | [x] End of line character is invalid; expected "\n" but found
     |       |     "\r\n"
 380 | ERROR | [ ] Files must end in a single new line character
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Time: 325ms; Memory: 9Mb

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Follows the guidelines for in-project documentation and the README Template.

I think both the README and project page need improvements.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

(*) It looks like the $active_currency needs to be plained in commerce_multicurrency_yah_getrates_forall on line 365. See https://www.drupal.org/node/28984

(*) It looks like $site_default_currency_name is used unsaniticed on line 371. Use t() with a placeholder here as that string needs to be translated.

Coding style & Drupal API usage

You don't need quotes around items in the .info file

I would add commerce as a dependency.

(+) The namespace in your css should match the namespace of your module.

Avoid linebreaks in the strings passed to t() (eg, commerce_multicurrency_yah_help). It makes translation harder. Don't linewrap these just to appease PAReview.

(+) Your permission names should be more closely aligned with your module name. What is there is an unrelated module that integrates with Yahoo! finance?

The access arguments in commerce_multicurrency_yah_menu() are specified twice.

(+) The namespaces in commerce_multicurrency_yah_theme() should be more closely aligned with your module name.

The paragraph in the output of theme_yahoo_fin_currency() isn't needed.

(+) theme_yahoo_fin_link() should build the URL paramaters via the options to l() and not manually.

Is commerce_multicurrency_yah_commerce_multicurrency_exchange_rate_sync_provider_info() a hook? If so, it needs a proper docblock.

The type signature of commerce_multicurrency_yah_form() is wrong if that is a proper Drupal form. It should be commerce_multicurrency_yah_form($form, &$form_state).

(*) Why the use of $_SESSION in commerce_multicurrency_yah_form() and commerce_multicurrency_yah_form_submit()? Comment needed. Or maybe you just need to use
$form_state['values']? I don't quite understand this. In general, the use of $_SESSION in Drupal form handling should be avoided, especially if the variables you
are setting stay within the form. I am marking this as a blocker, as it looks like a big bypass of the Form API.

In commerce_multicurrency_yah_form() you have some places where you create #markup and then call theme(). Don't do this. Use the #theme element and build
up a proper render array.

In commerce_multicurrency_yah_getrate(), but up the URL parameters with drupal_http_build_query() instead of manually.

commerce_multicurrency_yah_getrates_forall() needs a proper docblock.

I think commerce_multicurrency_yah_getrates_forall() should return a proper render array and not themeed output directly?

In commerce_multicurrency_yah_settings_form(), the variable_get needs a default value.

Since you need to set variables other than what your module provides, commerce_multicurrency_yah_settings_form() isn't a system settings form? I think this needs to be commented.

(+) commerce_multicurrency_yah_settings_form() has some untranslated strings (eg, line 30). Double check the module.

You should avoid directly creating HTML element IDs. You can probably just use classes.

(+) You need a hook_uninstall to variable_del() the variables you are creating.

Avoid double spaces in strings to be translated, eg line 134.

In theme_yahoo_fin_currency on line 131, I think !isset won't catch all of the cases you need. empty() may be better here, thouch empty() doesn't work for floating point per the manual.

In commerce_multicurrency_yah_form, line 216, you have an untranslated string. Use t() and pass the URL as a placeholder.

In commerce_multicurrency_yah_form_validate(), just mark the element as required instead of using a validator. And, for $form['amount'] set an element level validator and use element_validate_number().

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Review of the 7.x-1.x branch (commit 5127e7b):

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

  FILE: /home/klausi/pareview_temp/css/commerce_multicurrency_yah.css
  --------------------------------------------------------------------------------
  FOUND 4 ERRORS AFFECTING 4 LINES
  --------------------------------------------------------------------------------
    2 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
    6 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
   10 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
   14 | ERROR | Line indented incorrectly; expected 2 spaces, found 4
  --------------------------------------------------------------------------------
  

• 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. commerce_multicurrency_yah_form_validate(): do not line break the translatable strings here. Same in commerce_multicurrency_yah_settings_form_submit().
2. "$headers = array('Currency', 'Exchange Rate', 'Link to Yahoo Finance stats.');": all user facing text must run through t() for translation.

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Fixed tags.

(+) You have a few t() calls with linebreaks (commerce_multicurrency_yah_settings_form_submit, line 96, 100 and elsewhere). This makes translation harder. Do a pass through the module before stable release.

theme_commerce_multicurrency_yah_link() has some commented out code; remove it.

My blocking issues from #9 look like they have been addressed. Nothing else jumped out at me.

Thanks for your contribution, th_tushar!

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.