So what do we do now??

CommentFileSizeAuthor
#5 uc_migs_merchant.zip9.96 KBemTque
uc_migs_merchant.zip9.83 KBemTque

Comments

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

Hey Matt

Thanks for submitting this. Somehow I missed it until now, for which I apologise.

Will review shortly - much appreciate your work.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

Hey Matt

Looks fairly good. A couple of things will need to be done as we go, adding some notes here to kick off. Are you still keen to collab on getting this in?

  • Need to handle the case where curl is not available. We should probably just use drupal_http_request() in its place if possible (see below).
  • The two functions null2unknown() and getResponseDescription() need to be namespaced in order to ensure there's no clash with other modules implementing similarly named functions.
  • There's a $_POST[] reference which seems unused. It should go. (It's not an issue in this instance, but we've just released a security fix on uc_migs for similar code.)
  • If $response is empty after the cURL request, we'll end up at // ERROR! need to finish ERROR code function, and then record an order comment with an empty $msg.
  • There's an instruction to contact Salva if the payment declines. We should include his cellphone number, or maybe make the error message more generic :D

I see you had some drupal_http_request() code in there but went with cURL - can you tell me more about this decision? Looks like a lot of Ubercart payment modules use cURL instead of drupal_http_request() - I wonder if there's a specific issue with drupal_http_request() or whether it's just historic?

emTque’s picture

emTque commented

Hey grobot,

sure i can help out (not sure how much help i will be), just let me know what you need.

  • I used curl because it was mentioned in the example code provided by the bank my client was using.
  • sure thing, i will get on to this.
  • not a problem the whole thing could do with a little clean up i reckon. also does this mean that my code is not vunerable to the security issue mentioned here http://www.ubercart.org/project/MIGS%20Payment%20Gateway%20#comment-53689?
  • Again a little bit of loose-end-tieing that needs doing. i will get on it
  • I think making this more generic is better. Might add an admin field for client/site owner details

my code is based on the example code my clients bank provided (i basically just 'drupalfied' their Merchant hosted code), so that is why i used curl. as for why it used in Ubercart instead of drupal_http_request() i am not sure.

i will make those changes and upload a new version of the module as soon as i can.

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

* curl is fine, whatever works. just wondering out loud :)
* cool!
* your code is fine for SA-CONTRIB-2010-064, but coder module will prob say "what's that doing there". the $_POST reference isn't used anyway so it can go.
* admin field - no need to add that i think as ppl can just use the details alrady in UC and/or t() (string overrides etc)?

thx

emTque’s picture

emTque commented
StatusFileSize
new uc_migs_merchant.zip9.96 KB

Hey grobot,

here is the updated module code. let me know what you think.

cheers