Commerce Payment Network

I ran your project through ventral's online PAReview code checker, which noted out that you're doing your work on the master branch rather than a version-specific branch (here are some instructions on making the move). It also reported a series of coding style issues which are listed in the attachment.

When I took a look at your code by hand I didn't spot any other significant issues with the code itself but I did notice that while nearly all of the comments are in English but one block (the function comment for commerce_payment_network_feedback_status()) is in German. Perhaps I'm overlooking a reason for this.

It would also be a good idea to flesh out the project's description page. The text in your README would make a very good start.

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732

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

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. Get a review bonus and we will come back to your application sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

commerce_payment_network.module line 137
global $user is defined but never used so you should remove the global $user.

commerce_payment_network.module line 39
Use t() function so that the message is translatable. Use English as the base language:
watchdog('commerce_payment_network', 'Empfangen: ' . $_POST['order_id'], array(), WATCHDOG_INFO);

Sorry for my wrong information but the t() function doesn't belong inside watchdog. This was my bad!

Hi forward-media

1) commerce_payment already depend on commerce : no need of this dependency
2) You should maybe use a Constants for the url : define('URL_SOFORT' , 'https://www.sofortueberweisung.de/payment/start');
3) Is your api ever called ?

Good work though.

Hi,

sorry for the delay!
as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

Improve the module descritpion page. Add a short description what the module does. The content of README.txt seems good enough (not just a reference to the file).

• No module duplication issues found.
• The code looks good according to Drupal's coding / comment standards.
• There are no licensing issues. No third party code is included.

All existing issues got resolved.

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). 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. Get a review bonus and we will come back to your application sooner.

manual review:

• "watchdog('commerce_payment_network', 'Received: ' . $_POST['order_id']": do not concatenate the translatable string to watchdog(), use placeholders instead. Also, this is vulnerable to XSS exploits if I send a malicious script in the order_id.
• commerce_payment_network_notification(): $notification is unused.
• commerce_payment_network_notification(): anyone can create random watchdog entries on this path, meaning I can totally flood your database. You should at least verify that the order is valid or the hash or whatever.
• "define('URL_SOFORT','https://www.sofortueberweisung.de/payment/start');": all constants must be prefixed with your module's name to avoid name collisions. Also, you should define your constants at the top of your module.

Also, you have not posted any reviews of other project applications in your issue summary as recommended in https://drupal.org/node/1011698

+1 Quality assurance
-1 nitpicking (will not help community growth)
+1 Pareto principle
+1 for releasing this as a full project.

• Code looks good
• Maintainer is responsive, issues are resolved

manual review:

1. "define('URL_SOFORT', 'https://www.sofortueberweisung.de/payment/start');": all constants must be prefixed with your module's name to avoid name collisions.
2. You have syntax errors in your module file.
   

   >$ php -l commerce_payment_network.module                                                                       
   PHP Parse error:  syntax error, unexpected ')' in commerce_payment_network.module on line 37                                                                                                  
   Errors parsing commerce_payment_network.module                                   
   

3. "watchdog('commerce_payment_network', 'Received: ' . $_POST['order_id'], array(), WATCHDOG_INFO);": this is still XSS vulnerable.

just rename it to define('MODULE-NAME_URL_SOFORT', ...)

commerce_payment_network.info

Add commerce as a dependancy.

commerce_payment_network.api.php

Line 36 :

improperly calls isset():

$data['LANGUAGE'] = isset $language_mapping[$language->language] ? $language_mapping[$language->language] : $settings['language'];

should be:

$data['LANGUAGE'] = isset($language_mapping[$language->language]) ? $language_mapping[$language->language] : $settings['language'];

commerce_payment_network.module

Line 9:

Should your COMMERCE_PAYMENT_NETWORK_URL_SOFORT be a setting field instead of a defined constant so that users could change it if the payment gateway ever did?

Line 55:

'description' => t('Integrates payment_network payment system'),

should be

'description' => t('Integrates Payment Network payment system'),

Line 91:

'#description' => t('The ID from your Projekt, find in your payment network backoffice'),

should be (???)

'#description' => t('The ID from your project, find in your payment network backoffice'),

Line 124:

Errors in commerce_payment_network_redirect_form() should be added to watchdog if they are related to failed transactions or misconfigurations of payment gateway.

Line 135:

global $user is still defined even though it is not used/needed.

Line 141:

$customer_name is defined but not used. Should this be added to one of the user_variables in your $data array?

Line 182-201:

Could this be simplified?

array_push($data, $settings['project_pass']);
$hashdata = array_values($data);

Setting status to needs work

Hello guys,

(not forward-media) what is currently the problem to switch this project to a full project?

I'm working local to extend and optimize the Commerce Payment Network module. Guys, you can be sure that I'm keeping an eye on the code (code and style standards, etc.) in the next time.

You can still talk and post a long time. But, this might not really help the development of the module.
The more time goes by, the higher will be the frustration of the maintainer! And that's not good.

Please not longer behave like children. :-)

So, now it's time!

I've forgotten it, in #31.

This is a review bonus.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

@quiptime: oh, you are not the actual applicant ... then why don't give the project a decent review and push it forward to RTBC as indicated in the workflow guidelines?

@klausi,

to write a traditional review bonus, according to the rules is not possible to me.
My time is too precious to write a traditional review bonus.

Why?

I'm working local to extend and optimize the Commerce Payment Network module. Guys, you can be sure that I'm keeping an eye on the code (code and style standards, etc.) in the next time.

So I'm already several steps further than just

• to test if it works the module
• to examine the code standards
• to examine the comment (doxygen) standards
• to examine other things to be aware of

Against the background of what has been said up here:

I give a review bonus for this sandbox module.

@klausi, I very much hope that you can understand what I want to say above.

In the meantime

Meanwhile, I have spoken with the maintainer directly.

The maintainer is very frustrated with the process to contribute this sandbox module. I can understand him.
There is the possibility that he will delete this sandbox project. I hope he does not do this.

Concluding

@klausi,

make it simple. Give the project the full status.

Or, give permission to the maintainer to create full projects.

Consider me as a mentor of this project.