[WebProfilerBundle] Added filters to search results #14110

rvanlaak · 2015-03-30T12:50:48Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Quite often I find myself copy/pasting the results from the search results in the profiler search form. Adding links for this allows developers to narrow down to the proper profiler result fast.

fabpot · 2015-03-30T15:36:07Z

Can you add a screenshot here?

rvanlaak · 2015-03-31T07:10:45Z

xabbuh · 2015-04-01T19:11:04Z

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

-                        <td>{{ elements.url }}</td>
+                        <td>
+                            {{ elements.ip }}
+                            <a href="{{ path('_profiler_search_results', app.request.query.all|merge({'ip': elements.ip, 'token': elements.token})) }}">


Does it make sense to include the token here? Wouldn't you usually want to filter just by the IP address (same below for the two other properties)?

Without that no search results will be shown at all. The filtering does not happen on the token, but the token is part of search result URI:

https://symfony.dev/_profiler/01d9c6/search/results?ip=&limit=10&url=https%3A//symfony.dev/filter-link

Indeed, you're right.

xabbuh · 2015-04-01T19:20:07Z

👍 looks nice

Tobion · 2015-04-01T19:26:23Z

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

+                        <td>
+                            {{ elements.method }}
+                            <a href="{{ path('_profiler_search_results', app.request.query.all|merge({'method': elements.method, 'token': elements.token})) }}">
+                                <img style="margin: 0 5px 0 0; vertical-align: middle;" width="16" height="16" alt="Search" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAC2ElEQVR42u3XvUtbYRQG8JcggSxSiqlQoXUQUfEDnDoIEkK30ulKh0REFEOkitaIaBUU4gchQ8BBGyKGIC79B7rVxaGlm+JooYtQCq2U0oq9OX0eOBeCRXrf69DFwI9z73nPeTlJbrxXIyL/1e0AfyWueTVAEgrwGt5qLGge675e1gPUQaqxsfEgmUyerq6uft3e3v61v78vjDxnnuusYz3WTI0bDXAnHA6/Gh0d/bS7u+vu7e3JdbjOOtazDzmjAg9QF41Gy+vr6+eVSkX8Yj372I9zA8EGiEQi6bW1tfNyuSyK7/II0YEmMBodzYuHfezXmkADNAwNDX3c2dkRKpVKl4hZCIO5SvNZ1LleD/uxzz0c2w/Q0tLyAheYWywWRT0H4wPrhNjf1taWYd56gOHh4XdbW1tC+Xz+CNH4pfVCuo/9AAsLC182NzeFVlZWUojGL60X0n3sB8BFdFEoFISWlpYeIhq/tF5I97EfIJfLufgohZqbm+8jGr+0Xkj3sR9geXn5x8bGhlBHR8czROOX1gvpPvYDzM3NnWSzWaFYLPYG0fil9UK6j/0As7OzWVxMQul0+ht6nuDY/AvrWO/16j7WA/BCerC4uHiJKNTX13eid7wQzs1VzHOddV4P+n9zHwj0l5BfQ35+fl4Ix248Hj8NhUIlLPXDXeQNI8+Z5zrrvJ6BgYEzrMVxHGgAfg3hmZmZD4jiwd3ue3d393F9ff0hnwcYec4812tlMplqb2/vMepigW/H09PTUXgPEsTU1FS1p6dHhwj4QDI5ORmBHFyAXEfXK+DW5icmJqpdXV21Q9g/ko2Pj1MTvIQDOAPReKD5Jq1zwAVR/CVVOzs7OUR/oAFSqZQtB1wQz9jYWLW9vf0I2z2yHgAXWRAOuCCekZGRamtr66HtALw9B+WAC+JJJBI/rQfA081NOOCCEJ6gP1sPMDg4eFNP4Uw9vv3X7HaAq/4AszA5PJFqlwgAAAAASUVORK5CYII=">


We switched to inline svg for the icons. Should be used here as well.

Totally fine with me, but copied the search icon directly from the search form. Can you point me to the place where the inline svg icons can be found? The search icon at the right top of the profiler, left of the 'search on symfony website' form also still is a png icon.

then we dont have an svg for it yet i guess

Made one based on the current png, also did put it in the search form itself

rvanlaak · 2015-04-02T07:39:55Z

Made a svg of the search icon, and refactored it to its own inline css class.

Let me know when I'd rebase the commits.

stof · 2015-04-02T10:29:46Z

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

-            $end = $session->get('_profiler_search_end');
-            $limit = $session->get('_profiler_search_limit');
-            $token = $session->get('_profiler_search_token');
+            $ip = $request->get('ip', $session->get('_profiler_search_ip'));


please use $request->query->get(). There is no need to read it from the POST params or the attributes as it can only be in the query string (same for others).

And using the query string should also work when the session is disabled (a few lines above).
thus, we should avoid reading from the session if we have the query string param IMO instead of always reading the session value just to compute the default value for the next call (methods calls are not lazy in PHP)

dont use request->get, see phpdoc

exactly (even though the implementation has been optimized a but to avoid relying on chaining defaults, making it OK when you actually need to allow the 3 sources)

fabpot · 2015-10-05T14:22:37Z

👍 ping @symfony/deciders

rvanlaak · 2015-10-05T14:37:53Z

Don't think this PR still is valid on the profiler redesign?

fabpot · 2015-10-05T14:40:12Z

I do :)

rvanlaak · 2015-10-05T14:46:13Z

Let me at least squash all commits first then 👍

fabpot · 2015-10-05T14:54:51Z

As this PR is based on 2.7, you will have to submit a new one based on the 2.8 branch.

javiereguiluz · 2015-10-05T14:59:49Z

@rvanlaak I like this proposal. The only thing I don't fully like is the search icon. What if we use regular links like in the token link? If you think that it looks ugly because there are too many links, we could apply text-decoration: none by default and show the underline only when you :hover a table row.

rvanlaak · 2015-10-05T15:06:07Z

I can understand you don't like the search icon @javiereguiluz . But, is making it a link the right solution? What about showing an actual "filter" icon, and only showing it on td:hover?

Something like http://fontawesome.io/icon/filter/

xabbuh · 2015-10-05T17:50:36Z

Looks like something went wrong during the rebase.

rvanlaak · 2015-10-06T08:29:20Z

Was still busy rebasing, completed now. Did add the icon on tr:hover to make visible what fields can get filtered.

fabpot · 2015-10-06T15:42:57Z

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

@@ -53,8 +53,8 @@
                            </li>
                        </ul>
                    {% endif %}
-                    {{ render(path('_profiler_search_bar')) }}
-                    {% include '@WebProfiler/Profiler/admin.html.twig' with { 'token': token } only %}
+                    {{ render(path('_profiler_search_bar', app.request.query.all)) }}


We never use app in the profiler as it's not always available (in Silex for instance). request should be available (at least that's the case in 2.8 where we are going to merge this PR).. which means that you should probably close this PR and open a new one on the 2.8 branch.

By which you mean app.request can be request on every profiler page?

@fabpot for search results request is not available, should the controller pass it to the view?

stof · 2015-10-07T08:52:03Z

@rvanlaak please reopen this PR to the 2.8 branch as the profiler is totally redesigned in 2.8. We won't merge this in 2.7, and not having the diff with the right code makes the review painful

rvanlaak · 2015-10-07T08:57:10Z

So the target only is 2.8 and not 2.7, that wasn't clear to me up till now since the PR was from 30 March.

fabpot · 2015-10-07T13:01:44Z

Yes, the PR should be based on the 2.8 branch as this is where it's going to be merged but more importantly because the profiler is quite different in 2.8. Thanks.

fabpot · 2015-10-19T12:16:14Z

@rvanlaak Closing this PR. Can you reopen a new one on 2.8? Thanks.

rvanlaak · 2015-10-26T09:45:38Z

Done 👍 @fabpot see #16344

…anlaak) This PR was merged into the 2.8 branch. Discussion ---------- [WebProfilerBundle] Filter links in search results | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14110 | License | MIT | Doc PR | Quite often I find myself copy/pasting the results from the search results in the profiler search form. Adding links for this allows developers to narrow down to the proper profiler result fast. Commits ------- 4bffacc [WebProfilerBundle] Filter links in search results

…lts (Rvanlaak) This PR was merged into the 2.8 branch. Discussion ---------- [WebProfilerBundle] Filter links in search results | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#14110 | License | MIT | Doc PR | Quite often I find myself copy/pasting the results from the search results in the profiler search form. Adding links for this allows developers to narrow down to the proper profiler result fast. Commits ------- 4bffacc [WebProfilerBundle] Filter links in search results

aitboudad added the WebProfilerBundle label Mar 30, 2015

xabbuh reviewed Apr 1, 2015
View reviewed changes

Tobion reviewed Apr 1, 2015
View reviewed changes

stof reviewed Apr 2, 2015
View reviewed changes

rvanlaak force-pushed the 2.7 branch 2 times, most recently from b0b49ac to 31192c3 Compare October 6, 2015 08:27

fabpot reviewed Oct 6, 2015
View reviewed changes

rvanlaak force-pushed the 2.7 branch from 31192c3 to 8bdb491 Compare October 7, 2015 08:49

[WebProfilerBundle] Filter links in search results

d5e29e6

rvanlaak force-pushed the 2.7 branch from 8bdb491 to d5e29e6 Compare October 7, 2015 08:50

fabpot closed this Oct 19, 2015

rvanlaak mentioned this pull request Oct 26, 2015

[WebProfilerBundle] Filter links in search results #16344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebProfilerBundle] Added filters to search results #14110

[WebProfilerBundle] Added filters to search results #14110

rvanlaak commented Mar 30, 2015

fabpot commented Mar 30, 2015

rvanlaak commented Mar 31, 2015

xabbuh Apr 1, 2015

rvanlaak Apr 1, 2015

xabbuh Apr 1, 2015

xabbuh commented Apr 1, 2015

Tobion Apr 1, 2015

rvanlaak Apr 1, 2015

Tobion Apr 1, 2015

rvanlaak Apr 1, 2015

rvanlaak commented Apr 2, 2015

stof Apr 2, 2015

Tobion Apr 2, 2015

stof Apr 2, 2015

fabpot commented Oct 5, 2015

rvanlaak commented Oct 5, 2015

fabpot commented Oct 5, 2015

rvanlaak commented Oct 5, 2015

fabpot commented Oct 5, 2015

javiereguiluz commented Oct 5, 2015

rvanlaak commented Oct 5, 2015

xabbuh commented Oct 5, 2015

rvanlaak commented Oct 6, 2015

fabpot Oct 6, 2015

rvanlaak Oct 6, 2015

rvanlaak Oct 7, 2015

stof commented Oct 7, 2015

rvanlaak commented Oct 7, 2015

fabpot commented Oct 7, 2015

fabpot commented Oct 19, 2015

rvanlaak commented Oct 26, 2015

[WebProfilerBundle] Added filters to search results #14110

[WebProfilerBundle] Added filters to search results #14110

Conversation

rvanlaak commented Mar 30, 2015

fabpot commented Mar 30, 2015

rvanlaak commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Apr 1, 2015

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

rvanlaak commented Apr 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 5, 2015

rvanlaak commented Oct 5, 2015

fabpot commented Oct 5, 2015

rvanlaak commented Oct 5, 2015

fabpot commented Oct 5, 2015

javiereguiluz commented Oct 5, 2015

rvanlaak commented Oct 5, 2015

xabbuh commented Oct 5, 2015

rvanlaak commented Oct 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Oct 7, 2015

rvanlaak commented Oct 7, 2015

fabpot commented Oct 7, 2015

fabpot commented Oct 19, 2015

rvanlaak commented Oct 26, 2015