[D7] Commerce Cardsave

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmalovanets2179745git

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.

Overall very clean code and looks to be a well implemented integration module. Below are a few items I found.

commerce_cardsave.admin.inc
Line 129: Imploding a single array item doesn't do anything.
Lines 75, 172: Consider using REQUEST_TIME instead of time(). REQUEST_TIME is set at the beginning of the script and is generally faster than time().

commerce_cardsave.api.inc
Line 48: Making a copy of the global user object is safer so you don't accedentily modify the global. Consider $user = $GLOBALS['user']; instead of global $user;
Line 295: Needs a space before the closing / in <br />

commerce_cardsave.module
Line 184, 214: REQUEST_TIME vs time()
Line 372: use check_plain($api->getMessage())
Line 452: Don't pass variables to t(). Use check_plain() instead.

hi @malovanets

commerce_cardsave.module line 144
$form['js']['#markup'] = '<script type="text/javascript">window.document.forms[0].submit();</script>';

If there is other form, there is a possibility to submit them.　So you should specify form id to submit.

PAReview is still reporting issue on the GIT Repo. Please address them to move forward.

@malvanets, yes you are right. Those are not application blocker, but its good to adhere to basic coding standards.

Project applications with Review bonus takes priority. So please complete manual reviews & include those url's here. Once done, please include appropriate tags in the application.

Note: these are not review comments, they are are simple suggestion on project application.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

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.

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. The project page should mention that this is a payment gateway for Commerce, right? Currently it is just an advertising text from Cardsave and does not help users decide, please follow the tips for a great project page: https://drupal.org/node/997024
2. commerce_cardsave_3ds_init_form(): no need to call drupal_render() here, just return the form render array, it will rendered later for you.
3. '#markup' => '<p>Please log into your merhcant account a...: All user facing text must run through t() for translation.

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

Assigning to patrickd as he might have time to take a final look at this.

Sorry I'm currently not able to do a proper review of this project

Assigning to dman as he might have time to take a final look at this.

Hey. It's tricky to review full third-party integration things without setting up full accounts, so I'll just give the code a standard look, taking into account the existing RTBC...

* Thanks for the README, it is clear and helps.
* Code is clean, comments are clear and useful. Especially the attention to @params.
* The heavy use of SESSION variables is disturbing, but I won't second-guess that it's needed. The whole package looks complex.
* Generally, supremely readable code!
* I would warn against your getXMLValue() function. XML used to be hard, but doing it with regexp will bite painfully one day. Spend a little time with the PHP XML libraries. Ditto on your generateXML() that builds XML with string replacement. One day someone will have a @password that includes a < or a product name that includes an ' and bad things will inexplicably happen.

I'm unable to test the full end-to-end functionality, or even the UX, but as PHP, code, and Drupal stuff goes, this is looking really good.
I can't visually see any hidden security issues - not without spending a day or more digging into the protocol itself. But the solid coding conventions exhibited here give me confidence it's well-built.

-----------------------

Thanks for your contribution, malovanets!

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.