[D6] UC Braintree TR Gateway

Welcome,

please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxmarkhurley1820348git

I have installed and tested this module - everything functions well. I used a Braintree sandbox account to run the tests.

Some of the items tested:
- Single transactions
- Recurring transactions
- Role subscriptions as related to recurring transactions
- User permissions
- User security (Ensuring one user cannot access other user's profiles or subscriptions)
- Basic layout and functions

This is a well built module that is needed by the community.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

Review of the 6.x-2.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/uc_braintree_tr_payment.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 18 WARNING(S) AFFECTING 18 LINE(S)
  --------------------------------------------------------------------------------
   2496 | WARNING | Empty return statement not required here
   2573 | WARNING | Code after RETURN statement cannot be executed
   2708 | WARNING | Empty return statement not required here
  --------------------------------------------------------------------------------
  

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. gpl-3_0.txt: needs to be removed, all code on drupal.org is GPLv2+ and a LICENSE.txt file will be added for packaged downloads.
2. uc_braintree_tr_payment.module: that file has more than 3000 lines of code and is loaded on every single page request. Please try to split out stuff into include files that are loaded on demand.
3. uc_braintree_tr_payment.module: no need for the copyright header, you can give credit in README.txt
4. uc_braintree_tr_payment_redirect_error_handling(): all user facing text must run through t() for translation. Please check all your strings, there are a lot of places where t() must be used.
5. Everywhere you use uc_braintree_tr_payment_log_debug_message() you should wrap the message in t() and use the appropriate placeholders for variables in the message.
6. uc_braintree_tr_payment_get_email_uid(): that will not scale if I have a million users. Why do you need to build an array of all users?

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

You know, I would have reviewed this long ago, but you don't mention Ubercart anywhere in your module description above, so when I search for "Ubercart" projects this one isn't found ... You really need to say "this is a payment gateway for Ubercart" somewhere in your description of what the module does.

I'll do a complete review after you address the issues in #4 and set the project status to "needs review".

Partial review here...

You have license statements in your .install, .info, and .admin.inc files - those statements should be removed.

In your hook_install(), you set a weight for your module. This should be avoided unless you really need your module to be loaded before or after some other module, and in that case instead of just picking an arbitrary weight you should first SELECT the other module's weight out of the DB and add/subtract an offset to make sure your new weight will be less/greater than that other module's.

In your hook_uninstall(), you're deleting row(s) from the {system} table for your module. You should not do that. Also, you're not deleting all the system variables you created in your module, e.g. variable_del('uc_braintree_tr_payment_transaction_debugging').

You're not using hook_schema() correctly. You should define ONE function, called <yourmodulename>_schema(), which declares ALL your tables. You're currently defining four functions, none of which conform to the function-naming pattern required for the hook.

Is this a payment gateway or a payment method? You implement both hook_payment_gateway() and hook_payment_method(), and the hook_payment_gateway() implementation appears wrong because the 'callback' does not name a function used to process a payment.

You're doing the payment settings form wrong. You shouldn't be declaring it explicitly in hook_menu(), the settings form should be declared in the 'settings' case of your payment method 'callback'. See the core Ubercart payment modules for how this should be done.

Please re-examine your use of global variables. You are sometimes declaring them but not using them. Global variables are rarely needed, and should be treated with extreme caution because of the potential for inadvertently causing problems when you change these variables in your functions, especially when dealing with $user.

I didn't really review the .module file yet - too many other things need to be fixed before I can tackle that.

Added "This is a payment gateway for Ubercart" per the suggestion by TR. Thanks!

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

• Commit messages like 'a' and 'b' are not helpful, you should always provide a short description of what the commit does.
• Your INSTALL.txt file has some non-utf8 characters in the 5) section
• Also, the installation instructions are not correct. You should never add code to another module, it'll make upgrading very difficult. So uc_braintree_tr_payment should go in sites/all/modules. And the Braintree php library should use the Libraries API to let people install it.
• In 3) you should note that the key must be readable by the apache user. It's not always 'apache' though!
• You may want to create a drush.make file, it's sometimes the easiest way to let folks download 3rd party code.
• Why are all the variable_del() commented out in uc_braintree_tr_payment_uninstall()?
• A small point, but in your schema you have 'last_four' => array('type' => 'text'.... If it's always 4 digits a small int might be easier.
• require_once 'Braintree.php'; this is where the Libraries API is very useful - you don't have to worry about where it gets installed, making your module easier to use in all sorts of environments.
• Since you require Braintree.php to be available, you should document this in a hook_requirements() implementation.
• In uc_braintree_tr_payment_create_card_form(), Instead of $base_url = base_path() . "index.php?q=user/" . $user->uid . "/payment-method&paymentAction=addCard"; Drupal provides lots of url management functions. url() shoudl work here. Similarly with the $redirect_url.
• Since all the data is passed in the hidden form field tr_data, could someone modify this data to register a different credit card or otherwise modify user data? I'm not sure what's happening on the Braintree end, but that looks pretty insecure.
• In uc_braintree_tr_payment_form_alter() put the switch first so you're sure it's a form you're interested in. That function gets called for every form in Drupal.
• I'm not sure why you're changing the ubercart checkout and back buttons - that seems more like a client request than something I'd expect from a payment gateway. Could this code be moved to a different module?
• Drupal code standards always use && instead of 'and'.
• I'm honestly not sure what's going on in the uc_cart_checkout_review_form form alter, can you explain this a little more? That seems like a lot of work, does ubercart provide any more convenient methods of accomplishing this? More use of hidden fields to store sensitive data and manual url creation.
• Try and get rid of $_uc_braintree_tr_payment_connection_exception, we prefer not to add new globals.
• I think TR's point "You're doing the payment settings form wrong. You shouldn't be declaring it explicitly in hook_menu(), the settings form should be declared in the 'settings' case of your payment method 'callback'. See the core Ubercart payment modules for how this should be done." still applies.
• You're setting $form['#action'] quite a bit. That's ok when you need to submit to an external server, but within Drupal that's already handled for you. Just implement a form_name_submit() function and all your data will be passed there. This is also true for _validate().
• INSERT ... ON DUPLICATE KEY UPDATE is mysql-specific. You can use drupal_write_record() to accomplish the same thing, or document that your module requires mysql.
• Similarly with UNIX_TIMESTAMP(now()), just call PHP's time() and use that value instead.
• Try to avoid using drupal_goto() if possible.
• I'm not sure how useful counting your $_uc_braintree_tr_payment_connection_exceptions is. Doesn't logging already tell you this? Removing that global would be nice.
• Delete all your access functions :) The right way to do this in Drupal is to create permissions (using hook_perm()). Then you can just use user_access('some permission') instead. Or if it's in a hook_menu, just set the access arguments to array('some permission'). user_access takes care of loading the current user, and granting an exception for uid 1.
• I'm not sure why uc_braintree_tr_payment_get_email_uid() is needed? As klausi pointed out, it's quite possible to exhaust PHP memory with that. How about instead rewriting your loop so that it loads a user, does an operation, and then moves on the next user - without loading them all at once. If it's a long operation you can use the Batch API to execute it in chunks for you.
• In uc_braintree_tr_payment_payment_settings_form(), if all those values are just going to submit to a variable, instead you can use system_settings_form() that will take care of that for you. You could also validate quite a bit more (making sure the private key file exists, is readable, has valid data, most of those fields are required and should be noted with 'required' => TRUE.

That's all I can make it through right now, this was not a complete review.

The project application queue is really about the coder and whether or not they have a good grasp of Drupal's API and good practices. You might find it much easier to reduce this module down to it's essential components only, and then once approved, you're free to add in the rest of the features without our interference. Setting to needs work for the number of issues.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Hmm, not sure why this was back in needs review? Unless I missed some updates already?

----
Top Shelf Modules - Crafted, Curated, Contributed.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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