[D7] Commerce DirecPay

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2382569

Project 2: https://www.drupal.org/node/2338629

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

+1
I required these module for my project.

I was seaching for this module.
Thank you so much :)
+1

For review by me

Manual Reviews

- Remove the packaging lins from the .info. These will be added when the module is published.
- Drupal prefer drupal_base64_encode() over base64_encode(). Please use drupal_base64_encode() instead of base64_encode() function.

Assigning to myself for next review.

Automated Review

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

• 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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

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/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) #value is not plained by the Form API (#defaul_value) is. The addresses in commerce_direcpay_order_form() need to be run through check_plain(), as these are use input going
to output. See https://www.drupal.org/node/28984

I am assuming that the transaction message is using !arg placeholders because this just gets saved in the database? If so, this is not a problem, but I am unfamiliar with this exact mechanism.

(*) commerce_direcpay_redirect_form_validate() is using a !arg placeholder in 234. This should really be an @arg or %arg (and a print_r of the whole response is likely overkill).

It is always best to use @arg or %arg placeholders unless you are know you are *positive* dealing with safe HTML that needs to be shown as HTML.

Coding style & Drupal API usage

(+) Where did the merchant ID in commerce_direcpay_settings_form() come from? Comment needed (preferable w/ source if this is a test ID from DirecPay).

commerce_direcpay_redirect_form() doesn't have a proper docblock. Do the stock Commerce gateways have ones for this function?

You are explicitly using the Entity API in your module. Though commerce required this, I think it is best to add this to the .info file, too.

Why the urldecode() in commerce_direcpay_order_form? Comment needed.

$requestparameter is ununsed in commerce_direcpay_order_form().

Why does commerce_direcpay_redirect_form_validate() access $_POST directly? Comment needed.

Is there a DirecPay API guide? You should probably have a @see somewhere that references it. Also a little surprised
that this is direct form posting, and not drupal_http_reqquest() or cURL based, as most payment gateway modules are.

In commerce_direcpay_redirect_form_validate(), I am not seeing $mode go to HTML output, so I think the check_plain() is superfluous. It was also coming from an option, which was validated by the Form API.

In commerce_direcpay_encode_string(), comment why drupal_base64_encode() can't be used.

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.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

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

• 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_direcpay_redirect_form_validate(): why do you array_map() check_plain() onto all of the response? You should only do that when you actually print something of it to HTML. See https://www.drupal.org/node/28984
2. commerce_direcpay_redirect_form_validate(): where do you check the the data in $_POST is actually coming from directpay? Shouldn't you verify some token here that makes sure the request is valid? What prevents an attacker from sending a fake POST request that spoofs directpay data? This looks like a security issue to me.

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

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

I've just checked your project with pareview.sh and coder as well as did manual review.

Done some successful and failed transactions too, it works smoothly.

It looks RTBC to me.

Sorry for the delay. Reviewing more project applications and a new review bonus makes project applications go faster.

I didn't know about the payment_redirect_key that commerce_payment provides for you, so that seems to be in order.

Thanks for your contribution, Sagar Ramgade!

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.