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

Add http client request watcher #1073

Merged
merged 8 commits into from
Jun 9, 2021

Conversation

gdebrauwer
Copy link
Contributor

@gdebrauwer gdebrauwer commented Jun 6, 2021

In Laravel 8.45.0 (laravel/framework#37572), events were introduced in the Http Client. I thought it would be nice to add a watcher for the Http Client, using these new events. This will make it easier to debug requests/responses of the Http Client.

In the detail view, you can see the request payload and response body, but also the headers of both the request and the response.

The request payload and headers are being filtered based on the Telescope::$hiddenRequestParameters, similar to a regular Laravel request in Telescope.

The request body is converted to json (just as as is the case with a regular Laravel request in Telescope). There is one limitation currently in case of a multipart request. If the request contains a file without a filename or headers, then that file will not be converted to a json object containing name and size and headers. Instead the full file content will still be included in the saved request payload. This is because I don't think there is a way to detect if "contents" in multipart item is a file or not.

Schermafbeelding 2021-06-08 om 20 19 11

Schermafbeelding 2021-06-08 om 20 19 29

<li class="nav-item">
<router-link active-class="active" to="/client-requests" class="nav-link d-flex align-items-center">
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20">
<path d="M7 3H2v14h5V3zm2 0v14h9V3H9zM0 3c0-1.1.9-2 2-2h16a2 2 0 0 1 2 2v14a2 2 0 0 1-2 2H2a2 2 0 0 1-2-2V3zm3 1h3v2H3V4zm0 3h3v2H3V7zm0 3h3v2H3v-2z"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

same as "views" icon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced it with a globe icon from zondicons. (that is also the source of the other icons according to #679 (comment))

composer.json Outdated
@@ -16,6 +16,7 @@
"require": {
"php": "^7.3|^8.0",
"ext-json": "*",
"guzzlehttp/guzzle": "^7.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Guzzle itself is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locally I needed to install guzzle to run the tests, but you are right that this should not be a requirement to install Telescope. I removed the dependency and I updated the "tests" github workflow to install the "guzzlehttp/guzzle" composer package before running the tests.

@gdebrauwer gdebrauwer marked this pull request as ready for review June 8, 2021 18:32
{
parent::getEnvironmentSetUp($app);

if (! class_exists(\GuzzleHttp\Client::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

Import this class.

@@ -39,7 +39,7 @@ jobs:

- name: Install dependencies
run: |
composer require "illuminate/contracts=${{ matrix.laravel }}" --no-update
composer require "illuminate/contracts=${{ matrix.laravel }}" "guzzlehttp/guzzle" --no-update
Copy link
Member

Choose a reason for hiding this comment

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

Add this to the dev requirements instead please.

@taylorotwell taylorotwell merged commit 8edaeca into laravel:4.x Jun 9, 2021
@driesvints
Copy link
Member

Hey @gdebrauwer, can you also send in a PR to document this? https://laravel.com/docs/8.x/telescope

@gdebrauwer
Copy link
Contributor Author

Yes of course! 👍

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.

5 participants