-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added extra parameter arguments #5
Conversation
Now you can also send the sender_address
idea to drop the servicePointId parameter and replace it by arguments? |
* @return ShippingMethod[] | ||
* @throws SendCloudClientException | ||
*/ | ||
public function getShippingMethods(?int $servicePointId = null): array | ||
public function getShippingMethods(?int $servicePointId = null, $arguments = []): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove servicePointId and replace with $arguments?
@bockhauzen Hey! Sorry for not getting back on this sooner. I forgot to watch this repository and only just now discovered about this pull request =S If this is still desired, I'm more inclined to add the arguments as additional parameters so that user-error is less likely to occur. That's how the rest of the client is set up. It would then look something like: public function getShippingMethods(?int $servicePointId = null, ?int $senderAddressId, ?bool $returnMethodsOnly = null): array Would that work for you? I'd still have to look into what the "all" option for "sender_address" exactly does. If it's the same as just plain leaving the option out then it's not necessary to implement in the client either. |
Hi, Woops, late reply as well. I agree. That would def. be a cleaner solution. There also seem to be an issue with: When trying to use DE as country code it seems to fail. Im trying to debug it, probaly my brains are just failing. Thanks anyway for your response! |
Update: There seems to be a difference between passing "all" and leaving the parameter out entirely. Not passing the parameter will use the default sender address. On my test account not passing the parameter yields 336 shipping methods and passing "all" yields 984. The latter takes over a solid half minute to complete. I'll probably have to increase the client's timeout over this. |
@bockhauzen This is now available in v3.8.0! Note that I've moved your other issue to a separate thread (#10) that still requires reproduction steps. |
Now you can also send the sender_address