[DX][WebProfilerBundle] Add Pretty Print functionality for Request Content #28919

SamFleming · 2018-10-18T20:26:37Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	n/a ?

Why?

Quite often when attempting to debug issues with JSON requests sent to a Symfony API, I use the Web Profiler to check the request content. More often than not the request content isn't easily readable (99% of the time it's all stuck on a single line and impossible to read). I always find myself copying + pasting the content into a random online tool to have it "pretty-print" the JSON.

Usually this isn't an issue, but can be annoying when offline. There's also the security issue of sending entire JSON payloads to a third-party server just for formatting 😳. Alternatively, maybe developers copy+paste into their chosen editors and this PR is all a waste of time — I hope not 😛.

How?

This PR adds "Pretty-Print" JSON functionality straight into the profiler.

We can use collector.requestheaders to detect if the request was JSON and conditionally show the Pretty Print button.

When the button is clicked, we format the JSON from the "Request Content" card.

What does it look like?

Before:

After:

Non-JSON Requests (unchanged):

Things to consider

Is JSON.stringify(JSON.parse(content)); the safest, most efficient way to do this?
Should the "Pretty Print" button be in-line next to the "Request Content" header? I couldn't find a pattern for this sort of thing elsewhere in the profiler.
Do people want JSON formatted with 4 spaces, would 2 spaces be preferred? Should this be a configuration option stored in localStorage (such as the light/dark theme configuration)?
Should this be a toggle? E.g. click to pretty print, then click to undo

Future Improvements

Depending on how this is received it could be extended to support formatting different request content-types (e.g. XML formatting) — I assume.

Progress

Gather feedback and decide where to perform the pretty-print: server-side, or client-side.
It was decided server-side would be better.

javiereguiluz · 2018-10-19T06:51:06Z

I like this proposal a lot. I would apply this behaviour by default, without any button or toggle. To prevent issues with complex queries, I'd apply a max-height of around 500px to the container element.

ro0NL · 2018-10-19T07:13:26Z

I think it should be pretty by default, but a Raw option could still be useful?

stof · 2018-10-19T08:31:53Z

and if we want to do it all the time (with a tab switcher to view the raw one), it would make sense to do the pretty-printing server-side.

SamFleming · 2018-10-19T09:15:47Z

👍 Sounds good. I'll look at getting that implemented.

If we're doing the pretty-printing on the server, that would mean we're putting the entire payload into the DOM twice, once "pretty-printed", the other raw – I'm assuming most payloads aren't too big so this won't be an issue - are we okay with this?

ro0NL · 2018-10-19T17:21:26Z

also instead of pretty JSON, wouldnt VarDumper (thus collapsible) be far more useful?

SamFleming · 2018-10-19T21:44:15Z

Okay, so, I've taken the above feedback, rebased and updated the UI. There are now "Pretty" and "Raw" tabs. I've left the formatting down to JS for reasons discussed later.

I definitely prefer this. Thank you for the suggestions 👍

and if we want to do it all the time (with a tab switcher to view the raw one), it would make sense to do the pretty-printing server-side.

I don't have much experience with the internals of the profiler. However, from what I can workout there are a few options we have to pretty print on the server.

I'm assuming the method of pretty-printing we'd use is json_encode(json_decode($requestContent), JSON_PRETTY_PRINT)) (remember, at the moment our $requestContent is a string). Alternatively, we can look for additional libraries, but I'm not sure what the stance on that is?

collector is basically $this->data from RequestDataCollector.php. We could store an additional key (e.g. "content_formatted") and perform our pretty-printing during collection.
Pros: Don't have to pretty print in the profiler (is that a pro?)
Cons: Performance affect during collection; storing the entire request payload twice in the profiler.
We perform the print-printing in ProfilerController::panelAction and add a key to the available twig variables https://github.com/SamFleming/symfony/blob/d56ed851b4f2d012c8633f0632feb96d17b96c17/src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php#L104-L112.
Pros: Not storing duplicate content.
Cons: Placing Collector specific logic in the generic panelAction — should it really be the responsibility of panelAction to look into the Collector and its attributes to perform the printing?
Something obvious I'm missing? 😝 🤞

It would be great to hear some feedback on the above options as I'm not really sure which route to take.

also instead of pretty JSON, wouldnt VarDumper (thus collapsible) be far more useful?

I had been thinking about this. My main motivation for not using VarDumper was maintaining the ability to copy the pretty-printed output and edit it (e.g. for use in Postman). If we output using the VarDumper copy + paste wouldn't be possible. Perhaps we could have an additional "Dump" tab which contains the output — then we have three variations of request content though 🤔

Sidenote:

I've had to create commit 9cdcf0b as there appears to be a bug when nesting tabs in the profiler panels. Looks like until this PR it's never been required. Shall I split this out into a separate PR?

stof · 2018-10-22T09:51:51Z

You could add a getter on the Collector object returning the pretty-printed JSON, without the need to do it at collect time (it can be computed from the raw content at render time)

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/request.html.twig

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig

src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php

src/Symfony/Component/HttpKernel/Tests/DataCollector/RequestDataCollectorTest.php

src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php

ro0NL · 2018-11-24T09:38:20Z

We could also add a 3rd tab: "Debug" that uses profiler_dump() whereas "Pretty" uses json_encode (both available in twig)

ro0NL

depending on #28919 (comment)

SamFleming · 2018-11-26T10:03:24Z

Thanks for the approval @ro0NL – I've updated the regex as suggested 👌 — It will match application/hal+ld+json, are you okay with that – alternatively we could change the quantifier to ? to only match one or zero (instead of zero or more) – your thoughts here would be appreciated.

I'm currently trying to setup the "Debug" tab as suggested but am struggling with some unexpected errors. I'll keep trying.

ro0NL · 2018-11-26T10:13:25Z

@SamFleming perhaps better to introduce the debug tab in a new PR so we dont block/postpone this one (i was just brainfarting :)) But i definitively think it's a nice addition.

It will match application/hal+ld+json, are you okay with that

yes, i tweaked it to more generic. Now matches foo+bar+json, foo+++json. I think we want to be tolerant yes.

SamFleming · 2018-11-26T16:42:27Z

@SamFleming perhaps better to introduce the debug tab in a new PR so we dont block/postpone this one (i was just brainfarting :)) But i definitively think it's a nice addition.

Awesome, 👌

Anything left for me to do to get this ready for merging?

ro0NL · 2018-11-26T16:53:05Z

let's wait :) i do have one last question though; what happens if the request content is not valid JSON? Even if the content-type says so.. does it crash the panel? Will developer see the error?

SamFleming · 2018-11-26T19:07:11Z

Is what shows when the JSON is invalid. json_decode will return NULL when invalid JSON is encountered, then json_encode(NULL) returns null.

We could check json_last_error then display json_last_error_msg in the "Pretty" tab if there's an issue?

ro0NL · 2018-11-26T19:10:07Z

Perhaps fallback to raw content only without tabs then. I.e. the current behavior

SamFleming · 2018-11-26T19:13:48Z

Done 63c0219

Is this too... horrible?

ro0NL

the !="null" check is implicit, but if the null is valid raw&pretty show the same content anyway, so it doesnt really hurt and avoids 2 tabs showing null 👍

ro0NL

Explicit also fine for me 😅 t:

SamFleming · 2018-11-26T19:37:50Z

Oh... I thought I saw a comment about checking json_last_error 😅. I've implemented here 9446deb

I definitely prefer, as it removes any abiquity. However, if we're going this route, is there any point checking the Content-Type for being JSON? Is it worth attempting the json_encode(json_decode()) no matter what is in the body, then using json_last_error to determine whether the tab shows?

ro0NL · 2018-11-26T19:43:59Z

the extra overhead might bite us as we try to parse each request content, every time. Tend to prefer the explicit check here.

SamFleming · 2018-11-26T19:45:21Z

I also wonder if it's worth caching the result of getPrettyJson seeing as it is being called twice in twig and I'm assume json_decode(json_encode()) could be quite resource intensive... or assigning it to a variable in twig.

SamFleming · 2018-11-26T19:45:36Z

the extra overhead might bite us as we try to parse each request content, every time. Tend to prefer the explicit check here.

👌 Fair

ro0NL · 2018-11-26T19:47:21Z

actually we dont parse during the request :D (im tired), only during display in the profiler. But yeah, i think we should be conservative, and expand as needed.

ro0NL · 2018-11-26T19:48:53Z

I also wonder if it's worth caching the result of getPrettyJson seeing as it is being called twice in twig and I'm assume json_decode(json_encode()) could be quite resource intensive... or assigning it to a variable in twig.

correct. We should return string|null (checking for last_error intermediary) and call it once in twig.

SamFleming · 2018-11-26T19:53:28Z

We should return string|null (checking for last_error intermediary)

Apologies, not quite sure what you mean here. I was thinking this 91a2b39

ro0NL · 2018-11-26T20:01:06Z

return "string" for valid JSON, or null otherwise. Then in twig prettyJson is not null :)

ro0NL

👌

SamFleming · 2019-02-13T16:34:13Z

@dunglas was there anything else you wanted me to do on this?

…ntent

fabpot · 2019-02-21T09:58:03Z

Thank you @SamFleming.

… for Request Content (SamFleming) This PR was squashed before being merged into the 4.3-dev branch (closes #28919). Discussion ---------- [DX][WebProfilerBundle] Add Pretty Print functionality for Request Content | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | n/a ? ## Why? Quite often when attempting to debug issues with JSON requests sent to a Symfony API, I use the Web Profiler to check the request content. More often than not the request content isn't easily readable (99% of the time it's all stuck on a single line and impossible to read). I always find myself copying + pasting the content into a random online tool to have it "pretty-print" the JSON. Usually this isn't an issue, but can be annoying when offline. There's also the security issue of sending entire JSON payloads to a third-party server just for formatting 😳. Alternatively, maybe developers copy+paste into their chosen editors and this PR is all a waste of time — I hope not 😛. ## How? This PR adds "Pretty-Print" JSON functionality straight into the profiler. We can use `collector.requestheaders` to detect if the request was JSON and conditionally show the Pretty Print button. When the button is clicked, we format the JSON from the "Request Content" card. ## What does it look like? Before: ![without-pretty-print](https://user-images.githubusercontent.com/573318/47180751-36b0ce00-d319-11e8-86ed-eb0d78ebcbe3.png) After: ![pretty](https://user-images.githubusercontent.com/573318/47180763-3c0e1880-d319-11e8-995d-eba565aad827.png) Non-JSON Requests (unchanged): ![non-json-request](https://user-images.githubusercontent.com/573318/47181080-03227380-d31a-11e8-8cf2-e8b2e8c1a21d.png) ## Things to consider - Is `JSON.stringify(JSON.parse(content));` the safest, most efficient way to do this? - Should the "Pretty Print" button be in-line next to the "Request Content" header? I couldn't find a pattern for this sort of thing elsewhere in the profiler. - Do people want JSON formatted with 4 spaces, would 2 spaces be preferred? Should this be a configuration option stored in localStorage (such as the light/dark theme configuration)? - Should this be a toggle? E.g. click to pretty print, then click to undo ## Future Improvements Depending on how this is received it could be extended to support formatting different request content-types (e.g. XML formatting) — I assume. ## Progress - [x] Gather feedback and decide where to perform the pretty-print: [server-side, or client-side](symfony/symfony#28919 (comment)). *It was decided server-side would be better.* Commits ------- 9f85103151 [DX][WebProfilerBundle] Add Pretty Print functionality for Request Content

… for Request Content (SamFleming) This PR was squashed before being merged into the 4.3-dev branch (closes #28919). Discussion ---------- [DX][WebProfilerBundle] Add Pretty Print functionality for Request Content | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | n/a ? ## Why? Quite often when attempting to debug issues with JSON requests sent to a Symfony API, I use the Web Profiler to check the request content. More often than not the request content isn't easily readable (99% of the time it's all stuck on a single line and impossible to read). I always find myself copying + pasting the content into a random online tool to have it "pretty-print" the JSON. Usually this isn't an issue, but can be annoying when offline. There's also the security issue of sending entire JSON payloads to a third-party server just for formatting 😳. Alternatively, maybe developers copy+paste into their chosen editors and this PR is all a waste of time — I hope not 😛. ## How? This PR adds "Pretty-Print" JSON functionality straight into the profiler. We can use `collector.requestheaders` to detect if the request was JSON and conditionally show the Pretty Print button. When the button is clicked, we format the JSON from the "Request Content" card. ## What does it look like? Before: ![without-pretty-print](https://user-images.githubusercontent.com/573318/47180751-36b0ce00-d319-11e8-86ed-eb0d78ebcbe3.png) After: ![pretty](https://user-images.githubusercontent.com/573318/47180763-3c0e1880-d319-11e8-995d-eba565aad827.png) Non-JSON Requests (unchanged): ![non-json-request](https://user-images.githubusercontent.com/573318/47181080-03227380-d31a-11e8-8cf2-e8b2e8c1a21d.png) ## Things to consider - Is `JSON.stringify(JSON.parse(content));` the safest, most efficient way to do this? - Should the "Pretty Print" button be in-line next to the "Request Content" header? I couldn't find a pattern for this sort of thing elsewhere in the profiler. - Do people want JSON formatted with 4 spaces, would 2 spaces be preferred? Should this be a configuration option stored in localStorage (such as the light/dark theme configuration)? - Should this be a toggle? E.g. click to pretty print, then click to undo ## Future Improvements Depending on how this is received it could be extended to support formatting different request content-types (e.g. XML formatting) — I assume. ## Progress - [x] Gather feedback and decide where to perform the pretty-print: [server-side, or client-side](symfony/symfony#28919 (comment)). *It was decided server-side would be better.* Commits ------- 9f85103 [DX][WebProfilerBundle] Add Pretty Print functionality for Request Content

… for Request Content (SamFleming) This PR was squashed before being merged into the 4.3-dev branch (closes #28919). Discussion ---------- [DX][WebProfilerBundle] Add Pretty Print functionality for Request Content | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | n/a ? ## Why? Quite often when attempting to debug issues with JSON requests sent to a Symfony API, I use the Web Profiler to check the request content. More often than not the request content isn't easily readable (99% of the time it's all stuck on a single line and impossible to read). I always find myself copying + pasting the content into a random online tool to have it "pretty-print" the JSON. Usually this isn't an issue, but can be annoying when offline. There's also the security issue of sending entire JSON payloads to a third-party server just for formatting 😳. Alternatively, maybe developers copy+paste into their chosen editors and this PR is all a waste of time — I hope not 😛. ## How? This PR adds "Pretty-Print" JSON functionality straight into the profiler. We can use `collector.requestheaders` to detect if the request was JSON and conditionally show the Pretty Print button. When the button is clicked, we format the JSON from the "Request Content" card. ## What does it look like? Before: ![without-pretty-print](https://user-images.githubusercontent.com/573318/47180751-36b0ce00-d319-11e8-86ed-eb0d78ebcbe3.png) After: ![pretty](https://user-images.githubusercontent.com/573318/47180763-3c0e1880-d319-11e8-995d-eba565aad827.png) Non-JSON Requests (unchanged): ![non-json-request](https://user-images.githubusercontent.com/573318/47181080-03227380-d31a-11e8-8cf2-e8b2e8c1a21d.png) ## Things to consider - Is `JSON.stringify(JSON.parse(content));` the safest, most efficient way to do this? - Should the "Pretty Print" button be in-line next to the "Request Content" header? I couldn't find a pattern for this sort of thing elsewhere in the profiler. - Do people want JSON formatted with 4 spaces, would 2 spaces be preferred? Should this be a configuration option stored in localStorage (such as the light/dark theme configuration)? - Should this be a toggle? E.g. click to pretty print, then click to undo ## Future Improvements Depending on how this is received it could be extended to support formatting different request content-types (e.g. XML formatting) — I assume. ## Progress - [x] Gather feedback and decide where to perform the pretty-print: [server-side, or client-side](#28919 (comment)). *It was decided server-side would be better.* Commits ------- 9f85103 [DX][WebProfilerBundle] Add Pretty Print functionality for Request Content

carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) WebProfilerBundle Feature labels Oct 18, 2018

SamFleming force-pushed the feature/request-content-json-pretty-print branch from d985841 to d56ed85 Compare October 19, 2018 21:03

SamFleming changed the title ~~[DX][WebProfilerBundle] Add Pretty Print functionality for Request Content~~ [WIP][DX][WebProfilerBundle] Add Pretty Print functionality for Request Content Oct 19, 2018

nicolas-grekas added this to the next milestone Oct 20, 2018

dunglas requested changes Nov 1, 2018

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/request.html.twig Outdated Show resolved Hide resolved

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/request.html.twig Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 1, 2018

SamFleming force-pushed the feature/request-content-json-pretty-print branch from d73ef2e to 06361a0 Compare November 22, 2018 14:09

ro0NL reviewed Nov 22, 2018

View reviewed changes

src/Symfony/Bundle/WebProfilerBundle/Resources/views/Profiler/base_js.html.twig Show resolved Hide resolved

ro0NL reviewed Nov 24, 2018

View reviewed changes

src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php Outdated Show resolved Hide resolved

ro0NL approved these changes Nov 26, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Nov 26, 2018

SamFleming changed the title ~~[WIP][DX][WebProfilerBundle] Add Pretty Print functionality for Request Content~~ [DX][WebProfilerBundle] Add Pretty Print functionality for Request Content Nov 26, 2018

ro0NL approved these changes Nov 26, 2018

View reviewed changes

SamFleming force-pushed the feature/request-content-json-pretty-print branch from df6deac to 9446deb Compare November 26, 2018 19:35

ro0NL approved these changes Nov 26, 2018

View reviewed changes

fabpot approved these changes Feb 21, 2019

View reviewed changes

[DX][WebProfilerBundle] Add Pretty Print functionality for Request Co…

9f85103

…ntent

fabpot force-pushed the feature/request-content-json-pretty-print branch from 9fcbb1e to 9f85103 Compare February 21, 2019 09:58

fabpot merged commit 9f85103 into symfony:master Feb 21, 2019

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

fabpot mentioned this pull request May 9, 2019

Release v4.3.0-BETA1 #31435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DX][WebProfilerBundle] Add Pretty Print functionality for Request Content #28919

[DX][WebProfilerBundle] Add Pretty Print functionality for Request Content #28919

SamFleming commented Oct 18, 2018 •

edited

Loading

javiereguiluz commented Oct 19, 2018

ro0NL commented Oct 19, 2018

stof commented Oct 19, 2018

SamFleming commented Oct 19, 2018

ro0NL commented Oct 19, 2018 •

edited

Loading

SamFleming commented Oct 19, 2018 •

edited

Loading

stof commented Oct 22, 2018

ro0NL commented Nov 24, 2018

ro0NL left a comment

SamFleming commented Nov 26, 2018

ro0NL commented Nov 26, 2018

SamFleming commented Nov 26, 2018

ro0NL commented Nov 26, 2018

SamFleming commented Nov 26, 2018 •

edited

Loading

ro0NL commented Nov 26, 2018

SamFleming commented Nov 26, 2018

ro0NL left a comment

ro0NL left a comment

SamFleming commented Nov 26, 2018 •

edited

Loading

ro0NL commented Nov 26, 2018

SamFleming commented Nov 26, 2018

SamFleming commented Nov 26, 2018

ro0NL commented Nov 26, 2018

ro0NL commented Nov 26, 2018 •

edited

Loading

SamFleming commented Nov 26, 2018 •

edited

Loading

ro0NL commented Nov 26, 2018

ro0NL left a comment

SamFleming commented Feb 13, 2019

fabpot commented Feb 21, 2019

[DX][WebProfilerBundle] Add Pretty Print functionality for Request Content #28919

[DX][WebProfilerBundle] Add Pretty Print functionality for Request Content #28919

Conversation

SamFleming commented Oct 18, 2018 • edited Loading

Why?

How?

What does it look like?

Things to consider

Future Improvements

Progress

javiereguiluz commented Oct 19, 2018

ro0NL commented Oct 19, 2018

stof commented Oct 19, 2018

SamFleming commented Oct 19, 2018

ro0NL commented Oct 19, 2018 • edited Loading

SamFleming commented Oct 19, 2018 • edited Loading

stof commented Oct 22, 2018

ro0NL commented Nov 24, 2018

ro0NL left a comment

Choose a reason for hiding this comment

SamFleming commented Nov 26, 2018

ro0NL commented Nov 26, 2018

SamFleming commented Nov 26, 2018

ro0NL commented Nov 26, 2018

SamFleming commented Nov 26, 2018 • edited Loading

ro0NL commented Nov 26, 2018

SamFleming commented Nov 26, 2018

ro0NL left a comment

Choose a reason for hiding this comment

ro0NL left a comment

Choose a reason for hiding this comment

SamFleming commented Nov 26, 2018 • edited Loading

ro0NL commented Nov 26, 2018

SamFleming commented Nov 26, 2018

SamFleming commented Nov 26, 2018

ro0NL commented Nov 26, 2018

ro0NL commented Nov 26, 2018 • edited Loading

SamFleming commented Nov 26, 2018 • edited Loading

ro0NL commented Nov 26, 2018

ro0NL left a comment

Choose a reason for hiding this comment

SamFleming commented Feb 13, 2019

fabpot commented Feb 21, 2019

SamFleming commented Oct 18, 2018 •

edited

Loading

ro0NL commented Oct 19, 2018 •

edited

Loading

SamFleming commented Oct 19, 2018 •

edited

Loading

SamFleming commented Nov 26, 2018 •

edited

Loading

SamFleming commented Nov 26, 2018 •

edited

Loading

ro0NL commented Nov 26, 2018 •

edited

Loading

SamFleming commented Nov 26, 2018 •

edited

Loading