Problem/Motivation

Afterpay SDK does not implement all Afterpay API endpoints. When an API consumer would like to extend the SDK functionality by adding new Request types, it is impossible to send those requests directly using Client::sendRequest() because it is not publicly accessible. I propose making the method public to allow greater versatility in usage by API consumers.

In addition, it is not possible to override the Drupal\commerce_afterpay\Client\Client, because Drupal\commerce_afterpay\Client\ClientFactory::createClient() has hard-coded a return type of Drupal\commerce_afterpay\Client\Client, rather than using an interface.

Proposed resolution

- Make the method Drupal\commerce_afterpay\Client\Client::sendRequest() public to allow greater versatility in usage by API consumers.
- Create Drupal\commerce_afterpay\Client\ClientInterface, and make the return typehint for Drupal\commerce_afterpay\Client\ClientFactory::createClient() be that interface rather than directly using the Client class.

CommentFileSizeAuthor
#3 diff.patch4.33 KBJoeBashe
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JoeBashe created an issue. See original summary.

JoeBashe’s picture

Issue summary: View changes
JoeBashe’s picture

FileSize
4.33 KB
AndyF’s picture

Thanks @JoeBashe!

When an API consumer would like to extend the SDK functionality by adding new Request types, it is impossible to send those requests directly using Client::sendRequest() because it is not publicly accessible.

Just to make sure I understand the situation correctly, you have the official SDK, you've discovered some unimplemented API call, and you want to temporarily implement it yourself while waiting for the official SDK to catch up, and use that new request with this module? (Rather than you want to extend an existing request that is already supported? Or somit else entirely?)

The protected visibility was kind of on purpose: the client directly provides methods for calls it knows about and translates from Drupal/Commerce to Afterpay SDK. So as much as possible inputs and outputs are Drupally (eg. a Commerce price, or payment). sendRequest() on the other hand accepts and returns Afterpay-ey things.
This feels like exposing its internals a little. And there are now multiple ways to make the same call using the same client (eg. using the helper method, or manually setting things up and calling sendRequest()).

As an alternative, would it be palatable to either:

  • Extend the client with your own specialization that can directly call sendRequest() and has public methods for the newly implemented API calls that do the translation from Drupal to Afterpay?
  • Factor out the core request sending functionality from the Drupal/Afterpay translation layer, at which point the function to send a request will be public.

In addition, it is not possible to override the Drupal\commerce_afterpay\Client\Client, because Drupal\commerce_afterpay\Client\ClientFactory::createClient() has hard-coded a return type of Drupal\commerce_afterpay\Client\Client, rather than using an interface.

Thanks, this is great whatever we decide about the above. I think there's a whole load of interfaces waiting to be created... (:

Btw you can trigger the CI by changing the issue status to Needs review.

Thanks again!

AndyF’s picture

Btw, if you are extending the module to support API calls we expect to land upstream, feel free to host on the project repo on a different branch if you'd like.

  • AndyF committed 0f1cb58 on 1.x
    Issue #3206305 by JoeBashe, AndyF: Add interface for client
    
JoeBashe’s picture

Just to make sure I understand the situation correctly, you have the official SDK, you've discovered some unimplemented API call, and you want to temporarily implement it yourself while waiting for the official SDK to catch up, and use that new request with this module? (Rather than you want to extend an existing request that is already supported? Or somit else entirely?)

That's exactly correct! In particular, we're implementing the "Get Payment By Order ID" and "Reverse Payment By Token" API endpoints.

The protected visibility was kind of on purpose: the client directly provides methods for calls it knows about and translates from Drupal/Commerce to Afterpay SDK. So as much as possible inputs and outputs are Drupally (eg. a Commerce price, or payment). sendRequest() on the other hand accepts and returns Afterpay-ey things. This feels like exposing its internals a little.

This makes sense. As long as we can override the `Client` we can extend it with our custom API methods, which should be straightforward enough.

AndyF’s picture

Category: Feature request » Support request
Status: Active » Fixed

@JoeBashe I'm going to mark this as fixed - please feel free to reopen if something gets in your way. Thanks!

JoeBashe’s picture

Thanks for your help @AndyF!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.