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

Capture the request data to the response to enable logging and troubleshooting #40

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 0 additions & 1 deletion phpstan.neon
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
parameters:
checkMissingIterableValueType: false
customRulesetUsed: true
level: 1
reportUnmatchedIgnoredErrors: true
Expand Down
18 changes: 8 additions & 10 deletions src/Http/Client.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,9 @@ class Client
public $oauth2;

/**
* Client instance.
*
* @var \GuzzleHttp\Client
* The http request client.
*/
public $client;
public GuzzleClient $httpClient;

/**
* Guzzle allows options into its request method. Prepare for some defaults.
Expand All @@ -59,14 +57,14 @@ class Client
*
* @var string
*/
protected $user_agent = 'Chromatic_OrangeDam_PHP/1.0.0 (https://github.com/ChromaticHQ/orange-dam-php)';
protected $user_agent = 'Chromatic_OrangeDam_PHP (https://github.com/ChromaticHQ/orange-dam-php)';

/**
* Constructor.
*
* @throws \Chromatic\OrangeDam\Exceptions\OrangeDamException
*/
public function __construct(array $config = [], GuzzleClient $client = null, array $clientOptions = [])
public function __construct(array $config = [], GuzzleClient $httpClient = null, array $clientOptions = [])
{
// Set default property values.
$this->token = null;
Expand All @@ -80,14 +78,14 @@ public function __construct(array $config = [], GuzzleClient $client = null, arr
}

// Creates a new instance
if (is_null($client)) {
$client = new GuzzleClient([
if (is_null($httpClient)) {
$httpClient = new GuzzleClient([
'cookies' => true,
'timeout' => self::REQUEST_TIMEOUT,
'base_uri' => $config['base_path'],
]);
}
$this->client = $client;
$this->httpClient = $httpClient;
}

/**
Expand Down Expand Up @@ -128,7 +126,7 @@ public function request(

try {
// Make a request with given parameters.
return new Response($this->client->request($method, $url, $options));
return new Response($this->httpClient, $method, $url, $options);
Copy link
Member

Choose a reason for hiding this comment

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

@apotek Just checking if ->request() was accidentally left off. I see the original has it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apotek Just checking if ->request() was accidentally left off. I see the original has it.

You are right to ask about this. This change set changes the constructor of the Response class to no longer accept a GuzzleResponse (which is what client->request() would return), but instead accepts the request client object, and the request parameters:

https://github.com/ChromaticHQ/orange-dam-php/pull/40/files#diff-f6919b0f7fddf8ecc9da906c1a52a42e724966f3965be170e0ad30de2f2ee786L29-R37

This allows the constructor function in the Response object to save some data to itself about the request that is about to be made, so that the Response can be queried for data about the request that created that response.

} catch (ServerException $e) {
throw OrangeDamException::create($e);
} catch (ClientException $e) {
Expand Down
26 changes: 24 additions & 2 deletions src/Http/Response.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ class Response implements ResponseInterface
*/
public $data;

protected string $request;

protected array $requestOptions;

/**
* Response instance.
*
Expand All @@ -26,9 +30,11 @@ class Response implements ResponseInterface
* Constructor.
*
*/
public function __construct(ResponseInterface $response)
public function __construct(\GuzzleHttp\ClientInterface $client, string $method, string $url, array $options = [])
{
$this->response = $response;
$this->response = $client->request($method, $url, $options);
$this->request = sprintf('%s %s', $method, $url);
$this->requestOptions = $options;
}

/**
Expand Down Expand Up @@ -99,6 +105,22 @@ public function isEmpty(): bool
return is_null($this->response->getBody()->getSize());
}

/**
* Returns the request string that created this response.
*/
public function getRequest(): string
{
return $this->request;
}

/**
* Returns the request options array that created this response.
*/
public function getRequestOptions(): array
{
return $this->requestOptions;
}

/**
* {@inheritdoc}
*/
Expand Down
2 changes: 1 addition & 1 deletion tests/Http/ClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public function testClientCreation(): void
$client = new Client([
'base_path' => 'https://test.com',
]);
$this->assertInstanceOf(GuzzleClient::class, $client->client);
$this->assertInstanceOf(GuzzleClient::class, $client->httpClient);
}

/**
Expand Down
14 changes: 12 additions & 2 deletions tests/Http/ResponseTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace Chromatic\OrangeDam\Http;

use Chromatic\OrangeDam\Endpoints\Search;
use GuzzleHttp\Client as GuzzleClient;
use GuzzleHttp\Psr7\Response as GuzzleResponse;
use PHPUnit\Framework\TestCase;
use Psr\Http\Message\MessageInterface;
Expand All @@ -15,7 +17,11 @@ final class ResponseTest extends TestCase
*/
public function testDefaultConstructor(): void
{
$response = new Response(new GuzzleResponse());
$clientMock = $this->getMockBuilder(GuzzleClient::class)
->onlyMethods(['request'])
->getMock();
$clientMock->method('request')->willReturn(new GuzzleResponse());
$response = new Response($clientMock, 'GET', '');
$this->assertNull($response->getData());
$this->assertNull($response->toArray());
$this->assertInstanceOf(StreamInterface::class, $response->getBody());
Expand All @@ -35,7 +41,11 @@ public function testDefaultConstructor(): void
*/
public function testHeaders(): void
{
$response = new Response(new GuzzleResponse(200, ['Foo' => 'Bar']));
$clientMock = $this->getMockBuilder(GuzzleClient::class)
->onlyMethods(['request'])
->getMock();
$clientMock->method('request')->willReturn(new GuzzleResponse(200, ['Foo' => 'Bar']));
$response = new Response($clientMock, 'POST', '/');
$headers = $response->getHeaders();
$this->assertTrue($response->hasHeader('Foo'));
$this->assertSame('Bar', $response->getHeaderLine('Foo'));
Expand Down
Loading