search

ignited / omnipay-zippay

MIT License
1 stars 2 forks source link

Added an optional shipping address into the data for a RestAuthorizeRequest #7

Closed simonworkhouse closed 3 years ago

simonworkhouse commented 3 years ago

This pull request adds support for handling a shipping address in a RestAuthorizeRequest, but I am not certain if the way that it determines if a shipping address has been supplied is entirely suitable.

What would be the preferred way to determine whether or not to include the shipping address? Currently I have just implemented the following:

    public function hasShippingAddress()
    {
        return !empty($this->getShippingAddressLine1());
    }
alexw23 commented 3 years ago

Is Shipping Address Line 1 Required? If so I don't see any issue with this approach. If it's not then we may need to implement a flag of some kind.

simonworkhouse commented 3 years ago

According to the documentation at https://zip-online.api-docs.io/v1/api-specification/create-a-checkout all fields in the shipping address are required. Would you prefer the check be updated to ensure that a value has been supplied for all fields? Or just continue to check for ShippingAddressLine1 and allow Zip to respond with an error if fields do not contain the expected values?

alexw23 commented 3 years ago

Maybe just check how other libraries such as PayPal, Stripe handle this. But I'm open for the Line1 solution, just might seem messy if someone does use Line2.

simonworkhouse commented 3 years ago

@alexw23 I have changed this so that is uses a parameter instead of checking for the first line of the shipping address. That would appear to be the "standard" way that other Omnipay packages would tend to handle this.

alexw23 commented 3 years ago

Just to confirm getHasShippingAddress() is returning false/null by default correct?

simonworkhouse commented 3 years ago

Yes, it is returning null by default.