Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@SamFleming
Copy link
Contributor

@SamFlemingSamFleming commentedOct 18, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PRn/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 usecollector.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

After:
pretty

Non-JSON Requests (unchanged):
non-json-request

Things to consider

  • IsJSON.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

Koc, leonaves, plozmun, fbourigault, Kocal, noniagriconomie, ro0NL, zmitic, and bashleigh reacted with thumbs up emojiogizanagi and yceruto reacted with heart emoji
@carsonbotcarsonbot added Status: Needs Review DXDX = Developer eXperience (anything that improves the experience of using Symfony) WebProfilerBundle Feature labelsOct 18, 2018
@javiereguiluz
Copy link
Member

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 amax-height of around 500px to the container element.

@ro0NL
Copy link
Contributor

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

dunglas and apfelbox reacted with thumbs up emoji

@stof
Copy link
Member

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.

ogizanagi reacted with thumbs up emoji

@SamFleming
Copy link
ContributorAuthor

👍 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
Copy link
Contributor

ro0NL commentedOct 19, 2018•
edited
Loading

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

@SamFlemingSamFlemingforce-pushed thefeature/request-content-json-pretty-print branch fromd985841 tod56ed85CompareOctober 19, 2018 21:03
@SamFleming
Copy link
ContributorAuthor

SamFleming commentedOct 19, 2018•
edited
Loading

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.

screenshot 2018-10-19 at 22 04 05
screenshot 2018-10-19 at 22 04 33

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 isjson_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?

  1. collector is basically$this->data fromRequestDataCollector.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.
  2. We perform the print-printing inProfilerController::panelAction and add a key to the available twig variableshttps://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 genericpanelAction — should it really be the responsibility ofpanelAction to look into the Collector and its attributes to perform the printing?
  3. 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 inPostman). If we output using theVarDumper 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 commit9cdcf0b 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?

@SamFlemingSamFleming changed the title[DX][WebProfilerBundle] Add Pretty Print functionality for Request Content[WIP][DX][WebProfilerBundle] Add Pretty Print functionality for Request ContentOct 19, 2018
@nicolas-grekasnicolas-grekas added this to thenext milestoneOct 20, 2018
@stof
Copy link
Member

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)

SamFleming reacted with thumbs up emoji

@SamFlemingSamFlemingforce-pushed thefeature/request-content-json-pretty-print branch fromd73ef2e to06361a0CompareNovember 22, 2018 14:09
@ro0NL
Copy link
Contributor

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

Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

depending on#28919 (comment)

@SamFleming
Copy link
ContributorAuthor

Thanks for the approval@ro0NL – I've updated the regex as suggested 👌 — It will matchapplication/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
Copy link
Contributor

@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 matchesfoo+bar+json, foo+++json. I think we want to be tolerant yes.

SamFleming reacted with thumbs up emoji

@SamFlemingSamFleming changed the title[WIP][DX][WebProfilerBundle] Add Pretty Print functionality for Request Content[DX][WebProfilerBundle] Add Pretty Print functionality for Request ContentNov 26, 2018
@SamFleming
Copy link
ContributorAuthor

@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
Copy link
Contributor

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
Copy link
ContributorAuthor

SamFleming commentedNov 26, 2018•
edited
Loading

image

Is what shows when the JSON is invalid.json_decode will returnNULL when invalid JSON is encountered, thenjson_encode(NULL) returnsnull.

We could checkjson_last_error then displayjson_last_error_msg in the "Pretty" tab if there's an issue?

@ro0NL
Copy link
Contributor

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

@SamFleming
Copy link
ContributorAuthor

Done63c0219

Is this too... horrible?

Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

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 👍

@SamFlemingSamFlemingforce-pushed thefeature/request-content-json-pretty-print branch fromdf6deac to9446debCompareNovember 26, 2018 19:35
Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

Explicit also fine for me 😅 t:

@SamFleming
Copy link
ContributorAuthor

SamFleming commentedNov 26, 2018•
edited
Loading

Oh... I thought I saw a comment about checkingjson_last_error 😅. I've implemented here9446deb

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 thejson_encode(json_decode()) no matter what is in the body, then usingjson_last_error to determine whether the tab shows?

@ro0NL
Copy link
Contributor

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

@SamFleming
Copy link
ContributorAuthor

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

@SamFleming
Copy link
ContributorAuthor

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
Copy link
Contributor

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
Copy link
Contributor

ro0NL commentedNov 26, 2018•
edited
Loading

I also wonder if it's worth caching the result ofgetPrettyJson seeing as it is being called twice in twig and I'm assumejson_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
Copy link
ContributorAuthor

SamFleming commentedNov 26, 2018•
edited
Loading

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

Apologies, not quite sure what you mean here. I was thinking this91a2b39

@ro0NL
Copy link
Contributor

return"string" for valid JSON, ornull otherwise. Then in twigprettyJson is not null :)

Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

👌

@SamFleming
Copy link
ContributorAuthor

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

@fabpot
Copy link
Member

Thank you@SamFleming.

@fabpotfabpotforce-pushed thefeature/request-content-json-pretty-print branch from9fcbb1e to9f85103CompareFebruary 21, 2019 09:58
symfony-splitter pushed a commit to symfony/twig-bundle that referenced this pull requestFeb 21, 2019
… 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 ImprovementsDepending 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
symfony-splitter pushed a commit to symfony/web-profiler-bundle that referenced this pull requestFeb 21, 2019
… 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 ImprovementsDepending 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
@fabpotfabpot merged commit9f85103 intosymfony:masterFeb 21, 2019
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull requestFeb 21, 2019
… 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 ImprovementsDepending 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
fabpot added a commit that referenced this pull requestFeb 21, 2019
… 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 ImprovementsDepending 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
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@dunglasdunglasdunglas requested changes

@fabpotfabpotfabpot approved these changes

+1 more reviewer

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

DXDX = Developer eXperience (anything that improves the experience of using Symfony)FeatureStatus: ReviewedWebProfilerBundle

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

8 participants

@SamFleming@javiereguiluz@ro0NL@stof@fabpot@dunglas@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp