[WebProfilerBundle] Fix deprecated uses of profiler_dump #20595

nicolas-grekas · 2016-11-22T14:46:28Z

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.

stof · 2016-11-22T14:53:26Z

src/Symfony/Bundle/WebProfilerBundle/Controller/RouterController.php

@@ -99,4 +103,26 @@ private function getTraces(RequestDataCollector $request, $method)

        return $matcher->getTracesForRequest($traceRequest);
    }
+
+    /**
+     * Replaces scalars wrapped as Data instances by their actual values and removes other values.


why would it remove other values ?

Because keeping the Data instances (there can only be such things here really) can only trigger bad behaviors, so removing them looks more safe. We just need to resolve the values to allow the url matcher do to its work. Even if it could match on weird attributes or on array cookies, let's not care about that to keep the code effective.

but you also get rid of cases where the Data instance contains a non-scalar value. this is wrong in several cases:

when it wraps null (which is not a scalar)

when it wraps an array (and yes, having nested arrays in request attributes is supported today. It would be even worse for GET and POST but they are not filtered here)

In the current case, this Request object is used by a TracableUrlMatcher.
Do you really think we need to write code to deal with routes that have constraints on these types? Do you think anyone really did such a thing?

In fact, we'd better serialize the original request in the RequestDataCollector, don't you think?

Updated: raw data are now serialized to create the request as before

stof · 2016-11-22T14:53:36Z

src/Symfony/Bundle/WebProfilerBundle/Controller/RouterController.php

+        foreach ($array as $key => $data) {
+            if (!$data instanceof Data) {
+                continue;
+            } elseif (is_scalar($value = $data->getRawData()[0][0])) {


should be a simple if

stof · 2016-11-22T14:54:57Z

src/Symfony/Bundle/WebProfilerBundle/Controller/RouterController.php

+            } elseif (is_scalar($value->value)) {
+                $array[$key] = $value->value;
+            } else {
+                unset($array[$key]);


does altering the array during the iteration work fine in all PHP versions ? I think there was some weird cases here in some PHP versions (but I don't remember exactly which cases were listed in the PHP RFC talking about changing them in PHP 7)

I think it's ok, but anyway, I updated the code to populate a new $result var.
Other comments addressed also.

stof · 2016-11-22T14:55:22Z

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

@@ -183,7 +183,7 @@
            <div class="tab-content">
                <h3>Response Headers</h3>

-                {{ include('@WebProfiler/Profiler/bag.html.twig', { bag: collector.responseheaders, labels: ['Header', 'Value'] }, with_context = false) }}
+                {{ include('@WebProfiler/Profiler/bag.html.twig', { bag: collector.responseheaders, labels: ['Header', 'Value'], asHeaders: 1 }, with_context = false) }}


please use true rather than 1

stof · 2016-11-22T14:56:00Z

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

@@ -9,7 +9,17 @@
        {% for key in bag.keys|sort %}
            <tr>
                <th>{{ key }}</th>
-                <td>{{ profiler_dump(bag.get(key), maxDepth=maxDepth|default(0)) }}</td>
+                <td>
+                {% if asHeaders|default(false) %}


I would name it as_headers, to match the Twig naming conventions

stof · 2016-11-22T14:59:29Z

currently, the ProfilerController forces to disable the profiler on its pages. Should we add a configuration setting to allow keeping the profiler enabled in the WebProfilerBundle pages for people working on it ? This may be an idea

nicolas-grekas · 2016-11-22T15:18:11Z

configuration setting to allow keeping the profiler enabled in the WebProfilerBundle

Maybe, but I fear people will just miss the configuration option most of the time and fall in the same trap. Another idea would be to fail hard on any log message triggered above some threshold while rendering a profiler panel. Or something else, like extra special care.
For another PR of course if anyone wants to work on that.

stof · 2016-11-22T15:25:02Z

@nicolas-grekas being able to re-enable the profiler while working on it would not solve everything magically, but it would make it easier to solve them when taking care, and with an easy change.

ro0NL · 2016-11-22T17:25:44Z

@nicolas-grekas with this approach can the multiple headers bug target 2.7? - #20567 (comment)

nicolas-grekas · 2016-11-22T17:31:28Z

I don't know. The panel is so different in 2.7 that I'll let you look at it if you'd like... It's up to you, personally I'm fine with 2.7 asis :)

ro0NL · 2016-11-22T17:44:05Z

True. However for symfony being a HTTP framework and such.. i find it quite disturbing though :)) and we're still stuck with it for some while. Then again, it's not really security related or so..

I think ideally the bug is fixed separate from this PR, targeting 2.7+. Since this PR only fixes the profiler_dump issue on it right?

Ie. we should at least consider 2.8 and/or 3.1 on it right?

nicolas-grekas · 2016-11-22T17:48:34Z

This PR applies only on 3.2 yes. The rest is up to you :)

ro0NL

LGTM 👍

ro0NL · 2016-11-22T17:46:51Z

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

@@ -125,6 +119,12 @@ public function collect(Request $request, Response $response, \Exception $except
            $this->data['request_request']['_password'] = '******';
        }

+        foreach ($this->data as $key => $value) {
+            if (is_array($value) && !in_array($key, array('request_server', 'request_attributes', 'request_cookies'))) {
+                $this->data[$key] = array_map(array($this, 'cloneVar'), $value);


We could consider adding cloneArray/Iterable by now ;-)

ro0NL · 2016-11-22T18:02:10Z

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

+                <td>
+                {% if as_headers|default(false) %}
+                    {% if 1 == bag.get(key).rawData[1]|length %}
+                        {{ profiler_dump(bag.get(key).seek(0), maxDepth=maxDepth|default(0)) }}


Ill try this approach on <3.2 👍

ro0NL · 2016-11-22T18:03:14Z

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

@@ -170,27 +170,27 @@ public function getRequestQuery()

    public function getRequestHeaders()
    {
-        return new HeaderBag($this->data['request_headers']);
+        return new ParameterBag($this->data['request_headers']);


We are sure this breaks nothing right? In terms of possible unknown method calls by now (see also getResponseHeaders).

As decided in previous PRs about the profiler internals, we don't want to preserve bc here, that would slow us too much.

I was more thinking about references in core templates ;) but i guess were fine.

fabpot · 2016-11-24T00:19:02Z

Thank you @nicolas-grekas.

…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

@nicolas-grekas

…ro0NL) This PR was merged into the 2.7 branch. Discussion ---------- [WebProfilerBundle] Display multiple HTTP headers in WDT | Q | A | ------------- | --- | Branch? | 2.7 (partially in 3.2 already) | Bug fix? | yes | New feature? | no | BC breaks? | no-ish | Deprecations? | no | Tests pass? | yes | Fixed tickets | [#20595 (comment)](#20595 (comment)) | License | MIT | Doc PR | reference to the documentation PR, if any Backport of #20595 for 2.7, that includes a fix for displaying multiple HTTP headers in the WDT. ![image](https://cloud.githubusercontent.com/assets/1047696/20536902/43de2692-b0eb-11e6-8eb7-f0584daf5963.png) //cc @nicolas-grekas this should cover 2.7 - 3.1 right? Commits ------- 257a856 [WebProfilerBundle] Display multiple HTTP headers in WDT

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

This was referenced Nov 22, 2016

[WebProfilerBundle] Improved cookie traffic #20567

Merged

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

Closed

nicolas-grekas force-pushed the fix-profiler branch from e0c5b8a to faf8b34 Compare November 22, 2016 14:49

stof reviewed Nov 22, 2016

View reviewed changes

nicolas-grekas force-pushed the fix-profiler branch 3 times, most recently from a3545e9 to 91d8f81 Compare November 22, 2016 15:11

nicolas-grekas force-pushed the fix-profiler branch from 91d8f81 to 79b9f2b Compare November 22, 2016 15:18

nicolas-grekas added the Ready label Nov 22, 2016

nicolas-grekas force-pushed the fix-profiler branch 2 times, most recently from 3a58412 to 832a10e Compare November 22, 2016 17:15

ro0NL reviewed Nov 22, 2016

View reviewed changes

ro0NL mentioned this pull request Nov 22, 2016

[WebProfilerBundle] Display multiple HTTP headers in WDT #20599

Merged

nicolas-grekas force-pushed the fix-profiler branch 2 times, most recently from a38bf3d to 0cfb176 Compare November 22, 2016 19:19

[WebProfilerBundle] Fix deprecated uses of profiler_dump

b4c327d

nicolas-grekas force-pushed the fix-profiler branch from 0cfb176 to b4c327d Compare November 22, 2016 19:22

nicolas-grekas added this to the 3.2 milestone Nov 23, 2016

fabpot merged commit b4c327d into symfony:3.2 Nov 24, 2016

nicolas-grekas deleted the fix-profiler branch November 24, 2016 09:34

fabpot mentioned this pull request Nov 27, 2016

Release v3.2.0-RC2 #20649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebProfilerBundle] Fix deprecated uses of profiler_dump #20595

[WebProfilerBundle] Fix deprecated uses of profiler_dump #20595

nicolas-grekas commented Nov 22, 2016

stof Nov 22, 2016

nicolas-grekas Nov 22, 2016

stof Nov 22, 2016

nicolas-grekas Nov 22, 2016 •

edited

Loading

nicolas-grekas Nov 22, 2016 •

edited

Loading

nicolas-grekas Nov 22, 2016

stof Nov 22, 2016

stof Nov 22, 2016

nicolas-grekas Nov 22, 2016

stof Nov 22, 2016

stof Nov 22, 2016

stof commented Nov 22, 2016

nicolas-grekas commented Nov 22, 2016 •

edited

Loading

stof commented Nov 22, 2016

ro0NL commented Nov 22, 2016 •

edited

Loading

nicolas-grekas commented Nov 22, 2016

ro0NL commented Nov 22, 2016 •

edited

Loading

nicolas-grekas commented Nov 22, 2016

ro0NL left a comment

ro0NL Nov 22, 2016

ro0NL Nov 22, 2016

ro0NL Nov 22, 2016

nicolas-grekas Nov 22, 2016

ro0NL Nov 22, 2016

fabpot commented Nov 24, 2016

[WebProfilerBundle] Fix deprecated uses of profiler_dump #20595

[WebProfilerBundle] Fix deprecated uses of profiler_dump #20595

Conversation

nicolas-grekas commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Nov 22, 2016

nicolas-grekas commented Nov 22, 2016 • edited Loading

stof commented Nov 22, 2016

ro0NL commented Nov 22, 2016 • edited Loading

nicolas-grekas commented Nov 22, 2016

ro0NL commented Nov 22, 2016 • edited Loading

nicolas-grekas commented Nov 22, 2016

ro0NL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Nov 24, 2016

nicolas-grekas Nov 22, 2016 •

edited

Loading

nicolas-grekas Nov 22, 2016 •

edited

Loading

nicolas-grekas commented Nov 22, 2016 •

edited

Loading

ro0NL commented Nov 22, 2016 •

edited

Loading

ro0NL commented Nov 22, 2016 •

edited

Loading