Omnipay\SagePay\Message\ServerNotifyRequest::sendData() returns the wrong object

[andris-sevcenko]

trafficstars

Omnipay\SagePay\Message\ServerNotifyRequest::sendData() returns an instance of itself, however the docbloc of Omnipay\Common\Message\AbstractRequest::sendData() promises to return an instance of Omnipay\Common\Message\ResponseInterface.

Obviously, Omnipay\SagePay\Message\ServerNotifyRequest is not an instance of the said interface.

[judgej]

Maybe it shouldn't extend AbstractRequest to fix that confusion? It is only there to provide the ability to set parameters (security key to check the signature, which is a bit like the signature many gateways use, but there is an individual signature key for every transaction).

[andris-sevcenko]

It would seem odd that there would be a request that did not extend AbstractRequest. Also, it would seem logical that a request has a response, so a better fix would probably be introducing a ServerNotifyResponse class :)

[judgej]

You would think that, wouldn't you :-)

However, the notification request is a Server request, it's an incoming request, a notification or a webhook from the gateway, the opposite way around to all of the other requests.

The merchant site application does need to return a response to the gateway, and this is where Omnipay is silent on how that is done, which is a problem IMO. Ideally, the notification would return a PSR-7 Response object that the application can use to return to the gateway. What the gateways expect are very varied, so a PSR-7 message would be a good standard way for the driver to construct that response. It is still up to the merchant site to decide how it will do that, and gets its own cleanup in order.

For example, some gateways require a simple 200 or 500. Some need data to be returned in various formats, with specific content types. Some, such as Sage Pay, need the return payload to be constructed depending on whether the merchant site accepts the original payload or now, and it main contain a message and will contain a redirect URL. Others need signatures in headers, most don't. Omnipay just does not have a way for the notification server request message to provide that at this time. The Authorize.Net DPM actually needs a complete web page to be returned, that the gateway will then display to the user in its own domain with a manual or automatic redirect in it.

It would make sense for the send() method to return this constructed response, I guess. But it has to be documented well that this response is NOT the transaction notification from the gateway, but a HTTP message the merchant site needs to send to the gateway.

Does that make sense? I wonder if we just try it in this gateway with a custom method to see how it goes, then propose it as a change to the core Omnipay?

[andris-sevcenko]

Well now that you put it that way. :)

I suppose a clean-ish way would be to have an abstract class for webhooks that would combine concepts of both the request and response classes.

It wouldn't even need to be very complicated. On an interface level, it could just have getBody() (raw POST data), getData() (key/value pairs) and getHeaders() methods for the incoming webhook and analogue methods for response as well as a respond() method.

As far as things go, though, I can manage with the current implementation, even though it's breaking the promise made by the AbstractRequest class, so I suppose this can be closed.

[judgej]

Referencing an earlier discussion on this:

https://github.com/thephpleague/omnipay-common/issues/149

I would like to keep it open until there is a common way forward.

[thepurpleblob]

I'm seeing this error..... is it the problem described above?

Argument 1 passed to Aimeos\MShop\Service\Provider\Payment\OmniPay::saveRepayData() must implement interface Omnipay\Common\Message\ResponseInterface,

instance of Omnipay\SagePay\Message\ServerNotifyRequest given,

called in /home/shop/public_html/ext/ai-payments/lib/custom/src/MShop/Service/Provider/Payment/OmniPay.php on line 527