[D7] Commerce Payfirma Payment Module

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.

No negative output from code sniffer, which is great

1. Don't use echo, always use print in the .module line 320
2. The code could do with a little polish, a lot of newlines that don't need to be there.
3. Use $form['#redirect'] instead of drupal_goto in your submit handler

Manual Review (visual only; I don't have a working Commerce install right now)

You should remove the commented out tests in the .info file

I would add a dependency on entity. Commerce requires this, but you are explicitly using entity_metadata_wrapper(), so the dependency is best in case Commerce changes.

You may want to add an index on order_id since you are declaring this as an FK.

The commented out code in the JS should be removed for the public (eg, lines 26, 74--77)

The naked items in commerce_payfirma_menu() give me pause. Can you point to best practice / other commerce payment modules that have handlers w/ access checks?

Is there a reason you have to use cURL directly and not drupal_http_request()?

You can probably get rid of the empty page arguments on line 19.

Setting this back to Needs Work, pending an answer about the naked hook_menu() items. Otherwise, I didn't see anything wrong that wasn't mentioned yet.

OK, I checked out commerce_paypal, and saw the 'access callback' => TRUE,. I am trusting Commerce Guys in this case, that this is OK.

Understood about curl. Add a comment to the effect about being modeled after the API docs, and also provide the link. Any other Payfirm API references would be good, as they may aid in debug if this module breaks at some point in the future.

Also comment the bit about the drupal_goto(). Any time you need to deviate from best practices or the API, it is always best to comment why.

Leaving this as Needs Review, but I will give everything a proper look over tomorrow.

Sorry, noticed a few more small things.

1. You aren't using the context in your JS behavior. This is important for AJAX.

2. You are using Drupal.settings.commerce_payfirma directly instead of the settings passed into the attached.

3. You have a unused variable on line 37 in the JS.

4. commerce_payfirma_submit_form() You should really use #attached on the form instead of drupal_add_js() and drupal_add_css().

As a side note, using a IDE like PhpStorm can help identify unused variables and arguments.

.module
PHPDoc comment does not match function or method signature (at line 218)
PHPDoc comment does not match function or method signature (at line 375)
hook_menu: Consider using https://api.drupal.org/api/drupal/includes%21menu.inc/constant/MENU_CALL.... That seems a better fit.
hook_commerce_payment_method_info: use l() for the link in the display title and pass it in via t() as a variable. Current approach isn't easily translatable.
commerce_payfirma_settings_form:

$form['logo'] = array(
    '#markup' => "<img src='/$path/images/powered-by-payfirma-large.png' />",
  );

Use '#type' => 'image' (see theme_image)
commerce_payfirma_process_response: Consider using php://input vs $_POST. See http://stackoverflow.com/questions/8893574/php-php-input-vs-post
No check if all the input values are passed. Null values are quite possible to a menu callback.
drupal_set_message(t('ERROR: @error'), array('@error' => check_plain($response['error'])), 'error');
No need to pass through checkplain. t() can do that for you.

.install
Duplicate array key (at line 80)

.info
No explicit dependency on commerce_customer.module

.js
Code comments should follow all the same standards as PHP code i.e. end in full stops. See https://drupal.org/node/172169
Alert should use Drupal.t(). See https://drupal.org/node/323109
Single letter variable names r, z, v make reading the js difficult. Consider using more meaningful names.

images:
Are all the images usable via GPL v2?

Moving back to needs work, primarily because of the non-translatable string and missing dependency.

In general comment, consider doing the extra work to make commerce_payfirma data an entity. This will make it usable by views and will mean you won't have to write code for loading, saving and deleting a payment. It's almost there already, you mostly need to add in the hook_entity_info. See http://www.phase2technology.com/blog/developing-a-custom-drupal-entity/

Great progress.

commerce_payfirma_process_response and commerce_payfirma_handler still don't check for !empty (or equivalent) for all values being gathered from the URL.

There's still an inherent assumption that certain values exist.

  if (!$order_id) {
    $order_id = $response['order_id'];
  }
  $order = commerce_order_load($order_id);

If this isn't called from commerce_payfirma_handler and it doesn't have a POST operation, then $response will be NULL and $order_id will be NULL and so will $order. Try entering the menu path into a browser without any parameters and see what happens.

In commerce_payfirma_handler, rather than using $_REQUEST super global directly, try using drupal_get_query_parameters.

Also, this was introduced:
397 | ERROR | Variable "payRement_method" is camel caps format. do not use
| | mixed case (camelCase), use lower case and _
Plus that value will always be empty (there's no assignment previously).

Last example:

  if (isset($pane_values['token'])) {
    $data['token'] = $pane_values['token'];
  }
  else {
    $data['card_number'] = $pane_values['credit_card']['number'];
    $data['card_expiry_month'] = $pane_values['credit_card']['exp_month'];
    $data['card_expiry_year'] = $pane_values['credit_card']['exp_year'];
    $data['cvv2'] = $pane_values['credit_card']['code'];
  }

This assumes that the $_REQUEST has card_number, etc populated. It wouldn't if someone only entered the menu path without any parameters.

@sleepingmonk, sorry about that. It must have been inserted. Moving to RTBCed. The next step is to wait for a git admin to promote.

Sorry for the delay, it took me longer than expected to confirm the hook_menu() issue mentioned above.

Automated Review

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

• 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, and may be duplicate results from Coder Sniffer. See attachment.

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.

Nothing major; fix when you can.

Manual Review

Add a comment somewhere in the code that Commerce Paypal was used as a model.

I think the second jQuery selector on line 25 is missing the context that was passed in.

Missing t() on commerce_payfirma.module, line 120.

It would be best to have some comments w/ URL that refer to the Payfirm API when you are programming against it (eg, the cURL, the responses).

Your date() functions should use format_date() w/ a custom string, and also explictly use REQUEST_TIME.

Form API #default_value gets plained internally. See https://drupal.org/node/28984

I am not seeing any blocking issues on this.

Thanks for your contribution, sleepingmonk!

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.