Library does not depend on specific http client implementation (PSR-18) #79
Conversation
There was a problem hiding this comment.
There is a bug that causes integration tests to fail. Try to run them, even providing invalid access token, you will see.
You also made a requirements conflict - we still have to support 7.0 for some time.
While working on library we made a decision to mark all non api methods and classes as internal using '@internal' class annotation. Try to fix this and avoid final class keyword.
For more details see code comments.
| $this->uriFactory = $uriFactory; | ||
| $this->streamFactory = $streamFactory; | ||
|
|
||
| $this->method = RequestMethodInterface::METHOD_GET; |
There was a problem hiding this comment.
Name it and move to 'defaults' method.
There was a problem hiding this comment.
Could you elaborate? This is default state, you want to have no default state and move initialization to separate defaults method?
There was a problem hiding this comment.
Keep that default state but name it by extracting to method.
| "dealerdirect/phpcodesniffer-composer-installer": "^0.4" | ||
| "dealerdirect/phpcodesniffer-composer-installer": "^0.4", | ||
| "php-http/httplug": "2.x-dev@dev", | ||
| "php-http/guzzle6-adapter": "dev-patch-psr18@dev", |
There was a problem hiding this comment.
It seems that this package requires php 7.1 (check Travis output for this PR). This client has to provide support for php 7.0 also. It there a way to test it with 7.0?
There was a problem hiding this comment.
I need to think about. This is only for tests, so maybe I can prepare a PR for adapter supporting 7.0 so we can use it instead?
I guess you have a good reason to support 7.0 because it's reaching EOL soon.
I totally missed that during development.
There was a problem hiding this comment.
Looks like guzzle itself will end with php 7.0 support, but PR is very wip: https://github.com/guzzle/guzzle/pull/2122/files
There was a problem hiding this comment.
Sorry, but supporting 7.0 is our bussiness requirement.
|
It seems like you disable proxy feature, see SmsapiClient::setProxy. |
|
Yes - this is implementation-specific so it would make sense to tell users how to setup proxy with implementation of choice. Now I'm thinking abbout logging - this should be also something that users handle on their side by decorating client (exactly the same way this is currently implemented, by outside of this lib) - of course we can provide decorator and tell them how to use it. WDYT? I will rebase this PR in the following days. |
|
Split this PR into smaller chunks. Separate PSR-18 changes from other changes (i.e. HTTP statuses and methods). That will make that PRs more suitable for next reviews. 80+ files to check is really a challenge. |
Hi.
As discussed in #78 PR introduces PSR-18 to this library.
Had to rewrite a lot of stuff to do this, and I am not sure if you'll like it :)
Would love to head what do you think, then we can discuss it and work something out :)
I changed how executors work, introduced request/response builders, managed to get rid of internal request/response implementations.
Unit tests pass, but I couldn't test integration with real service.
TODO: