Skip to content

Allow overriding of HttpClient #49

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.

Sign up for GitHub

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
tchiotludo wants to merge 1 commit into from
Closed

Allow overriding of HttpClient #49

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 15 additions & 20 deletions src/Http/GraphRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,17 +231,17 @@ public function setTimeout($timeout)
public function execute($client = null)
{
if (is_null($client)) {
$client = $this->createGuzzleClient($this->proxyPort);
$client = new Client();
}

$result = $client->request(
$this->requestType,
$this->_getRequestUrl(),
[
array_merge($this->getGuzzleOptions($this->proxyPort), [
Copy link

@caitlinrussell caitlinrussell Dec 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a client is passed in, we should not assume they are using Guzzle

Copy link
Author

@tchiotludo tchiotludo Dec 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't really understand this comment.
We need to assume that people are using a compliant client that works with the SDK.

Since the http client interface is not standardize (like PSR7 for example), we need to declare the current implementation of a http client we are using.

From my point of view, we need to force an http client (Guzzle or HTTPlug or another one), the simpliest is Guzzle :

use GuzzleHttp\ClientInterface;

    /**
    * Executes the HTTP request using Guzzle
    *
    * @param ClientInterface $client The client to use in the request
    *
     * @throws GraphException if response is invalid
     *
    * @return mixed object or array of objects
    *         of class $returnType
    */
    public function execute(ClientInterface $client = null)
    {

If the people use HTTPlug, they can use the guzzle adapter to give to msgraph-sdk the client needed.

Or if you want to use HTTPlug to abstract completely, it's not the same work since it need to refractor the whole sdk since there is a lot of dependency with Guzzle (also on generated part on the sdk) and it will be breaking change and a major version bump

Copy link
Contributor

@Ndiritu Ndiritu Apr 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can work with Guzzle as a default for now as we prep for support PSR-18 HTTP clients in the future.

'body' => $this->requestBody,
'stream' => $this->returnsStream,
'timeout' => $this->timeout
]
])
);

// Wrap response in GraphResponse layer
Expand Down Expand Up @@ -272,17 +272,17 @@ public function execute($client = null)
public function executeAsync($client = null)
{
if (is_null($client)) {
$client = $this->createGuzzleClient($this->proxyPort);
$client = new Client();
}

$promise = $client->requestAsync(
$this->requestType,
$this->_getRequestUrl(),
[
array_merge($this->getGuzzleOptions($this->proxyPort), [
'body' => $this->requestBody,
'stream' => $this->returnsStream,
'timeout' => $this->timeout
]
])
)->then(
// On success, return the result/response
function ($result) {
Expand Down Expand Up @@ -322,19 +322,20 @@ function ($reason) {
public function download($path, $client = null)
{
if (is_null($client)) {
$client = $this->createGuzzleClient();
$client = new Client();
}

try {
if (file_exists($path) && is_writeable($path)) {
$file = fopen($path, 'w');

$client->request(
$this->requestType,
$this->_getRequestUrl(),
[
array_merge($this->getGuzzleOptions($this->proxyPort), [
'body' => $this->requestBody,
'sink' => $file
]
])
);
fclose($file);
} else {
Expand All @@ -359,9 +360,6 @@ public function download($path, $client = null)
*/
public function upload($path, $client = null)
{
if (is_null($client)) {
$client = $this->createGuzzleClient();
}
try {
if (file_exists($path) && is_readable($path)) {
$file = fopen($path, 'r');
Expand All @@ -384,7 +382,6 @@ public function upload($path, $client = null)
private function _getDefaultHeaders()
{
$headers = [
'Host' => $this->baseUrl,
'Content-Type' => 'application/json',
'SdkVersion' => 'Graph-php-' . GraphConstants::SDK_VERSION,
'Authorization' => 'Bearer ' . $this->accessToken
Expand All @@ -403,7 +400,7 @@ private function _getRequestUrl()
if (stripos($this->endpoint, "http") !== FALSE) {
return $this->endpoint;
}
return $this->apiVersion . $this->endpoint;
return $this->baseUrl . $this->apiVersion . $this->endpoint;
}

/**
Expand Down Expand Up @@ -431,20 +428,18 @@ protected function getConcatenator()
* @param string $proxyPort The port to forward
* requests through. If null, a proxy is not used
*
* @return \GuzzleHttp\Client The new client
* @return array
*/
protected function createGuzzleClient($proxyPort = null)
protected function getGuzzleOptions($proxyPort = null)
Copy link
Contributor

@Ndiritu Ndiritu Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be getDefaultGuzzleRequestOptions? Or basically add default somewhere in the naming? and without the proxyPort param passed in

{
$clientSettings = [
'base_uri' => $this->baseUrl,
'headers' => $this->headers
];
if ($proxyPort != null) {
$clientSettings['verify'] = false;
$clientSettings['proxy'] = $proxyPort;
}
$client = new Client($clientSettings);
}

return $client;
return $clientSettings;
}
}