Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebProfilerBundle] Improved cookie traffic#20567
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ogizanagi commentedNov 19, 2016
Shouldn't the header bag part (the bug fix) be in a separated PR targeting older branches ? |
ro0NL commentedNov 19, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
And of course this is one of those files that is different in all branches ( Ie. the bug is fixed by a new feature :) |
| {%blockbody %} | ||
| {%forkeyinbag.keys|sort %} | ||
| {%forvaluein bag.get(key, [],false) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
On the request panel, what about keeping one line by header but exposing the value as an array if needed instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Considered it :) however what i like about this, it shows headers more or less the same as the HTTP message itself.
This goes for allHeaderBag's btw, as we cannot callbag.get(key, [], false) on aParamaterBag (hence the new template file).
nicolas-grekas commentedNov 22, 2016
Displaying multiple HTTP headers should be fixed in#20595. |
ro0NL commentedNov 22, 2016
Do you think the cookie panel is viable? I tend to like it (cookies shown as data objects that is). I can update the PR to only include that.. so multiple header fixes goes with#20595 ? |
| $cookies[] =$this->getCookieHeader($cookie->getName(),$cookie->getValue(),$cookie->getExpiresTime(),$cookie->getPath(),$cookie->getDomain(),$cookie->isSecure(),$cookie->isHttpOnly()); | ||
| } | ||
| if (count($cookies) >0) { | ||
| $responseHeaders['Set-Cookie'] =$cookies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And perhaps fixup 2.7 first with#20599
This PR was merged into the 3.3-dev branch.Discussion----------[HttpKernel] Continuation of#20569| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20567 (comment)| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->Forgotten in#20569 and makes#20567 truly a feature only.BeforeAfterCommits-------e1616b3 [HttpKernel] Continuation of#20569
ro0NL commentedDec 22, 2016
@javiereguiluz now all the cookie hassle is fixed.. WDYT? (bug label can be removed) |
fabpot commentedMar 1, 2017
@javiereguiluz Can you review this PR? Is it ok for you? |
fabpot commentedMar 22, 2017
@ro0NL Can this one be merged? |
ro0NL commentedMar 22, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@fabpot for me yes :) rebased and updated. Waiting for travis. |
fabpot commentedMar 22, 2017
@ro0NL Apparently, that breaks the tests. |
| <h3class="tab-title">Cookies</h3> | ||
| <divclass="tab-content"> | ||
| <h3>Cookies Received</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just asking: would the following changes make this easier to understand?
Instead ofCookies Received ->Request Cookies and
instead ofCookies Created ->Response Cookies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No strong opinion.. i think i've tried to avoid those terms because of theRequest | Response tabs already on the left (which also shows the cookies as raw headers). This is more the makeup version :)
But maybe you're right...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think@javiereguiluz is right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The problem is that if I read "Cookies Received" I may wonder: "received by my server" (request cookies) or "received by the user [from our server]" (response cookies)
And the same for "Cookies Created" -> "created by the user and sent to us" ... "or created by the server and sent to the client" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated :)
ro0NL commentedMar 22, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yeah im looking at it. Problem is asserting collected response cookies, due the var cloning thingy. It now happens at root level (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php#L145) making it nearly impossible to mock. For now i added only an additionalinstance of assertion, basically what we do for other public methods. |
javiereguiluz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 I left a minor comment about a possible rewording ... but the rest looks OK to me. Thanks@ro0NL
fabpot commentedMar 22, 2017
Thank you@ro0NL. |
This PR was merged into the 3.3-dev branch.Discussion----------[WebProfilerBundle] Improved cookie traffic| Q | A| ------------- | ---| Branch? | "master"| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | comma-separated list of tickets fixed by the PR, if any| License | MIT| Doc PR | reference to the documentation PR, if anyRelates to#20569 in terms of getting _all_ the cookies.Commits-------171c6d1 [WebProfilerBundle] Improved cookie traffic
Uh oh!
There was an error while loading.Please reload this page.
Relates to#20569 in terms of gettingall the cookies.