Skip to content

HTTPClient injection through constructor is wrongfully reused #29

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

Closed
SpacePossum opened this issue Sep 22, 2016 · 4 comments
Closed

HTTPClient injection through constructor is wrongfully reused #29

SpacePossum opened this issue Sep 22, 2016 · 4 comments
Labels

Comments

@SpacePossum
Copy link
Contributor

When construction the client you can only pass one HTTPClient; @ see
https://github.com/messagebird/php-rest-api/blob/master/src/MessageBird/Client.php#L95
The client is used for both chat and messaging while the services have diff. end-points.

I would like to suggest to add an additional parameter to the constructor allowing two HTTPClient's to be passed, i.e. something like this;

public function __construct($accessKey = null, Common\HttpClient $httpClient = null, Common\HttpClient $httpChatClient = null)

Since the class is not declared final the change would be a BC break, so I would like to suggest doing the change on a new 2.x line and release the change on a 2.0 tag.

@sotoz
Copy link
Contributor

sotoz commented Sep 22, 2016

Hello SpacePossum,
Thank you for your help and your helpful feedback. We will check this matter and reply to you as soon as possible. You are right about this being a breaking change so we would like to pay extra attention at this.

@samwierema
Copy link
Contributor

@SpacePossum this would indeed be a breaking change, and would require at least a major version upgrade. At this stage we're not yet considering a major version upgrade.

An alternative could be to create two child classes that inherit from Client, e.g. ChatClient. That would not be a backwards incompatible change, and would allow specific configuration for the REST API and Chat API respectively.

For our reference, do you currently have a specific requirement that would allow for two separate HttpClient instances?

@SpacePossum
Copy link
Contributor Author

Hi,

Thanks for the feedback and thoughts. Currently I have no real need for the two separate clients.

I came to this issue when working on #28.
There I have a use case for injecting the client such that I can prepare/configure it myself and set a different timeout than the default. So for now I'm good :)

@SpacePossum
Copy link
Contributor Author

@samwierema please ping me when a new major release is being considered, I'm happy to see if I can help out at that time. Thanks for reply 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants