terdelyi / omnipay-wirecard

Wirecard gateway for Omnipay payment processing library
MIT License
1 stars 3 forks source link

Omnipay compatibility #2

Closed dercoder closed 7 years ago

dercoder commented 7 years ago

You have created setters and getters for the URLs which already exists in omnipay. For example you use confirmUrl

    public function getConfirmUrl()
    {
        return $this->getParameter('confirmUrl');
    }
    public function setConfirmUrl($value)
    {
        return $this->setParameter('confirmUrl', $value);
    }

but basically you should use

    /**
     * Get the request notify URL.
     *
     * @return string
     */
    public function getNotifyUrl()
    {
        return $this->getParameter('notifyUrl');
    }

    /**
     * Sets the request notify URL.
     *
     * @param string $value
     * @return AbstractRequest Provides a fluent interface
     */
    public function setNotifyUrl($value)
    {
        return $this->setParameter('notifyUrl', $value);
    }

from Omnipay AbstractRequest.php

This is only one example. There are more issues like this in your code.

terdelyi commented 7 years ago

Hi @dercoder, thank you for your response.

Yes, you're right, it's a mistake, but not because it's not compatible with Omnipay. Wirecard Checkout doesn't use the confirmUrl parameter, but Wirecard Checkout Seamless does. However my gateway only supports simple Checkout payments therefore I'm going to remove those two methods.

On the other hand I think I understand what you wanted to say there and I thought a lot about this issue when I wrote this package. There are some parameters what you can't reconcile with Omnipay variables and methods. Because of this and to avoid confusing I stayed with Wirecard naming conventions. I also looked over several other existing Omnipay gateways and they mostly do the same with custom methods.

In this particular case I tend to agree with you, so if this library may use notifications it should use notifyUrl method instead of confirmUrl. But I also believe that it wouldn't be pretty obvious to other developers for the first sight who have a parameter list from Wirecard.

This is only one example. There are more issues like this in your code.

I'm open for any merge requests, but without examples this is only an opinion, not a helpful review.

dercoder commented 7 years ago

Hi,

Wirecard Checkout use the confirmUrl parameter: https://guides.wirecard.at/request_parameters#confirmurl

I understand why you did this. But i think that Omnipay should unify the payment providers so every developer can use the same parameters for all payments without reading the payment docs. Thats why i have create my own package: https://github.com/dercoder/omnipay-wirecard

What do you think about it?

terdelyi commented 7 years ago

As I see it's basically my package with some improvements and complete test coverage. Instead of forking my work can I ask why didn't you pull those changes in a merge request?

dercoder commented 7 years ago

Because i am not finished yet. Currently i have problems with "completePurchase" function. Wirecard does not send any transactionId which i can use to link the payment notification to any transaction.

For example:

array (
  'amount' => '1.00',
  'currency' => 'USD',
  'paymentType' => 'CCARD',
  'financialInstitution' => 'MC',
  'language' => 'de',
  'orderNumber' => '5423206',
  'paymentState' => 'SUCCESS',
  'instrumentCountry' => '',
  'authenticated' => 'No',
  'anonymousPan' => '0012',
  'expiry' => '01/2019',
  'cardholder' => 'Test',
  'maskedPan' => '550000******0012',
  'gatewayReferenceNumber' => 'C957926148348456728537',
  'gatewayContractNumber' => '00000031629CAFD5',
  'responseFingerprintOrder' => 'amount,currency,paymentType,financialInstitution,language,orderNumber,paymentState,instrumentCountry,authenticated,anonymousPan,expiry,cardholder,maskedPan,gatewayReferenceNumber,gatewayContractNumber,secret,responseFingerprintOrder',
  'responseFingerprint' => '186272b148462972472f49c2dc2e0e81',
)

My internal transactionId is 10807 I have also tried to use orderNumber instead of orderReference but this does not work. Do you have any idea? How does it work on your side?

terdelyi commented 7 years ago

Use a query string for that in the passed urls. For example: http://your-website.com/response?type=success&orderId=1234

dercoder commented 7 years ago

Yeah thats maybe one possibility but i think this is not the best one. I will contact the wirecard support tomorrow to find a solution. But thx for this idea ;)

terdelyi commented 7 years ago

Before Omnipay I also used that, I don't know about any other solution, but I hope you'll get lucky :) Please keep me updated in email.