Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This is an Ubercart payment gateway module for the Stripe payment processor.
It is unique because there haven't yet been any modules for Ubercart which use Stripe as the payment processor. It also has support for the uc_recurring module so people can create recurring subscriptions using Stripe's subscription capabilities.
The sandbox page is: http://drupal.org/sandbox/victorquinn/1339840
And the git repo: git clone http://git.drupal.org/sandbox/victorquinn/1339840.git uc_stripe
It is currently for Drupal 6, but a Drupal 7 will come shortly.
Comment | File | Size | Author |
---|---|---|---|
#22 | drupalcs-result.txt | 590 bytes | klausi |
#17 | 1339850-watchdog.patch | 937 bytes | TR |
Comments
Comment #1
doitDave CreditAttribution: doitDave commentedHi,
Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)
Review of the 6.x-1.x-dev branch:
This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
Manual additions:
OK, more in a second round.
Hth, dave
Comment #2
mail@victorquinn.com CreditAttribution: mail@victorquinn.com commentedGreetings doitDave, I believe I have made all of the above changes. Let me know if anything further is required!
Comment #3
doitDave CreditAttribution: doitDave commentedHi again,
Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)
Review of the 6.x-1.x-dev branch:
This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
Don't forget to re-set the status to "needs review" as soon as you think it's time :)
Comment #4
mail@victorquinn.com CreditAttribution: mail@victorquinn.com commentedOk, I have removed the 6.x-1.x-dev branch, replaced it with a 6.x-1.x branch, and pushed the changes appropriately this time.
The only thing I'm unsure of is the "All text files should end in a single newline (\n)." thing. I opened each file one by one and each ends in a newline already. So after the final curly brace, there is one new line, then the end of the file. I'm not sure what I'm doing wrong here. I use emacs and it's set to Unix mode, so it's not a problem of having a Windows newline or something, and Googling it for and searching Drupal.org got me nowhere. So I'm not sure how to fix a problem I cannot see.
If I can have some guidance on this newline thing, that would be great. Otherwise, things should be good to go.
Comment #5
doitDave CreditAttribution: doitDave commentedHi Victor!
Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)
Review of the 6.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
Regarding the newlines, check my comment here: http://drupal.org/coding-standards#comment-5262644 as it has cost me lots of sleep myself... ;( Sticking to that, you will be on the safe side.
Comment #6
TR CreditAttribution: TR commented@victorquinn: All the files in your sandbox repository currently have the correct newline file endings - there is nothing you need to change in regards to newlines. But the other coding style issues pointed out above should be fixed.
Re: #5: http://drupal.org/coding-standards#comment-5262644 is wrong, and I have replied in that thread.
Comment #7
mail@victorquinn.com CreditAttribution: mail@victorquinn.com commentedOk, took another pass, I believe I fixed everything now.
Comment #8
raynimmo CreditAttribution: raynimmo commentedAutomated review
Please keep in mind that this is primarily a high level check that does not replace but eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.
Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
Manual Review
I realise it is not a strict prerequisite for many reviewers but you should maybe make an attempt to adhere to the CSS coding standards. Also, you include the browser specific styles -webkit and -moz but have omitted to include any for other browsers. Possibly have a read of http://reference.sitepoint.com/css/vendorspecific to make your module a bit more cross browser compatible. These are just personal points of mine though and should not hold up your project application if they are ignored.
Please ensure to change your project status back to 'needs review' once you have addressed the issues highlighted above.
Good luck with the rest of your review.
Comment #9
raynimmo CreditAttribution: raynimmo commentedComment #10
mail@victorquinn.com CreditAttribution: mail@victorquinn.com commentedOk, made all of the changes to the comments as requested by the code sniffer.
Somewhat curious as to why all of these comment changes didn't show up on earlier passes through the code sniffer or I would have fixed them then and not wasted someone's time with another pass, but oh well.
Also made the suggested changes to the CSS to make it more cross-browser compatible -- thanks raynimmo, I had simply overlooked that!
Comment #11
raynimmo CreditAttribution: raynimmo commentedThe Code Sniffer and Coder modules change regularly, always make sure you are using the most up to date versions as these modules are constantly being updated with new/better rule checks. Always check out the development releases as they are usually newer than the 'stable' versions.
When I run Code Sniffer I usually pass it all file types, such as as php,module,install,js,css,inc,txt , it is possible that previous checks did not run against all of these file types and hence checks where not performed on that group of files.
Comment #12
raynimmo CreditAttribution: raynimmo commentedJust ran your code through the online PAReview service that patrickd put together.
I am glad to say it didnt flag any errors. I will try and find time to do a manual review in the next few days.
Well done.
Comment #13
mcfilms CreditAttribution: mcfilms commentedPerfect timing, since I was investigating using Stripe. I may be able to test this in the next couple of weeks.
Comment #14
jthomasbailey CreditAttribution: jthomasbailey commentedThat git checkout just gives me a readme that says "See major version branches", where can I find the module to test it?
Comment #15
TR CreditAttribution: TR commented@hobgobbler: That's the way it should be. git clone will leave you on the master branch, which is unused by Drupal. Do a git branch -a to see the available branches for this project. One of them is 6.x-1.x. Then you can git checkout 6.x-1.x .
Read the contents of the "Version control" tab on the sandbox project page for details.
Comment #16
jthomasbailey CreditAttribution: jthomasbailey commentedThanks got it, that git is all voodoo to me :) It appears you need an SSL certificate to use it though, because of the dependency on the Credit Card module, is that right?
Comment #17
TR CreditAttribution: TR commentedI signed up for a test account with Stripe and tried out your module. Just a couple of issues:
Two of your watchdog calls have the wrong arguments. Attached is a patch that fixes this.
My preference would be to add a hook_requirements() to check for the presence of the Stripe library when the user tries to install uc_stripe. What you do now is defer this check until you actually try to process a payment via Stripe - if the payment fails because the library isn't there the only way you learn what's wrong is by looking in the dblog (and only after the above watchdog calls are fixed :-)) You can use uc_cybersource as an example of how to check for a library from hook_requirements().
Stripe.php also requires cURL - this is not something that is built into PHP by default, and some sites might not have it. You should also check for this in hook_requirements().
In the Stripe gateway configuration, the textfield for entering the secret key should absolutely be changed to a password field. Otherwise, anyone browsing your admin interface can read off your secret key.
The Stripe.php code you're using has a comment that says it's been tested with PHP 5.2 and 5.3. Because it's object oriented code, it's highly likely that it will have problems in lower versions of PHP. Your uc_stripe.info file specifies only php=5.0. I strongly suggest you change that to php=5.2.
I would make the configuration default to using Test mode. The admin should have to consciously switch on Production transactions.
Other than that, the module works and looks like it's ready to go! I'd like to see the password field issue addressed before approving the project.
Comment #18
mail@victorquinn.com CreditAttribution: mail@victorquinn.com commented@TR Thanks for the comments and the patch!
I've applied your patch, good catch there. I also changed the .info file to require PHP 5.2 and am now checking for cURL in the hook_requirements.
I also made the secret keys password fields. I actually intentionally left those as plain textfields so the admins could view them and because they are in plaintext in the Stripe console, but I didn't really consider a situation under which the person viewing the Stripe account wasn't the site admin for the Drupal site. So I have changed those to password fields as per your suggestion.
I'm still figuring out how to reliably perform the check for the Stripe library in hook_requirements() because it is expected to be in sites/all/libraries and the libraries API functions are not available in hook_requirements()** and drupal_get_path() won't really work because it's not a module or a theme. Anyway, wanted to push the rest of the changes, will update when this is done.
** at least I don't think they are, maybe they are, needs testing
Comment #19
mail@victorquinn.com CreditAttribution: mail@victorquinn.com commented@hobgobbler: Yes, basically, you will need an SSL cert. I really wouldn't recommend doing anything with payment processing without it. Although we aren't storing any credit card data locally due to the Stripe magic, it's still a good idea and many customers will expect it.
You can get a free one here: http://www.startssl.com/?app=1
Comment #20
mail@victorquinn.com CreditAttribution: mail@victorquinn.com commented@TR: Forgot to change the test mode default to TRUE. Now changed and committed.
Comment #21
TR CreditAttribution: TR commentedI think this is ready to be approved.
Comment #22
klausiReview of the 6.x-1.x branch:
This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.
manual review:
But I think that are just minor issues, so ...
Thanks for your contribution, victorquinn! Welcome to the community of project contributors on drupal.org.
I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.
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.
As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.