This project adds support to the Commerce module for payment via the Paymill payment service:

http://www.paymill.com/

I've put the code through PAReview already:

http://ventral.org/pareview/httpgitdrupalorgsandboxgazoakley1840226git

There are a couple of issues showing up for "all functions should be prefixed with your module/theme name to avoid name clashes" - this appears to be a result of including another module purely to provide libraries support for Paymill's PHP library - details of how to install this are available in the included README.txt. I'd probably split an actual release up so that other modules can take advantage of Paymill's API without need for the Commerce module.

One unusual design choice is that in commerce_paymill_submit_form I render a form out and then include it again as a markup element. This is done so that the originally rendered form does not have name attributes for input elements - this prevents credit card details from being sent back when the form is submitted (card details are instead sent to Paymill directly from the client using Javascript). There might be a better way of doing this, but I couldn't find one myself.

Summary

Project page:
http://drupal.org/sandbox/gazoakley/1840226

GIT link:
http://git.drupal.org/sandbox/gazoakley/1840226.git

GIT clone:
git clone http://git.drupal.org/sandbox/gazoakley/1840226.git commerce_paymill

Reviews of other projects

I'm planning on reviewing other projects to qualify for the review bonus as time permits. I'll edit this as I carry out reviews.

Comments

grisendo’s picture

grisendo commented

There is something on commerce_paymill.module in line 276:

drupal_goto('admin/commerce/config/payment-methods/manage/commerce_payment_commerce_paymill/edit/3');

Why 3? It looks like it's specific value in most drupal installation (since it's OK initially), but not for general purpose... What if anyone accidentally deletes that action and later recreates it? It won't be 3.

"Configure" link for this module in Module list page (admin/modules) sends to a wrong page when Payment UI or Commerce UI are not enabled. Please, show a warning message in that case telling the user that for configuring options, Payment UI and Commerce UI are required to be enabled. Right now I can't remember if Rules UI is also required for configuring, please, check it also.

grisendo’s picture

grisendo commented
Status: Needs review » Needs work
gazoakley’s picture

gazoakley commented
Status: Needs work » Needs review

Hi grisendo,

Thanks for your code review :-).

I've committed changes which:

  • Check the Payment UI module is enabled (this also requires the Rules UI and Commerce UI to be enabled)
  • Check the default payment rule configuration for the module is in place
  • Check an action is available for the default payment rule configuration
  • Redirect the user to edit the action for the default payment rule configuration
  • Redirect the user to an appropriate page if an error occurs

It's possible a user could have replaced the first rule with a completely different action - but I don't think this will happen often and I've raised an issue to filter for the first payment action: http://drupal.org/node/1842642

PAReview should be clean once again as well.

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Adding referred code review

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Adding another peer review

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Added another project review

gazoakley’s picture

gazoakley commented
Issue tags: +PAreview: review bonus

Other reviews showing in issue, adding review bonus tag

gazoakley’s picture

gazoakley commented
Issue tags: +PAReview: Commerce

This should also have the commerce tag

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus +PAreview: security

manual review:

  1. You have a submodule that has a completely different name, which is bad practice. Usually all modules in one project should have the same prefix to avoid name collisions. You could move paymill_api to its own sandbox? But I think that additional module is really not necessary, just move your library hooks to the main module.
  2. commerce_paymill.js: use Drupal.behaviors instead of jQuery(document).ready(). See http://drupal.org/node/304258 and http://drupal.org/node/756722
  3. "define('PAYMILL_BRIDGE', 'https://bridge.paymill.com/');": all constants need to be prefixed with your module's name to avoid name clashes.
  4. "global $_commerce_paymill_settings;": why do you need a global variable? Please add a comment. Better use $form_state?
  5. "form_set_error('', $transaction['Error']);": has the error message been sanitized before? Text from third parties is untrusted user provided input, so we need to protect against XSS exploits. Could you either use check_plain() or add a comment where in the paymill API sanitization is done?
  6. commerce_paymill_loadjs(): this is vulnerable to XSS exploits. If I enter 5'; alert('XSS'); var x = '5 as public test key I get a nasty javascript popup on the checkout page. You need to sanitize user provided text before printing. And this case you should use Drupal.settings to pass down PHP variables to javascript, which will automatically escape malicious text.

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

gazoakley’s picture

gazoakley commented
Status: Needs work » Needs review

Hi Klaus,

Thanks for your review. I've gone through and made changes as requested:

You have a submodule that has a completely different name, which is bad practice. Usually all modules in one project should have the same prefix to avoid name collisions. You could move paymill_api to its own sandbox? But I think that additional module is really not necessary, just move your library hooks to the main module.

I've merged the code from paymill_api to commerce_paymill.
 

commerce_paymill.js: use Drupal.behaviors instead of jQuery(document).ready(). See http://drupal.org/node/304258 and http://drupal.org/node/756722

Done - commerce_paymill.js has been updated.
 

"define('PAYMILL_BRIDGE', 'https://bridge.paymill.com/');": all constants need to be prefixed with your module's name to avoid name clashes.

Done - commerce_paymill.module has been updated.
 

"global $_commerce_paymill_settings;": why do you need a global variable? Please add a comment. Better use $form_state?

No longer required - I'm now using ['#attached']['js'] to pass the settings across.
 

"form_set_error('', $transaction['Error']);": has the error message been sanitized before? Text from third parties is untrusted user provided input, so we need to protect against XSS exploits. Could you either use check_plain() or add a comment where in the paymill API sanitization is done?

Now uses check_plain.
 

commerce_paymill_loadjs(): this is vulnerable to XSS exploits. If I enter 5'; alert('XSS'); var x = '5 as public test key I get a nasty javascript popup on the checkout page. You need to sanitize user provided text before printing. And this case you should use Drupal.settings to pass down PHP variables to javascript, which will automatically escape malicious text.

Whilst these settings are only editable by those with the "administer payment methods" permission (normally only given to User 1) this should be resolved by using Drupal.settings to set up this variable on the client.

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Added another project review

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Adding more reviews

gazoakley’s picture

gazoakley commented
Issue tags: +PAreview: review bonus

Re-adding review bonus (see original issue for review links)

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Added another review

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Adding another review

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

manual review:
commerce_paymill_transaction(): @return doc is missing, see http://drupal.org/node/1354#functions

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

gazoakley’s picture

gazoakley commented

Cheers Klausi, I'll try and get that tweaked shortly :-)

gazoakley’s picture

gazoakley commented

Fixed - will work to review bonus tag again

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Adding another review

gazoakley’s picture

gazoakley commented
Issue tags: +PAreview: review bonus

Re-adding review bonus (see original issue for review links)

gazoakley’s picture

gazoakley commented
Issue summary: View changes

Adding more code reviews

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

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

But otherwise there were no objections for more than a week, so ...

Thanks for your contribution, gazoakley!

I updated your account to let you 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:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get 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.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Adding another code review