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

Remove dependency form Guzzle #131

Merged
merged 4 commits into from
Jul 27, 2016
Merged

Conversation

Nyholm
Copy link
Contributor

@Nyholm Nyholm commented Jul 21, 2016

Removed dependency from Guzzle. If we use php-http/discovery instead of guzzlehttp/psr7 we can let the user decide what implementation to use.

I also did some code cleanup and make sure someone can add a async client.

@avigoldman
Copy link
Contributor

Hey @Nyholm, thanks for the awesome PR! I just went through your changes and they look good 👍 If you don't mind it would be great if you could write a couple of quick tests to cover the setMessageFactory function and the branch in setHttpClient where the parameter is not a instance of the HttpAsyncClient or HttpClient. If not just let me know and I'll merge in your PR and handle those tests myself at some point.

@Nyholm
Copy link
Contributor Author

Nyholm commented Jul 27, 2016

Of course. I've added some more tests now

@avigoldman
Copy link
Contributor

Looks great 😄 I'm curious what you think about removing the php-http/client-implementation dependency. It only seems complicated the install process without actually adding any value.

@Nyholm
Copy link
Contributor Author

Nyholm commented Jul 27, 2016

It is a virtual package. It means that you have to have a package that provides a php-http/client-implementation to be able to install sparkpost.

Virtual packages are used to make sure that composer fails if you do not have all the requirements. I would not remove it.

@avigoldman
Copy link
Contributor

avigoldman commented Jul 27, 2016

Right its just that the package won't work without it anyways and we've found that it has confused some people when installing the SparkPost package.
In either case, thanks again for contributing! I'm going to merge this branch in and create a new version ASAP.

@Nyholm
Copy link
Contributor Author

Nyholm commented Jul 27, 2016

Awesome. Thank you!

@avigoldman avigoldman merged commit a91a8e1 into SparkPost:master Jul 27, 2016
@Nyholm Nyholm deleted the dev-guzzle branch July 27, 2016 14:29
@Nyholm Nyholm mentioned this pull request Aug 6, 2016
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