Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

bockhauzen
Copy link

Now you can also send the sender_address

Now you can also send the sender_address
@bockhauzen
Copy link
Author

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
Copy link
Author

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?

@villermen
Copy link
Member

@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.

@bockhauzen
Copy link
Author

bockhauzen commented Nov 9, 2020

Hi,

Woops, late reply as well.

I agree. That would def. be a cleaner solution.
One other question;

There also seem to be an issue with:
client->createLabel

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!

@villermen
Copy link
Member

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.

@villermen
Copy link
Member

@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.

@villermen villermen closed this Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants