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 Guzzle library #39

Closed
wants to merge 1 commit into from

Conversation

trandangtri
Copy link
Contributor

Close #34

@saimaz saimaz added the qa label Jan 5, 2016
@mvar
Copy link
Member

mvar commented Jan 5, 2016

@trandangtri you cannot call real 3rd party service in your tests. You should mock it somehow.

@saimaz
Copy link
Member

saimaz commented Jan 5, 2016

@trandangtri the issue is about to make it OPTIONAL not to REMOVE.

@saimaz
Copy link
Member

saimaz commented Jan 5, 2016

Guzzle has a http mock. You can use it to simulate actual request to openexchange service by pretending that it works.

@trandangtri
Copy link
Contributor Author

The OpenExchangeDriver just calls a normal json request to a 3rd party service, I dont think we need to use Guzzle. That's why I do suggest replace it by a curl request.

@saimaz
Copy link
Member

saimaz commented Jan 5, 2016

Disagree. Guzzle is perfect object oriented HTTP client, which is has tests, is testable and well maintained. OpenExchange rate is one of many 3rd party providers for currency. Normally when somebody will use this bundle they will need a HTTP connection anyway. So to give an advice for user to install Guzzle is nice to have IMO.

@saimaz
Copy link
Member

saimaz commented Jan 5, 2016

The only way to test a native curl function would be to use aspect oriented mocking library which would be hardcore style 😄

@murnieza
Copy link

murnieza commented Jan 5, 2016

Another advantage of using Guzzle is easily controllable logging.

I also think that it should not be replaced with plain curl.

@trandangtri
Copy link
Contributor Author

I misunderstood this issue. I think I should close this and will back with a new PR

@trandangtri trandangtri closed this Jan 6, 2016
@saimaz saimaz removed the qa label Jan 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.

None yet

4 participants