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

Replace Buzz with HttpClient #68

Closed

Conversation

Projects
None yet
2 participants
@renanbr
Copy link

commented Apr 18, 2019

No description provided.

@nicolas-grekas
Copy link
Contributor

left a comment

LGTM thanks. Here are some minor comments.
Could you please also bump the branch alias to "dev-master": "5.x-dev" and declare php >=7.1.3 in composer.json?
It looks like we're missing a .travis.yml file btw. Just saying... :)

Show resolved Hide resolved src/SymfonyCorp/Connect/Api/Api.php Outdated
Show resolved Hide resolved src/SymfonyCorp/Connect/Api/Api.php Outdated
Show resolved Hide resolved src/SymfonyCorp/Connect/OAuthConsumer.php Outdated

@renanbr renanbr force-pushed the renanbr:http-client branch from 50f4767 to 7360588 Apr 19, 2019

@renanbr

This comment has been minimized.

Copy link
Author

commented Apr 19, 2019

Updates:

  • uses $exception->getMessage() when possible in cases we wrap exceptions
  • keeps $response variable naming in OAuthConsumer
  • branch alias to "dev-master": "5.x-dev"
  • bump php dep to >=7.1.3
  • remove non supported version from .travis.yml

I don't know why travis is not triggered, btw tests are green on local PHP 7.1.28, 7.2.17 and 7.3.4

@nicolas-grekas
Copy link
Contributor

left a comment

Because this bumps to PHP 7.1, we're going to have it part of 5.1 (5.0 should still use Guzzle/Buzz)
@azjezz could you please have a look to rebase+finish this one?

"kriswallsmith/buzz": ">=0.8,<0.17",
"psr/log": "^1.0"
"psr/log": "^1.0",
"symfony/contracts": "1.1.x@dev",

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 4, 2019

Contributor

can be removed now that the component is stable

"psr/log": "^1.0"
"psr/log": "^1.0",
"symfony/contracts": "1.1.x@dev",
"symfony/http-client": "4.3.x@dev",

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 4, 2019

Contributor

^4.3

@@ -25,7 +27,7 @@
},
"extra": {
"branch-alias": {
"dev-migrate-to-symfonyconnect": "5.x-dev"
"dev-master": "5.x-dev"

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 4, 2019

Contributor

5.1-dev

private $parser;
private $logger;
private $endpoint;
private $accessToken;
public function __construct($endpoint = null, Browser $browser = null, ParserInterface $parser = null, LoggerInterface $logger = null)
public function __construct($endpoint = null, HttpClientInterface $httpClient = null, ParserInterface $parser = null, LoggerInterface $logger = null)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 4, 2019

Contributor

the type-hint should be replaced by a docblock annotation and passing a Browser should trigger a deprecation (and we should then act as if null was passed)

public function __construct($appId, $appSecret, $scope, $endpoint = null, Browser $browser = null, LoggerInterface $logger = null)
public function __construct($appId, $appSecret, $scope, $endpoint = null, HttpClientInterface $httpClient = null, LoggerInterface $logger = null)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 4, 2019

Contributor

same as Api class

@@ -149,9 +155,9 @@ public function getEndpoint()
return $this->endpoint;
}
public function getBrowser()

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 4, 2019

Contributor

should be deprecated instead and return a dummy new Browser(new Curl());

@@ -149,9 +155,9 @@ public function getEndpoint()
return $this->endpoint;
}
public function getBrowser()
public function getHttpClient()

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jul 4, 2019

Contributor

if unused, should be removed (and if used, let's double check it's legit and remove if not)

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

Continued in #78, thanks @renanbr

nicolas-grekas added a commit that referenced this pull request Jul 5, 2019

feature #78 Replace Buzz with HttpClient (renanbr, azjezz)
This PR was merged into the 5.0-dev branch.

Discussion
----------

Replace Buzz with HttpClient

see #68 / #77

Commits
-------

40d290b deprecate using Buzz\Browser
bc28a50 Replace Buzz with HttpClient
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.