This module adds new payment method for Drupal Commerce allowing users to complete checkout using Polish offsite payment gateway Dotpay (dotpay.pl).

After redirect user can choose one of many available payment options including all Polish banks quick transfers, credit card payments, cash points like 7-11 shops and more.

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/philipz/2223349.git

Sandbox page: https://drupal.org/sandbox/philipz/2223349
Git repository: http://git.drupal.org/sandbox/philipz/2223349.git
Pareview.sh report: http://pareview.sh/pareview/httpgitdrupalorgsandboxphilipz2223349git

Comments

PA robot’s picture

PA robot commented

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.

thomas.fleming’s picture

thomas.fleming commented
Status: Needs review » Needs work

Hey philipz,

Great seeing the module pass http://pareview.sh/ without issues :)

After a manual review, I noticed that in commerce_dotpay_urlc() in commerce_dotpay.inc that exit is used. Those exits should really be return FALSE.

Additionally, although I think there must be a better way to get $_POST values without making such a direct call, I have a hard time arguing against it, since Commerce PayPal also does this. This is, however, something mentioned in the Security Review of the Project Application Checklist: http://pareview.sh/pachecklist.

philipz’s picture

philipz commented

Thank you for the review! I did spend good amount of time trying to squash all pareview.sh complaints before posting application review :)

These two things you've reported are two things I didn't like myself but I'm not sure if there's a better way of doing those.

First the exit() - the only thing I could think of is using instead is drupal_exit() since there is nothing more that needs to be done. I found that in other payment modules that have such payment notification endpoint - the one that responds to payment request notification coming directly from the provider - a similar exit function at the end.
Otherwise just returning FALSE would end up rendering drupal page which is not technically bad in Dotpay's case but seems a bit unclean.

Now the $_POST values are done exactly like Commerce PayPal does it and I've search how to get them in better way but couldn't find it.
Commerce PayPay even says in a comment that

// Note that Drupal has already run stripslashes() on the contents of the
// $_POST array at this point, so we don't need to worry about them.

thomas.fleming’s picture

thomas.fleming commented

I think if the exits are at least changed to drupal_exit(), everything else looks good to me.

philipz’s picture

philipz commented
Status: Needs work » Needs review

OK I've chagned those exit() calls to drupal_exit() and commited that.

thomas.fleming’s picture

thomas.fleming commented
Status: Needs review » Reviewed & tested by the community

Reviewed and went through PA checklist. Looks good to me.

MattWithoos’s picture

MattWithoos commented
Status: Reviewed & tested by the community » Needs work

Hi, and thanks for your contribution!

$urlc = $_POST;

I couldn't understand the purpose of this after a quick glance & read of the comments. If possible, use the Drupal API (ie https://www.drupal.org/coding-standards/docs#forms) to avoid $_POST.

Is it getting a URL? If so, can't you use current_path()?

I couldn't find anything else wrong. Just need some clarification.

Cheers

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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