Hey,
Wow. Talking about feedback. Those are some great pointers you have right
there. I will tackle them starting tomorrow to make it as much as developer
friendly as possible. Now to your pointers
About the first question you had with the notification upon payment or
subscription. From what i have read about it on paypal's developer central
and across the web is that you specify a return url or a notify_url that
paypal 'pings' or silently callbacks and that link can just be an action in
a controller that uses one of the methods to process the payment. I am not
sure if this is the exact case but in my testing (using sandbox) my
subscriptions were processed fine. So more testing on this or ideas will be
required i assume. To receive notifications based on the way your processing
the request the _parseIPNResponse will be used for 3 methods that actually
need it and they are documented in the class. I am not that familiar with
Paypal so if anyone willing to give this a go and test/edit the code he is
more then welcome.
1) Yea i agree. private will be changed to protected.
2) Yet again another CS thing, i will go over the code and try and make it
more CS complint.
3) Great idea. I will do just that. and include the setter method as well.
4) yea i could probably do that.
5) What if i want to get the returned array and use it's contents for
storing? I see the point of having isVerfiedAddress but wouldn't it be a
better idea to have both then just returning a single boolean value?
6) I was thinking about that while i was wrtting this. But then i said for
consistency i should stick with one class for each gateway. It's not a
problem to split those up to there appropriate classes. That's why i wanted
this feedback. The question is if this is something that more developers
would like to see? And in what way to split them up?
7) They maybe not named the way you said it but by default they return the
link not redirect. If you pass a boolean value of true to the $autoRedirect
then it will redirect. Both options are available.
8) Yea, i could probably use that for all methods.
9) Ahh i tried really hard on this one (phpdoc) and i provided the right
phpdoc style for the file, class, methods and property names. What did i
miss? Could you be more specific?
10) Yes, I was thinking if throwing exceptions will be a better choice then
just storing the error message returned in a class variables and then
retriving them later on. At some point i even included both. Do you think
throwing exceptions will be best handled? If so i could probably throw them
then just storing the message in a varible.
11) Actually that's already exists. and used in the Tranzila gateway. If you
take a look at the Zend_Payment_Gateway_Abstract you will notice them
/**
* Payment result codes
*/
const APPROVED = 100; // payment approved
const DECLINED = 101; // payment declined
const OTHER = 102; // other
const FRAUD = 103; // fraud detected
const DUPLICATE = 104; // duplicate
const REVIEW = 105; // marked for reivew
const CCNUMBER_INVALID = 106;
const CVV_INVALID = 107;
const CCNUMBER_MISSING = 108;
const CCNUMBER_EXPIRED = 109;
const TRANSACTION_TYPE_INVALID = 110;
const TRANSACTION_CODE_INVALID = 111;
const CREDIT_TYPE_NOT_SUPPORTED = 112;
const CURRENCY_NOT_SUPPORTED = 113;
const GATEWAY_INACCESSIBLE = 114;
const FILL_ALL_FIELDS = 115;
Thanks again for the feedback. And if you or anyone else would like to help
out, It will be apprecaited.
Vince.
On Tue, Apr 28, 2009 at 5:03 PM, till <klimp...@gmail.com> wrote:
> Hey,
>
> so here are some thoughts. I'm commenting mostly on paypal, since I'm
> familiar with them. I think most of this applies to your other
> drivers/gateways as well. From what I remember with working with
> Paypal (we do a lot of subscriptions), it's a pain to work with the
> API and it's far from obvious. I think the objective should be to make
> it usable to the developer.
>
> First off, one question. We are doing subscriptions, and the way it's
> setup is, that Paypal notifies us. This notification is independent of
> the general request and the customer. They aim to do it in 'realtime',
> but it can sometimes take up to five minutes for Paypal to notify us
> that the 'order' has been processed while the customer has already
> seen a 'Thank you for your money' on Paypal's website. Now, the Q is
> -- how do I receive these notifications with your code?
>
> I couldn't figure it out, because what I have to do is wait for a
> request from paypal and resubmit all parameters for verification. But
> maybe I've overlooked that one. So apologies in advance.
>
> On to my feedback, I hope you don't mind. (Just let me stress again,
> that even though I got some things that I'd like to see
> improved/changed, I really appreciate the work done so far very much.)
>
> 1) generally, don't use private, as it makes extending your code
> 'harder'. I couldn't override/extend the paypal gateway and then I
> couldn't access the httpclient. Use protected instead.
>
> 2) Generally public class members (variables and methods) shouldn't be
> prefixed with '_', only protected and private should.
>
> 3) Provide a way for people to override the Zend_Http_Client, e.g.:
> public function __construct(Zend_Http_Client $client = null)
> {
> if ($client !== null) {
> $this->client = $client;
> } else {
> // create instance here
> }
> }
>
> Maybe also add a setter, setHttpClient(Zend_Http_Client $client).
>
> 4) use set and get more appropriately, e.g. creditcardInfo(), should
> be something like, setCreditcardInfo() (or maybe, setCreditcard()?).
>
> 5) Methods such as 'addressVerify' could be more simple/convenient for
> the developer, e.g. isVerifiedAddress($street, $etc) -- it would
> return a boolean. I don't see why 'addressVerify' in its current form
> returns an array. A boolean would be more straight to the point (kind
> of like you did with isSuccess()).
>
> 6) In general/IMHO, there's too much in one class. I understand that
> it's all related to paypal, but authorization, transaction management,
> etc. could be phased out into subclasses.
>
> e.g.:
> Zend_Payment_Gateway_Paypal_Transaction
> (Zend/Payment/Gateway/Paypal/Transaction.php)
>
> $paypal = Zend_Payment::factory('paypal');
> $transaction = $paypal->transaction();
> $transaction->get($id);
> $transaction->doRefund($id, 'full');
> $transaction->search('2009-04-27');
>
> Or:
> $order = $transaction->create(...);
> $order->setCustomer(Zend_Payment_Gateway_Paypal_Customer $customer);
> ...
>
> Zend_Payment_Gateway_Paypal_Customer
> (Zend/Payment/Gateway/Paypal/Customer.php)
>
> $customer = $paypal->customer($id); // an identifier of the customer
> $customer->getBalance();
> $customer->getAddressbook();
> $customer->addAddress();
> $customer->isVerified();
>
> (These are just examples, as of how the interface could be more intuitive.)
>
> 7) header() calls in your library should be avoided at all costs for
> multiple reasons, e.g. it makes it harder (for yourself) in terms of
> testability. And what if I wanted to display the link instead to my
> customer? (E.g. "click here to continue to paypal"). Instead those
> methods should be renamed to, 'getExpressCheckoutLink',
> 'getBillingAgreementLink' (or similar). With the getter, the developer
> can either pipe it to header() himself, or do the redirect using the
> framework code, or display the link in a view so people click on it
> themselves, etc..
>
> 8) Simplify control-flow, e.g. in your current form, massPay() should
> look like this instead:
> public function massPay(array $_receivers=array() )
> {
> if (count($_receivers) == 0) {
> return null;
> }
> $this->addParam('METHOD', 'MassPay');
> foreach($_receivers as $i => $receiver) {
> $this->addParam('L_EMAIL'.$i, $receiver['email']);
> $this->addParam('L_AMT'.$i, urlencode($receiver['amount']));
> $this->addParam('L_UNIQUEID'.$i, urlencode($receiver['uniqueID']));
> $this->addParam('L_NOTE'.$i, urlencode($receiver['note']));
> }
> return $this->_buildRequest();
> }
>
> Save an is_array() by forcing the type to array in the method's
> signature, and (my personal favorite) exit the method early if there's
> nothing to process.
>
> 9) phpdoc and general CS could use some love. :)
>
> 10) Since you're using Zend_Http_Client in your code, you should trap
> its exceptions, and re-cast, e.g.:
>
> (in Zend_Payment_Gateway_Paypal::_buildRequest())
>
> try {
> $this->client->request();
> } catch (Zend_Http_Client_Exception $e) {
> $msg = "Message: {$e->getMessage()}, Code: {$e->getCode()}";
> throw new Zend_Payment_Exception($msg, Zend_Payment::ERR_HTTP);
> }
>
> 11) I'm not sure, if I'm the only one using exception codes, but I'd
> also like to see that across the board. E.g, defining them as class
> constants in Zend_Payment, ERR_HTTP, ERR_INPUT, ERR_GATEWAY (just
> examples). An alternative is using different exception classes,
> Zend_Payment_Http_Exeption, Zend_Payment_Input_Exception -- but IMHO,
> that's overheadish.
>
>
> Just some pointers, hope it helps.
>
> Till
>
--
Vincent Gabriel.
Lead Developer, Senior Support.
Zend Certified Engineer.
Zend Framework Certified Engineer.
-- http://www.vadimg.co.il/
