[D7] Commerce FirstData Connect

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

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

Given that https://www.drupal.org/project/commerce_firstdata is seeking a new maintainer, and hasn't had a commit in a long time (I am assuming the 2013-09-30 was part of the general site wide repackaging), is there any chance that you can fold your functionality into that module or take that project over?

And just as a side note, this module is pretty huge. It is going to take us a while to read through about 2700 lines of code, so your patience with the process is appreciated.

Automated Review

No best practice issues identified by pareview.sh

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.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Unsure: Does not cause module duplication and fragmentation.

Reviewing @mpdonadio 's comments at #8, perhaps looking into adopting that module through the abanded module process, and merging the functionality of your sandbox project into it?

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes/No: 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.

Does not follow the README template, consider modifying the contents to better fit the template, for consistency acrossed all projects.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

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

My concern is with commerce_firstdata_connect.module, function commerce_firstdata_connect_cardonfile_update. It appears you are using $_GET and $_POST to set $params, and using these values else where. I do not see anywhere that directly displays these values, but have not been able to fully test the module fully due to it's integration complexity. Check into the possibility of using check_plain() or similiar if appropriate. Security is of utmost importance especially when dealing with credit card data.

I'm not going to tag this as a security issue, yet, as I've not been able to fully untagle how the data from the GET/POST is handled.

Coding style & Drupal API usage

1. (*) Major finding
2. Minor finding
3. (+) Release blocker
4. (*) Major finding

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.

This review uses the Project Application Review Template.

Setting as my next review.

BTW, this is a big, dense module. I may not have a review done until tomorrow. I'm sorry for the delay, but your patience is appreciated.

deleted double submit

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 35c5c8f):

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

This was another module that exposed a bug in PHPCS and/or CodeSniffer, so results may not be accurate.

Manual Review

Individual user account

Yes/No: 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, but you need to set the default for the project.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

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

(*) commerce_firstdata_connect_createHash, use drupal_hmac_base64() instead of sha1().

(*) In the API callbacks (eg, commerce_firstdata_connect_3d_secure_callback), how do you know you have a legit callback and not a spoof? Is there a token that gets checked? I am not seeing this.

Coding style & Drupal API usage

WHy your own required markup / styles for required fields? Drupal already provides this. Also, using !important will make it much harder for someone to change how this looks.

In commerce_firstdata_connect_capture_form(), you have some double translated strings (line 35). This will also lead to double escaping. You can just use $description there; the t() w/ @ placeholders made it safe.

commerce_firstdata_connect_capture_form() has some bad indentation around the confirm_form() bit.

drupal_goto() shouldn't be called from a form submit handler; $form_state['redirect'] is preferred.

In commerce_firstdata_connect_capture_form_submit(), $response_data_codes may used initialized on line 123.

(+) It looks like commerce_firstdata_connect_cancel_form_submit() has an untranslated string near the end.

commerce_firstdata_connect.api.inc is a class and should be listed in the files[] in the .info

The normal convention with Commerce stuff is to use entity_metadata_wrappers instead of directly poking into the entities themselves.

(+) The methods on CommerceFirstdataConnectConnectApi() need proper docblocks with parameters, return, etc.

CommerceFirstdataConnectConnectApi::directPayments() should use URL to build up the urls instead of $base_root.

CommerceFirstdataConnectConnectApi:buildUrl(), look into drupal_http_build_query()

CommerceFirstdataConnectConnectApi::commerceFirstdataConnectDevice(), UA detection and reliance is very fragile. Avoid it when possible.

CommerceFirstdataConnectConnectApi::crossPayment(), what is the sleep() for. Comment needed.

Why the cache_clear_all in commerce_firstdata_connect_commerce_cardonfile_checkout_pane_form_alter(). Comment needed.

(+) commerce_firstdata_connect_commerce_cardonfile_checkout_pane_form_alter() has a handful of untranslated strings. Double check the module.

In commerce_firstdata_connect_submit_form_validate(), why two checks for $validate? Comment needed.

In commerce_firstdata_connect_submit_form_submit(), $api appears to be unused.

commerce_firstdata_connect_commerce_payment_method_info(). Why the comment about Ogone?

In commerce_firstdata_connect_cardonfile_update(), which the check_plains? These don't appear to be going to output? See https://www.drupal.org/node/263002 and https://www.drupal.org/node/263002

In commerce_firstdata_connect_hosted_fail_callback(), you don't need to use $base_root with url(). Also elsewhere.

In commerce_firstdata_connect_hosted_callback, you have a drupal_goto(t()). I suspect you wanted something else here. drupal_goto() will internally use url(), which makes the URL safe. Also elsewhere.

This module could use a fair more comments and/or vertical whitespace to separate out logical blocks.

(+) Does commerce_cardonfile need to be a dependency? It looks like in some places you feature check, but not always.

commerce_firstdata_connect_settings_form() has a variable_get() w/o a default value. Also elsewhere.

(*) I am not positive if your handling of $time_zone_string in commerce_firstdata_connect_settings_form() is truly save. check_plain() or use t() with placeholders.

[module line 1226]

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.

The security issue is an easy fix, but this module needs a second sets of eyee regardless to look at it. It is over 2500 lines of dense code.

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

This review uses the Project Application Review Template.

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 7824295):

• (*) PHP Fatal error: Default value for parameters with array type hint can only be an array or NULL in ./includes/commerce_firstdata_connect.api.inc on line 239
  
  Errors parsing ./includes/commerce_firstdata_connect.api.inc
• 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_firstdata_connect_batch_operation(): $_SESSION['http_request_count']++; Usage of session doesn't make any sense here, IMO that's not the right way to do this at all. Use the batch sandbox to store details from one batch iteration to the next, it's what it's there to do.
   

   foreach ($all_transactions as $key => $value) {
       $transaction_id = $value->transaction_id;
   
       $operations[] = array(
         'commerce_firstdata_connect_batch_operation',
         array(
           $transaction_id,
           t('(Operation @operation)', array('@operation' => $key)),
         ),
       );
     }
   

   commerce_firstdata_connect_batch_process(): That's not the best way to use BatchAPI. You should only add the operation once, and let the operation iterate over a set number of results, using the sandbox to mark how far it got each time.

2. (+) commerce_firstdata_connect_get_response_data(): Lack of comments and unfriendly variable names make it hard to understand this code. Same thing applies at other places too in your code as huge code is there :)
   $new_array = array_combine($names_array, $values_array);

   $names_array and $values_array both are not declared within this function, in any case if $exp comes empty then unnecessary notices will be produced.

3. commerce_firstdata_connect_settings_form(): '#default_value' => !empty($settings['url']) ? $settings['url'] : 'test', Looks like default value is 'test', not meaningful at all.

   commerce_firstdata_connect_redirect_form(): if ($payment_methods['settings']['url'] == 'test') {, again 'test' coming within if condition, comment needed?

4. commerce_firstdata_connect_submit_form_submit():
   

   foreach ($new_form as $k => $v) {
         if ($k != (count($new_form) - 1)) {

   count() inside loop for same array is not a good practice, I would recommend you to store count once and use the same variable further. Check the same for other places too.

5. hook_help() is missing.
6. Project Page: It contains enough information but looks like not well organised, better to go through Tips for a great project page again.
7. (+) commerce_firstdata_connect_settings_form() :
   

   '#description' => t('Please enter your store id. For more information, visit <a href= "https://www.firstdatams.co.uk/fdms/en_gb/home.html"> firstdata web site </a>'),
   

   In form elements, you are directly keeping hyperlink within markup string instead insert hyperlink markup into the translatable string with placeholder. See https://www.drupal.org/node/322774.

8. You have hardcoded amount value to 10 at 13 places in your code either make it configurable from admin or define as a constant. Also your comment needed why it is 10?
9. commerce_firstdata_connect_get_response_code() : Lot of if statement coming within this function, addition of inline comment would be good.

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.

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

Thanks!

Automated Review

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

• 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

commerce_firstdata_connect_batch_rule(): $_SESSION['http_request_count'] = 0; not in use, so remove it.

My all #21 concerns have been addressed or answered, I'm not seeing any blocking issue further, setting RTBC to this.

Thanks for your contribution, veso_83!

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.