[D7] Commerce PesoPay

The README.txt lacks a brief description of the project. Here is the Drupal guideline I found useful: https://drupal.org/node/161085

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/README.txt
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   39 | ERROR | Files must end in a single new line character
  --------------------------------------------------------------------------------
  

• Codespell has found some spelling errors in your code.
  

  ./README.txt:3: Sponsered  ==> Sponsored
  

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. The Git commits are not connected to your user account. You need to specify an email address. See http://drupal.org/node/1022156 and http://drupal.org/node/1051722
2. "$log = 'Commerce pesopay module cannot load the transaction returned by the Pesopay server.' . $transaction_id;": do not concatenate $transaction_id into the translatable string, use placeholders with watchdog() instead.
3. commerce_pesopay_redirect_form(): do not document $form and $form_state, see https://drupal.org/node/1354#forms
4. commerce_pesopay_redirect_form(): doc block is wrong, $order is not an array?

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 patrickd as he might have time to take a final look at this.

Although your menu item is just a callback that will never face a user, I'd recommend you to set a descriptive title anyway. This helps to understand what the route does when a developer dumps the output of hook_menu_alter().

I don't see any advantage of the existence of this constant?
define('COMMERCE_PESOPAY_SECRET_KEY', '');

I'd at least comment this out when it's not used.. but.. that's not important..

 279 function commerce_pesopay_redirect_form_submit($order, $payment_method) {
 280   // Do nothing for now on submit.
 281 }

You have to think about whether this controller might be called with one or all of the POST variables missing..

 287   $src = $_POST['src'];
 288   $prc = $_POST['prc'];
...

Otherwise you'll end up with a lot of nonsense error messages in the logs.
I'd suggest you either prefix each of these $_POST variable accesses with @ so it won't produce error messages but will fail on hash validation anyway:

 287   $src = @$_POST['src'];
 288   $prc = @$_POST['prc'];
...

or you make sure that the variables are set in the first place:

if (!isset($_POST['src'], $_POST['prc'], ....)) {
  // log the invalid access.
  return 'some error message';
}

Though I couldn't find anything critical..

Thanks for your contribution!

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.