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

Conversation

apotek
Copy link
Contributor

@apotek apotek commented Sep 26, 2024

Description

In this merge request we add the API request data to our custom response object so that any caller who receives a response will be able to see the exact request data that produced the response. This can be helpful for diagnostics and troubleshooting,.

Motivation / Context

Greater insight into what the API is doing.

Resolves #39

Testing Instructions / How This Has Been Tested

Documentation

@apotek apotek self-assigned this Sep 26, 2024
@apotek
Copy link
Contributor Author

apotek commented Sep 26, 2024

The failing tests are not trivial to fix. Will take some work.

@apotek apotek requested review from mmatsoo and removed request for agarzola October 15, 2024 05:38
@@ -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.

Copy link
Member

@mmatsoo mmatsoo left a comment

Choose a reason for hiding this comment

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

👍 I asked a question to double-check something that caught my eye, but otherwise looks mean and clean.

@apotek apotek changed the base branch from 1.x to 2.x October 15, 2024 19:02
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@apotek apotek merged commit f21c3a5 into 2.x Oct 15, 2024
4 checks passed
@apotek apotek deleted the 39-provide-a-means-for-implementors-to-be-able-to-access-the-final-api-request-made branch October 15, 2024 20:12
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.

Provide a means for implementors to be able to access the final API request made
2 participants