[3.2][HttpKernel] Fix rendering route params in WDT #20571

ro0NL · 2016-11-20T14:19:49Z

Q	A
Branch?	3.2
Bug fix?	yes
New feature?	no
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

The route parameters are not rendered in 3.2 in the WDT (routing panel) as is empty in twig fails on a Data object. This preserves the array type.

With or without params.

See also https://github.com/symfony/symfony/pull/19614/files#diff-e8f5b14fbfbbeac60fc9f3abe310c3b0L59

javiereguiluz · 2016-11-20T16:25:36Z

👍 I can confirm this error in 3.2.0-rc1 ... and I can also confirm that the proposed change fixes the error.

Thanks @ro0NL!!

wouterj · 2016-11-20T16:59:55Z

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

@@ -63,6 +64,10 @@ public function collect(Request $request, Response $response, \Exception $except
            if ('_route' === $key) {
                $route = is_object($value) ? $value->getPath() : $value;
            }
+
+            if ('_route_params' === $key && is_array($value)) {
+                $routeParams = $value;


shouldn't we clone $value here to get a nicer output?

Well.. that's what broke it before :)) The value is passed to table.html.twig (if is empty passes) which uses profiler_dump already.

We could wrap the array in a ParameterBag (seems consistent..) and use bag.html.twig, but it's the same result and probably a BC break for getRouteParams.

I agree with @wouterj , here is the patch (fixing session data display meanwhile):

diff --git a/src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php b/src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php index f0c18c5..7bcd77a 100644 --- a/src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php +++ b/src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php @@ -56,6 +56,10 @@ class RequestDataCollector extends DataCollector implements EventSubscriberInter foreach ($request->attributes->all() as $key => $value) { if ('_route' === $key && is_object($value)) { $attributes[$key] = $this->cloneVar($value->getPath()); + } elseif ('_route_params' === $key) { + foreach ($value as $k => $v) { + $attributes[$key][$k] = $this->cloneVar($v); + } } else { $attributes[$key] = $this->cloneVar($value); } @@ -83,7 +87,9 @@ class RequestDataCollector extends DataCollector implements EventSubscriberInter $sessionMetadata['Created'] = date(DATE_RFC822, $session->getMetadataBag()->getCreated()); $sessionMetadata['Last used'] = date(DATE_RFC822, $session->getMetadataBag()->getLastUsed()); $sessionMetadata['Lifetime'] = $session->getMetadataBag()->getLifetime(); - $sessionAttributes = $session->all(); + foreach ($session->all() as $k => $v) { + $sessionAttributes[$k] = $this->cloneVar($v); + } $flashes = $session->getFlashBag()->peekAll(); } } @@ -264,7 +270,7 @@ class RequestDataCollector extends DataCollector implements EventSubscriberInter */ public function getRouteParams() { - return isset($this->data['request_attributes']['_route_params']) ? $this->data['request_attributes']['_route_params'] : $this->cloneVar(array()); + return isset($this->data['request_attributes']['_route_params']) ? $this->data['request_attributes']['_route_params'] : array(); } /** diff --git a/src/Symfony/Component/HttpKernel/Tests/DataCollector/RequestDataCollectorTest.php b/src/Symfony/Component/HttpKernel/Tests/DataCollector/RequestDataCollectorTest.php index 2ae1c5c..be24ea9 100644 --- a/src/Symfony/Component/HttpKernel/Tests/DataCollector/RequestDataCollectorTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/DataCollector/RequestDataCollectorTest.php @@ -47,7 +47,7 @@ class RequestDataCollectorTest extends \PHPUnit_Framework_TestCase $this->assertInstanceOf('Symfony\Component\HttpFoundation\ParameterBag', $c->getRequestQuery()); $this->assertSame('html', $c->getFormat()); $this->assertEquals('foobar', $c->getRoute()); - $this->assertEquals($cloner->cloneVar(array('name' => 'foo')), $c->getRouteParams()); + $this->assertEquals(array('name' => $cloner->cloneVar('foo')), $c->getRouteParams()); $this->assertSame(array(), $c->getSessionAttributes()); $this->assertSame('en', $c->getLocale()); $this->assertEquals($cloner->cloneVar($request->attributes->get('resource')), $attributes->get('resource'));

@nicolas-grekas now the request attributes break 😭

Not sure we can support this with only 1 structural value..

@ro0NL in your screenshot the Appbundle... string appears cut out. If this is because of VarDumper, please configure it to never trim short strings. In #20380 we did some changes related to this.

Last commit fixes both request attributes as route attributes, in terms of looking great.

@javiereguiluz ill have a look.

Hmz not sure where to begin or how that affects this PR change. Maybe @nicolas-grekas can give a heads up.

give me a few minutes, patch coming :)

nicolas-grekas · 2016-11-21T19:38:24Z

@javiereguiluz the ellipsis on AppBundle is not specific to this screenshot, and depends on the actual font in use I'd say.

nicolas-grekas · 2016-11-21T19:40:06Z

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

@@ -83,6 +90,9 @@ public function collect(Request $request, Response $response, \Exception $except
                $sessionMetadata['Created'] = date(DATE_RFC822, $session->getMetadataBag()->getCreated());
                $sessionMetadata['Last used'] = date(DATE_RFC822, $session->getMetadataBag()->getLastUsed());
                $sessionMetadata['Lifetime'] = $session->getMetadataBag()->getLifetime();
+                foreach ($sessionAttributes as $key => $value) {


this should be foreach ($session->all() as $key => $value) {
right now, $sessionAttributes is always empty, isn't it?

nicolas-grekas · 2016-11-21T20:07:43Z

Looks like appveyor doesn't like the patch :(

ro0NL · 2016-11-21T20:36:42Z

Hmz it seems to fail due $sessionAttributes[$key] = $this->cloneVar($value);

C:\projects\symfony.phpunit\phpunit-4.8\vendor\symfony\phpunit-bridge\DeprecationErrorHandler.php:73
C:\projects\symfony\src\Symfony\Component\HttpKernel\DataCollector\DataCollector.php:133
C:\projects\symfony\src\Symfony\Component\HttpKernel\DataCollector\DataCollector.php:89
C:\projects\symfony\src\Symfony\Component\HttpKernel\DataCollector\RequestDataCollector.php:94

https://ci.appveyor.com/project/fabpot/symfony/build/1.0.14541#L1044

I can have a look tomorrow.. otherwise perhaps revert the session prettifying?

nicolas-grekas · 2016-11-21T21:08:20Z

calling profiler_dump with anything else than a Data object is deprecated. That's another reason why we need to "clone" session data.

nicolas-grekas · 2016-11-22T12:47:31Z

Which makes me figure out there are much more deprecations to fix. I'm on it.

nicolas-grekas · 2016-11-22T12:50:02Z

Side note: please mind your commit messages, especially the first one. Should match the PR title.

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

I ended up with a too big patch, so I created a new PR that should supersede this one: #20595

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

👍 closing in favor of #20595

Could cherry-pick the original commits first next time? Now my credits are gone :)) which i dont mind.. but still ;-)

…nicolas-grekas) This PR was merged into the 3.2 branch. Discussion ---------- [WebProfilerBundle] Fix deprecated uses of profiler_dump | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Replaces #20571 that made me realize that we completely missed updating the "Request / Response" panel that is current triggering a bunch of deprecation notices about profiler_dump. Yet, these notices triggered by the profiler are not displayed anywhere (what, we don't have a profiler for the profiler? :) ). And we missed them. Commits ------- b4c327d [WebProfilerBundle] Fix deprecated uses of profiler_dump

carsonbot added Status: Needs Review HttpKernel Bug labels Nov 20, 2016

fixed

b60091d

javiereguiluz added Status: Reviewed and removed Status: Needs Review labels Nov 20, 2016

javiereguiluz added this to the 3.2 milestone Nov 20, 2016

wouterj reviewed Nov 20, 2016

View reviewed changes

revised

68ea327

nicolas-grekas reviewed Nov 21, 2016

View reviewed changes

ro0NL added 2 commits November 21, 2016 20:47

oops

749837a

aargh

2ec98de

nicolas-grekas mentioned this pull request Nov 22, 2016

[WebProfilerBundle] Fix deprecated uses of profiler_dump #20595

Merged

fabpot closed this Nov 22, 2016

ro0NL deleted the wdt/route-params branch November 22, 2016 15:16

Uh oh!

[3.2][HttpKernel] Fix rendering route params in WDT #20571

[3.2][HttpKernel] Fix rendering route params in WDT #20571

Uh oh!

Conversation

ro0NL commented Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javiereguiluz commented Nov 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Nov 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Nov 21, 2016

Uh oh!

ro0NL commented Nov 21, 2016

Uh oh!

nicolas-grekas commented Nov 21, 2016

Uh oh!

nicolas-grekas commented Nov 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Nov 22, 2016

Uh oh!

nicolas-grekas commented Nov 22, 2016

Uh oh!

ro0NL commented Nov 22, 2016

Uh oh!

Uh oh!

ro0NL commented Nov 20, 2016 •

edited

Loading

ro0NL Nov 20, 2016 •

edited

Loading

nicolas-grekas Nov 21, 2016 •

edited

Loading

nicolas-grekas commented Nov 21, 2016 •

edited

Loading

nicolas-grekas commented Nov 22, 2016 •

edited

Loading