[WebProfilerBundle] Improved cookie traffic #20567

ro0NL · 2016-11-19T13:01:34Z

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 any

Relates to #20569 in terms of getting all the cookies.

ogizanagi · 2016-11-19T13:19:58Z

Shouldn't the header bag part (the bug fix) be in a separated PR targeting older branches ?

ro0NL · 2016-11-19T13:27:42Z

And of course this is one of those files that is different in all branches (bag.html.twig), making it hard to merge this approach upstream (as we would need to modify headerbag.html.twig as well)...

Ie. the bug is fixed by a new feature :)

HeahDude · 2016-11-19T14:31:22Z

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

+
+{% block body %}
+    {% for key in bag.keys|sort %}
+        {% for value in bag.get(key, [], false) %}


On the request panel, what about keeping one line by header but exposing the value as an array if needed instead?

Considered it :) however what i like about this, it shows headers more or less the same as the HTTP message itself.

This goes for all HeaderBag's btw, as we cannot call bag.get(key, [], false) on a ParamaterBag (hence the new template file).

nicolas-grekas · 2016-11-22T14:47:02Z

Displaying multiple HTTP headers should be fixed in #20595.

ro0NL · 2016-11-22T15:14:35Z

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 ?

ro0NL · 2016-12-17T20:05:22Z

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

-            $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;


@fabpot now #20569 is merged, we should not forget to update this.. i kinda did ;-) and it will cause duplicate headers i guess.

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#...  Forgotten in #20569 and makes #20567 truly a feature only. Before ![image](https://cloud.githubusercontent.com/assets/1047696/21361738/971e4f8e-c6e5-11e6-8b08-8a5f8ab4f1f0.png) After ![image](https://cloud.githubusercontent.com/assets/1047696/21361752/a6dc5380-c6e5-11e6-9f18-3bc3d7db94be.png) Commits ------- e1616b3 [HttpKernel] Continuation of #20569

ro0NL · 2016-12-22T11:05:16Z

@javiereguiluz now all the cookie hassle is fixed.. WDYT? (bug label can be removed)

fabpot · 2017-03-01T15:55:06Z

@javiereguiluz Can you review this PR? Is it ok for you?

fabpot · 2017-03-22T20:08:22Z

@ro0NL Can this one be merged?

ro0NL · 2017-03-22T20:18:25Z

@fabpot for me yes :) rebased and updated. Waiting for travis.

fabpot · 2017-03-22T20:34:52Z

@ro0NL Apparently, that breaks the tests.

javiereguiluz · 2017-03-22T20:40:24Z

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

+            <h3 class="tab-title">Cookies</h3>
+
+            <div class="tab-content">
+                <h3>Cookies Received</h3>


Just asking: would the following changes make this easier to understand?

Instead of Cookies Received -> Request Cookies and
instead of Cookies Created -> Response Cookies

No strong opinion.. i think i've tried to avoid those terms because of the Request | 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...

I think @javiereguiluz is right.

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

ro0NL · 2017-03-22T20:41:14Z

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 additional instance of assertion, basically what we do for other public methods.

javiereguiluz

👍 I left a minor comment about a possible rewording ... but the rest looks OK to me. Thanks @ro0NL

fabpot · 2017-03-22T21:19:56Z

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 any ![image](https://cloud.githubusercontent.com/assets/1047696/20455635/a033a814-ae60-11e6-8500-e60146f4619e.png) Relates to #20569 in terms of getting _all_ the cookies. Commits ------- 171c6d1 [WebProfilerBundle] Improved cookie traffic

carsonbot added Status: Needs Review WebProfilerBundle Bug Feature labels Nov 19, 2016

HeahDude reviewed Nov 19, 2016

View reviewed changes

This was referenced Nov 22, 2016

[WebProfilerBundle] Fix deprecated uses of profiler_dump #20595

Merged

[WebProfilerBundle] Display multiple HTTP headers in WDT #20599

Merged

ro0NL changed the title ~~[WebProfilerBundle] Display multiple HTTP headers + improved cookie traffic~~ [WebProfilerBundle] Improved cookie traffic Nov 22, 2016

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

ro0NL commented Dec 17, 2016

View reviewed changes

ro0NL mentioned this pull request Dec 20, 2016

[HttpKernel] Continuation of #20569 #21001

Merged

javiereguiluz removed the Bug label Dec 22, 2016

javiereguiluz self-assigned this Mar 1, 2017

javiereguiluz reviewed Mar 22, 2017

View reviewed changes

javiereguiluz approved these changes Mar 22, 2017

View reviewed changes

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Mar 22, 2017

[WebProfilerBundle] Improved cookie traffic

171c6d1

fabpot merged commit 171c6d1 into symfony:master Mar 22, 2017

ro0NL deleted the wdt/cookies branch March 22, 2017 21:37

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebProfilerBundle] Improved cookie traffic #20567

[WebProfilerBundle] Improved cookie traffic #20567

ro0NL commented Nov 19, 2016 •

edited

Loading

ogizanagi commented Nov 19, 2016

ro0NL commented Nov 19, 2016 •

edited

Loading

HeahDude Nov 19, 2016

ro0NL Nov 19, 2016

nicolas-grekas commented Nov 22, 2016

ro0NL commented Nov 22, 2016

ro0NL Dec 17, 2016

ro0NL Dec 18, 2016 •

edited

Loading

ro0NL commented Dec 22, 2016

fabpot commented Mar 1, 2017

fabpot commented Mar 22, 2017

ro0NL commented Mar 22, 2017 •

edited

Loading

fabpot commented Mar 22, 2017

javiereguiluz Mar 22, 2017

ro0NL Mar 22, 2017 •

edited

Loading

fabpot Mar 22, 2017

javiereguiluz Mar 22, 2017

ro0NL Mar 22, 2017

ro0NL commented Mar 22, 2017 •

edited

Loading

javiereguiluz left a comment

fabpot commented Mar 22, 2017

[WebProfilerBundle] Improved cookie traffic #20567

[WebProfilerBundle] Improved cookie traffic #20567

Conversation

ro0NL commented Nov 19, 2016 • edited Loading

ogizanagi commented Nov 19, 2016

ro0NL commented Nov 19, 2016 • edited Loading

HeahDude Nov 19, 2016

Choose a reason for hiding this comment

ro0NL Nov 19, 2016

Choose a reason for hiding this comment

nicolas-grekas commented Nov 22, 2016

ro0NL commented Nov 22, 2016

ro0NL Dec 17, 2016

Choose a reason for hiding this comment

ro0NL Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

ro0NL commented Dec 22, 2016

fabpot commented Mar 1, 2017

fabpot commented Mar 22, 2017

ro0NL commented Mar 22, 2017 • edited Loading

fabpot commented Mar 22, 2017

javiereguiluz Mar 22, 2017

Choose a reason for hiding this comment

ro0NL Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

fabpot Mar 22, 2017

Choose a reason for hiding this comment

javiereguiluz Mar 22, 2017

Choose a reason for hiding this comment

ro0NL Mar 22, 2017

Choose a reason for hiding this comment

ro0NL commented Mar 22, 2017 • edited Loading

javiereguiluz left a comment

Choose a reason for hiding this comment

fabpot commented Mar 22, 2017

ro0NL commented Nov 19, 2016 •

edited

Loading

ro0NL commented Nov 19, 2016 •

edited

Loading

ro0NL Dec 18, 2016 •

edited

Loading

ro0NL commented Mar 22, 2017 •

edited

Loading

ro0NL Mar 22, 2017 •

edited

Loading

ro0NL commented Mar 22, 2017 •

edited

Loading