Webador / sendcloud

Provides a PHP client to interact with the SendCloud API in an object-oriented way.
MIT License
16 stars 15 forks source link

Improve client methods taking a lot of optional parameters #38

Closed villermen closed 1 month ago

villermen commented 1 month ago

The parameters of the createParcel() method grew naturally, but I think it would be better to model them (and parameters of other methods) using a value object instead. That way, mandatory parameters are enforced and the dedicated model can be documented and extended better.

https://github.com/Webador/sendcloud/blob/779620556cb5137ac5085ccc26f00671b9bc0872/src/Client.php#L156-L168

villermen commented 1 month ago

I never thought I'd say this, but I think I want it similar to PayPal's setup in their (deprecated) checkout PHP SDK where you create a *Request and execute it to obtain a *Response. The benefit of this approach is that it can use more advanced setters on the request that validate the input and do things like forcing paired properties to be set together (total_order_value and total_order_value_currency). Typing the response matching the request will need separate methods for each endpoint, unlike PayPal's single execute() method:

$request = new CreateParcelRequest($mandatoryArgument);
$request->setRequestLabel(true);

$client->createParcel($request); // => CreateParcelResponse

Composing similar requests and responses in a smart way using traits is probably desirable.

villermen commented 1 month ago

After an internal discussion, it actually sounds better to more away from value objects for these API methods entirely. Mainly because of this point:

[..] when using a wrapper for an external API then I mostly want the parameters to mapped 1:1 to whatever is written in the API documentation, because then I don't have to think how the SDK maps to the API. :slightly_smiling_face:

Introducing a new major version for splitting a single argument up is not worth it, so I'll wontfix this issue but keep this in mind for future additions.