Slightly re-design the debugging toolbar. #3833

Closed
wants to merge 30 commits into from
@shiroyuki

The toolbar is very useful and containing lots of information. However, as there are too much information, it is very distracting and the toolbar area somehow ends up taking too much space.

The main purpose of this pull request is to hide any information and show only whenever the user wants to see.

Additionally, the CSS is partially re-organized when we embed the toolbar on each responded page.

pminnieur and others added some commits Mar 9, 2012
@pminnieur pminnieur use SecurityContextInterface instead of SecurityContext 0c9b2d4
@fabpot fabpot merged branch pminnieur/2.0 (PR #3537)
Commits
-------

0c9b2d4 use SecurityContextInterface instead of SecurityContext

Discussion
----------

[2.0][Security] use SecurityContextInterface instead of SecurityContext

see symfony/symfony#3522 (this is a fix for the 2.0 branch)

---------------------------------------------------------------------------

by pminnieur at 2012-03-21T13:25:59Z

*ping* it still missed the 2.0.12 release ...

---------------------------------------------------------------------------

by stof at 2012-03-21T16:41:28Z

@pminnieur you PR has been merged into master, not into 2.0, so it will only be in 2.1

---------------------------------------------------------------------------

by pminnieur at 2012-03-21T16:43:02Z

I know, and this is a second PR for 2.0 branch.
5ede111
@Seldaek Seldaek [FrameworkBundle] Add support for full URLs to redirect controller 86a3512
@fabpot fabpot merged branch Seldaek/redirect (PR #3693)
Commits
-------

86a3512 [FrameworkBundle] Add support for full URLs to redirect controller

Discussion
----------

[FrameworkBundle] Add support for full URLs to redirect controller

I'd consider this a bugfix since at the moment using an URL just redirects to `/current/pathhttp://example.org`.
42b8c7a
@jalliot jalliot Simplified CONTENT_ headers retrieval 15dd17e
@fabpot fabpot merged branch jalliot/header-bag (PR #3702)
Commits
-------

15dd17e Simplified CONTENT_ headers retrieval

Discussion
----------

Header bag

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes (actually no but I think this has nothing to do with this PR...)
Fixes the following tickets: -
Todo: -
51eac4b
@clemens-tolboom clemens-tolboom Fixed example code. e4f3fd9
@fabpot fabpot merged branch clemens-tolboom/2.0 (PR #3725)
Commits
-------

e4f3fd9 Fixed example code.

Discussion
----------

[Translation] example code did not work.

The example code for Translation did not compile. And the trans() result was void.
d12e398
@igorw igorw [DependencyInjection] Support Yaml calls without arguments 24a0d0a
@fabpot fabpot merged branch igorw/dic-yaml-without-args (PR #3747)
Commits
-------

24a0d0a [DependencyInjection] Support Yaml calls without arguments

Discussion
----------

[DependencyInjection] Support Yaml calls without arguments
a10fee1
Juti Noppornpitak Prepare for modifying toolbar. 032ab94
@ruimarinho ruimarinho [Security] [HttpDigest] Fixes a configuration error caused by an inva…
…lid key child node configuration
fd1ea69
@fabpot fabpot merged branch ruimarinho/http_digest (PR #3778)
Commits
-------

fd1ea69 [Security] [HttpDigest] Fixes a configuration error caused by an invalid key child node configuration

Discussion
----------

[Security] [HttpDigest] Fixes a configuration error caused by an invalid "key" child node configuration

```
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
```
eb6a26f
@fabpot fabpot Revert "merged branch ruimarinho/http_digest (PR #3778)"
This reverts commit eb6a26f57269d844cf9ef90150e3e0925d5e5bf3, reversing
changes made to a10fee16c193d93080c22528e58b1fd018341229.
e4ebffb
@ruimarinho ruimarinho [Security] [HttpDigest] Fixes a configuration error caused by an inva…
…lid 'key' child node configuration
fc41d4f
@hhamon hhamon [Routing] improved exception message when giving an invalid route name. f7647f9
@hhamon hhamon [Routing] fixed exception message. 04ae7cc
@Seldaek Seldaek [Console] Avoid outputing \r's in exception messages 97f7b29
@Seldaek Seldaek [Console] Wrap exception messages to the terminal width to avoid ugly…
… output
595cc11
@fabpot fabpot merged branch ruimarinho/http_digest (PR #3781)
Commits
-------

fc41d4f [Security] [HttpDigest] Fixes a configuration error caused by an invalid 'key' child node configuration

Discussion
----------

[Security] [HttpDigest] Fixes a configuration error caused by an invalid "key" child node configuration

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: ![Build status](https://secure.travis-ci.org/ruimarinho/symfony.png?branch=http_digest)
c140386
@fabpot fabpot merged branch Seldaek/console_ex_20 (PR #3802)
Commits
-------

595cc11 [Console] Wrap exception messages to the terminal width to avoid ugly output
97f7b29 [Console] Avoid outputing \r's in exception messages

Discussion
----------

[Console] Exception rendering fixes

This fixes two things:

- `\r`'s in exception messages were output (in case of `\r\n` newlines), creating really weird results on windows.
- long exception messages were wrapping and then the "red" block was completely messed up, with half black/half red lines, now it's wrapped before output if the terminal width can be detected.

If you don't care about merging this for 2.0, you can also merge the `console_ex` branch which applies on master. Due to moving tests and renaming of some normalize stuff in the tests, the two test patches are kind of different.

RFC: I am really not sure where to put those getTerminalWidth/Height methods. I guess this is not the best place.
f2398f6
@fabpot fabpot merged branch hhamon/route_collection_better_exception_message (PR #3…
…801)

Commits
-------

04ae7cc [Routing] fixed exception message.
f7647f9 [Routing] improved exception message when giving an invalid route name.

Discussion
----------

Route collection better exception message

Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
1387415
@shiroyuki shiroyuki Merge branch '2.0' of http://github.com/symfony/symfony into 2.0 cfd993b
@Seldaek Seldaek [Console] Mock terminal size to prevent formatting errors on small te…
…rminals
8a2b115
@Seldaek Seldaek [Console] Add docblocks 7ce22f0
@fabpot fabpot merged branch Seldaek/termwidth (PR #3805)
Commits
-------

7ce22f0 [Console] Add docblocks
8a2b115 [Console] Mock terminal size to prevent formatting errors on small terminals

Discussion
----------

[Console] Fixes terminal width in tests

This fixes the [tests that broke on travis-ci](http://travis-ci.org/#!/symfony/symfony/jobs/1031109) (which seems to advertise a terminal width of ~34, not sure why).
38e17c2
@shiroyuki shiroyuki Reverted the position. 6bd64b5
@shiroyuki shiroyuki Revamped the profiler toolbar in both embedded mode and normal. 420e00b
@shiroyuki shiroyuki Merge branch '2.0' of http://github.com/symfony/symfony into 2.0 3a202a2
@shiroyuki shiroyuki minor update to the toolbar. d6a2938
@cordoval

you should have rebased @shiroyuki , you are an expert in js telling by your page but here you will have to do things right with git to get this PR merged. I can help you for now just rebase or just rebase a clean branch and perhaps cherry pick your stuff.

@fabpot
Symfony member

It looks like you have worked on the 2.0 branch but submitted the PR on the master one but such changes should be done in the master branch anyway.

@dlsniper

@shiroyuki This http://symfony.com/doc/current/contributing/code/patches.html#working-on-a-patch and this might help you: http://symfony.com/doc/current/contributing/code/patches.html#id1

And like @fabpot said, you should do your changes against master branch as they are not bug fixes :)

@shiroyuki

My mistake... this PR should have been for 2.0. :P

Originally it is for 2.1 but until now, i still can't get 2.1 running on my environment. I can rework on this again from master and also clean up my commit too. What do you guys think? (This means this PR will be obsolete until the rework is done... shouldn't be long.)

In any case, I forgot to mention that the redesign is available at http://home.shiroyuki.com at the moment. Please let me know if you want me to change something.

@dlsniper

@shiroyuki It looks good. I do have to ask you if it's possible to make it so that the memory + execution time panel are visible by default? I mean those are the most important things I look for while using the toolbar.

Another cool thing that just crossed my mind would be if we could have the toolbar able to be minimized / maintain its state between requests. What do you guys think?

@fabpot
Symfony member

Great work!

Some feedback:

  • It should definitely be done on master, not 2.0
  • Instead of the P "logo", what about using the PHP logo (in black and white for instance)
  • I think we can keep some information in the toolbar (like execution time and response status code?)
  • Memory is not something that needs to be displayed by default as it is probably not something you can really do anything with
  • I'm not convinced about the opacity change
  • With this change, we can probably remove the verbose option as it does not make any sense now
@stloyd

Nice one, some additional feedback:

  • Opacity should be removed,
  • IMO you use to many <span>s (maybe you should try with lists / divs ?),
  • :hover on elements is glitching (<a> are not as big as toolbar),
  • -1 for using -*-transition in CSS it's not widely supported,
  • What about adding info to "tabs" on hover ? I.e.: Used memory: 10666 KB ?
@willdurand

Nice work.

I like the opacity feature.. In symfony 1.x we had the ability to reduce the debug bar without closing it but in Symfony2, you just can close it, that's why I think the opacity is a nice improvement.

@fabpot
Symfony member

Opacity is not needed as the toolbar is displayed after all the page content (which was not the case in symfony 1.x where the toolbar was displayed on top of the content).

@willdurand

I experienced some issues where I couldn't see the footers stuck to the bottom of pages.

@fabpot
Symfony member

@willdurand: It's a bug then.

@shiroyuki

As @willdurand mentioned, the opacity is to solve the problem the problem of the toolbar overlays the footer. The earlier attempt of mine was to use position: -30px or something like that to partially hide the toolbar. But I felt that it sort of defeated the purpose of the toolbar. And I also realized that the toolbar already has a hide button. So, I thought Opacity would be better in this case.

@dlsniper: maintaining the state of toolbar will probably duplicate the functionality of a browser's developer toolbar featuring in Firefox, Safari, Chrome and Opera.

Per @stloyd, span is what already there. I agree that there are too many span's used on the toolbar.

As I design this with progressive enhancement in mind, most of browsers' specific CSS properties used by the toolbar should have a fallback for incapable browsers.

Also, I like the idea that it could be a little bit more descriptive with this design.

I will try to incorporate everything else I didn't mention above :D

@stof
Symfony member

@fabpot it was the reason why the height of the block behind the toolbar was changed to 80px to allow the toobar to use 2 lines without hiding the content of the page. But you merged a revert a while ago because of people complaining about the extra space when the toobar stayed on 1 line.

@fabpot
Symfony member

but with this new design, the WDT will never be rendered on two lines anymore.

@shiroyuki

As soon as I can get 2.1 work, I'll close this pull request and send out a new one. :(

@shiroyuki

I close this pull request for now. But I'm welcome for any idea you might have here.

These are the screenshots:

New Toolbar for Symfony 2.1 (Normal State)

New Toolbar for Symfony 2.1 (Hover State)

@shiroyuki shiroyuki closed this Apr 10, 2012
@marcw

excellent work!

@fabpot fabpot added a commit that referenced this pull request Apr 20, 2012
@fabpot fabpot merged branch shiroyuki/master (PR #3850)
Commits
-------

75c7d3a Fixed the link to the method with onclick event.
ecbabec renamed 'Request handler' to 'Controller' and 'Route ID' to 'Route name'.
3c5ede4 Add a new query to display all information.
f6a866b Merged CSS for the toolbar in both embedded mode (on each page) and profiler.
306533b Updated the responsive design in addition to the scenario with authenticated users and exception notification.
4a3312b Updated the toolbar with the responsive design (normal-to-large scenario).
1eec2a2 Updated the toolbar with the responsive design (normal-to-small scenario).
03c8213 Refactored the CSS code for the toolbar out of the template.
37843b3 Updated with PHP logo (only the text).
d5e0ccc Made the toolbar to show the version, memory usage, the state of security (both a abbreviation and an associate description) and number of DB requests and request time.
37ad8a6 Removed the check for verbose and adjusted the style when the toolbar is on the top of the page.
67b0532 Redesigned the WDT.

Discussion
----------

Re-design the debugging toolbar

The toolbar is very useful and containing lots of information. However, as there are too much information, it is very distracting and the toolbar area somehow ends up taking too much space and then becomes something like a panel.

The main purpose of this pull request is to hide any information and show only whenever the user wants to see, except the status code and response time.

This is based on [the pull request #3833](#3833) with the feedbacks and for 2.1 (master).

The testing app is available at http://home.shiroyuki.com.

---------------------------------------------------------------------------

by stof at 2012-04-10T06:24:36Z

@shiroyuki your testing app denies the access because of the restriction in app_dev.php

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T06:27:27Z

@stof: I'm sorry. It should be working now.

---------------------------------------------------------------------------

by stof at 2012-04-10T06:45:39Z

Moving the toolbar to the top of the page means it will hide some content of the page. You should keep it at the bottom

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T06:48:28Z

Just a moment ago, I changed the position of the toolbar via `config_dev` so I could check when WDT is on the top.

I just reverted the config file. :D

---------------------------------------------------------------------------

by fabpot at 2012-04-10T06:55:16Z

Some comments:

 * I would have kept the number of database request as this number is probably the one everybody should have a look at on every page.
 * I would have used the original PHP logo (in black and white) instead of a non-standard one

But overall, this is a very nice improvement.

---------------------------------------------------------------------------

by stloyd at 2012-04-10T06:55:43Z

There is an issue with "bubbling" at Firefox 11 (at least), when you hover `<a>` element, the hover event seems to be "launched" twice.

---------------------------------------------------------------------------

by fabpot at 2012-04-10T06:56:13Z

As the verbose mode has been removed from the template, it should also be removed from the configuration (I can do that after merging if you don't know how to do that).

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T07:05:31Z

@stloyd I noticed that too. As I couldn't find the same issue on Webkit-based browsers and all effects on this toolbar heavily relies on CSS, it could have been a glitch on Firefox.

@fabpot I'll see what I can do with the number of DB request and the logo.

---------------------------------------------------------------------------

by asm89 at 2012-04-10T07:26:28Z

Will there be options to somehow keep the debug toolbar 'expanded' or something? I guess the folding of the sf and php information makes sense, but I personally look at the request/time/memory/security and query parts of the toolbar a lot. As my browser window is big enough to show all information at once, this would be a huge step backwards imo.

---------------------------------------------------------------------------

by XWB at 2012-04-10T07:28:38Z

Agreed with @asm89, I also want the option to show all the information on my screen.

---------------------------------------------------------------------------

by fabpot at 2012-04-10T08:28:00Z

I tend to agree too with @asm89. What about reusing the `verbose` option for that. This was already its purpose anyway.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T14:56:45Z

How about using media query?

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T02:20:32Z

Please note that the latest commit still doesn't have the new logo for PHP.

As DoctrineBundle now has its own repository, the change to show the number of DB requests is already done via DoctrineBundle's [PR 57](doctrine/DoctrineBundle#57).

---------------------------------------------------------------------------

by guilhermeblanco at 2012-04-11T02:50:47Z

@fabpot @shiroyuki as soon as this patch is merged I will do the same on DoctrineBundle.
All you need to do is look at me over our desks' separator. =D

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T03:17:41Z

The last commit has the updated PHP logo. Unfortunately as @stloyd and @guilhermeblanco pointed out, the flicking on the toolbar when the mouse is over might have been due to the CSS issue on Firefox.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T04:46:36Z

Nice work shiroyuki. I always had the feeling the toolbar can be improved. Good that you got this one going.
I would remove the verbose option (rarely nobody changes it) and use media queries to accomplish a responsive design that shows as much information as possible. And only shows the most important facts when there is not enough space.
E.g. the symfony version could be removed if it doesn't fit on the screen because it's mostly static from request to request.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T04:48:45Z

Another idea: Add a panel "PHP Info" to the profiler that shows the output of `phpinfo()`. This panel is linked from the PHP logo in the WDT which currently has no link on it.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T15:47:51Z

@Tobion: It would be an overkill if `phpinfo()` was visible in the toolbar. Additionally, the toolbar doesn't fit to show that amount of information. Plus, the information released by `phpinfo()` is also static and easily obtained by a simple PHP script. I don't think that WDT should be showing this information.

Please note that the media query is not yet implement. The followings are still unknown to me:

* should we support the toolbar for mobile device?
* what is the minimum screen size?

---------------------------------------------------------------------------

by Tobion at 2012-04-11T15:52:43Z

@shiroyuki you misunderstood me. phpinfo() should be a new panel in the PROFILER, not the WDT. It is reachable from the WDT by clicking on the PHP logo. But that can be implemented in a seperate PR. It's just an idea and before I would implement it, I'd like to receive feedback if it would be accepted at all.

---------------------------------------------------------------------------

by fabpot at 2012-04-11T16:38:44Z

Displaying `phpinfo()` data is not in the scope of this PR.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T16:48:50Z

@fabpot yeah. But would you accept such a PR or do you think it's not useful?

---------------------------------------------------------------------------

by fabpot at 2012-04-11T16:57:49Z

@Tobion The web profiler is mainly about information for the current request; so I'm not sure it would be useful to have such a tab in the profiler.

---------------------------------------------------------------------------

by vicb at 2012-04-11T17:06:15Z

@fabpot @Tobion what about adding it in the config panel ? Not sure if it is very useful but I have seen to many `phpinfo.php` in the web root folder. (It could be an expandable panel loaded via ajax like what is used for the Doctrine explain panel).

---------------------------------------------------------------------------

by shiroyuki at 2012-04-12T03:11:40Z

@tobian @vicb: what kind of information are you looking from `phpinfo()`?

---------------------------------------------------------------------------

by Felds at 2012-04-12T03:30:02Z

The equivalent for `phpinfo()` was extremely convenient and helped a lot in Symfony 1. It's out of scope but an optional panel could be nice.
Ini flags are of great help when debugging on a hurry.

👍 for that!

---------------------------------------------------------------------------

by Tobion at 2012-04-12T03:37:52Z

@shiroyuki I don't understand your question. Everything of it should be displayed. But don't worry about phpinfo(), I'll work on that in a seperate PR. You can focus on the responsive design. ;)

---------------------------------------------------------------------------

by vicb at 2012-04-12T06:54:35Z

@shiroyuki I am not looking for anything specific. Just saying I have seen many times customer code using a publicly accessible file to return the info and it would help to get ride of this file.

---------------------------------------------------------------------------

by sstok at 2012-04-12T07:59:18Z

```
should we support the toolbar for mobile device?
```

Good question, I don't think so because the screen-size is to small to show anything useful.
Maybe a small icon to display the information as overlay, including the token so you can refer to that on a bigger screen?.

---------------------------------------------------------------------------

by johnnypeck at 2012-04-13T06:45:43Z

If your interested in a useful but not so intrusive way of providing the toolbar on mobile devices perhaps take a look at what the guys at Twitter have done with the topbar navigation converting to a semi-accordion style menu on mobile in Bootstrap. I can see the usefulness. Checkout the responsive.less which makes it easy enough to include/exclude depending on screen size. I found it quite useful in a recent project.

Regarding adding a tab for phpinfo, sure it would be useful BUT if the reasoning is that some people leave a publicly available phpinfo script therefore just include it then I would not include it. There are many more useful requirements of the toolbar rather than to insulate intro to web issues. That's like saying don't include the toolbar because someone may build an application that makes the toolbar available publicly (which will happen). I've seen too many projects in my years having no clue of versioning tools that must have been built on the server, live, with filenames like indexv1.php, indexv2.php, indexTryAgain.php, db credentials in the clear, and just hoping to find a point where it works enough. And yes, I've found those scripts were publicly available and still around years after they were created; security holes and all! I'm preaching to the choir here. You'll never stop stupid. All we can do is educate by any means we have and share our knowledge with one another. Aside from that devils advocate reasoning, I would include the phpinfo tab, it does make sense in those random "did I/they compile that in" circumstances. ;-) Sorry for the rant.

+1 for mobile
sorta+1 for phpinfo
+10 for better educating on how to include anything you need so phpinfo could be a "my first foray into adding a tool to my toolbar for Symfony" tutorial in the cookbook.

Again, sorry for the long winded rant. Cheers everyone. Goodnight.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-13T23:33:21Z

@stof I think we can remove the CSS.
e2326a7
@mmucklo mmucklo pushed a commit that referenced this pull request May 23, 2013
@fabpot fabpot merged branch shiroyuki/master (PR #3850)
Commits
-------

75c7d3a Fixed the link to the method with onclick event.
ecbabec renamed 'Request handler' to 'Controller' and 'Route ID' to 'Route name'.
3c5ede4 Add a new query to display all information.
f6a866b Merged CSS for the toolbar in both embedded mode (on each page) and profiler.
306533b Updated the responsive design in addition to the scenario with authenticated users and exception notification.
4a3312b Updated the toolbar with the responsive design (normal-to-large scenario).
1eec2a2 Updated the toolbar with the responsive design (normal-to-small scenario).
03c8213 Refactored the CSS code for the toolbar out of the template.
37843b3 Updated with PHP logo (only the text).
d5e0ccc Made the toolbar to show the version, memory usage, the state of security (both a abbreviation and an associate description) and number of DB requests and request time.
37ad8a6 Removed the check for verbose and adjusted the style when the toolbar is on the top of the page.
67b0532 Redesigned the WDT.

Discussion
----------

Re-design the debugging toolbar

The toolbar is very useful and containing lots of information. However, as there are too much information, it is very distracting and the toolbar area somehow ends up taking too much space and then becomes something like a panel.

The main purpose of this pull request is to hide any information and show only whenever the user wants to see, except the status code and response time.

This is based on [the pull request #3833](#3833) with the feedbacks and for 2.1 (master).

The testing app is available at http://home.shiroyuki.com.

---------------------------------------------------------------------------

by stof at 2012-04-10T06:24:36Z

@shiroyuki your testing app denies the access because of the restriction in app_dev.php

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T06:27:27Z

@stof: I'm sorry. It should be working now.

---------------------------------------------------------------------------

by stof at 2012-04-10T06:45:39Z

Moving the toolbar to the top of the page means it will hide some content of the page. You should keep it at the bottom

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T06:48:28Z

Just a moment ago, I changed the position of the toolbar via `config_dev` so I could check when WDT is on the top.

I just reverted the config file. :D

---------------------------------------------------------------------------

by fabpot at 2012-04-10T06:55:16Z

Some comments:

 * I would have kept the number of database request as this number is probably the one everybody should have a look at on every page.
 * I would have used the original PHP logo (in black and white) instead of a non-standard one

But overall, this is a very nice improvement.

---------------------------------------------------------------------------

by stloyd at 2012-04-10T06:55:43Z

There is an issue with "bubbling" at Firefox 11 (at least), when you hover `<a>` element, the hover event seems to be "launched" twice.

---------------------------------------------------------------------------

by fabpot at 2012-04-10T06:56:13Z

As the verbose mode has been removed from the template, it should also be removed from the configuration (I can do that after merging if you don't know how to do that).

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T07:05:31Z

@stloyd I noticed that too. As I couldn't find the same issue on Webkit-based browsers and all effects on this toolbar heavily relies on CSS, it could have been a glitch on Firefox.

@fabpot I'll see what I can do with the number of DB request and the logo.

---------------------------------------------------------------------------

by asm89 at 2012-04-10T07:26:28Z

Will there be options to somehow keep the debug toolbar 'expanded' or something? I guess the folding of the sf and php information makes sense, but I personally look at the request/time/memory/security and query parts of the toolbar a lot. As my browser window is big enough to show all information at once, this would be a huge step backwards imo.

---------------------------------------------------------------------------

by XWB at 2012-04-10T07:28:38Z

Agreed with @asm89, I also want the option to show all the information on my screen.

---------------------------------------------------------------------------

by fabpot at 2012-04-10T08:28:00Z

I tend to agree too with @asm89. What about reusing the `verbose` option for that. This was already its purpose anyway.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T14:56:45Z

How about using media query?

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T02:20:32Z

Please note that the latest commit still doesn't have the new logo for PHP.

As DoctrineBundle now has its own repository, the change to show the number of DB requests is already done via DoctrineBundle's [PR 57](doctrine/DoctrineBundle#57).

---------------------------------------------------------------------------

by guilhermeblanco at 2012-04-11T02:50:47Z

@fabpot @shiroyuki as soon as this patch is merged I will do the same on DoctrineBundle.
All you need to do is look at me over our desks' separator. =D

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T03:17:41Z

The last commit has the updated PHP logo. Unfortunately as @stloyd and @guilhermeblanco pointed out, the flicking on the toolbar when the mouse is over might have been due to the CSS issue on Firefox.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T04:46:36Z

Nice work shiroyuki. I always had the feeling the toolbar can be improved. Good that you got this one going.
I would remove the verbose option (rarely nobody changes it) and use media queries to accomplish a responsive design that shows as much information as possible. And only shows the most important facts when there is not enough space.
E.g. the symfony version could be removed if it doesn't fit on the screen because it's mostly static from request to request.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T04:48:45Z

Another idea: Add a panel "PHP Info" to the profiler that shows the output of `phpinfo()`. This panel is linked from the PHP logo in the WDT which currently has no link on it.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T15:47:51Z

@Tobion: It would be an overkill if `phpinfo()` was visible in the toolbar. Additionally, the toolbar doesn't fit to show that amount of information. Plus, the information released by `phpinfo()` is also static and easily obtained by a simple PHP script. I don't think that WDT should be showing this information.

Please note that the media query is not yet implement. The followings are still unknown to me:

* should we support the toolbar for mobile device?
* what is the minimum screen size?

---------------------------------------------------------------------------

by Tobion at 2012-04-11T15:52:43Z

@shiroyuki you misunderstood me. phpinfo() should be a new panel in the PROFILER, not the WDT. It is reachable from the WDT by clicking on the PHP logo. But that can be implemented in a seperate PR. It's just an idea and before I would implement it, I'd like to receive feedback if it would be accepted at all.

---------------------------------------------------------------------------

by fabpot at 2012-04-11T16:38:44Z

Displaying `phpinfo()` data is not in the scope of this PR.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T16:48:50Z

@fabpot yeah. But would you accept such a PR or do you think it's not useful?

---------------------------------------------------------------------------

by fabpot at 2012-04-11T16:57:49Z

@Tobion The web profiler is mainly about information for the current request; so I'm not sure it would be useful to have such a tab in the profiler.

---------------------------------------------------------------------------

by vicb at 2012-04-11T17:06:15Z

@fabpot @Tobion what about adding it in the config panel ? Not sure if it is very useful but I have seen to many `phpinfo.php` in the web root folder. (It could be an expandable panel loaded via ajax like what is used for the Doctrine explain panel).

---------------------------------------------------------------------------

by shiroyuki at 2012-04-12T03:11:40Z

@tobian @vicb: what kind of information are you looking from `phpinfo()`?

---------------------------------------------------------------------------

by Felds at 2012-04-12T03:30:02Z

The equivalent for `phpinfo()` was extremely convenient and helped a lot in Symfony 1. It's out of scope but an optional panel could be nice.
Ini flags are of great help when debugging on a hurry.

👍 for that!

---------------------------------------------------------------------------

by Tobion at 2012-04-12T03:37:52Z

@shiroyuki I don't understand your question. Everything of it should be displayed. But don't worry about phpinfo(), I'll work on that in a seperate PR. You can focus on the responsive design. ;)

---------------------------------------------------------------------------

by vicb at 2012-04-12T06:54:35Z

@shiroyuki I am not looking for anything specific. Just saying I have seen many times customer code using a publicly accessible file to return the info and it would help to get ride of this file.

---------------------------------------------------------------------------

by sstok at 2012-04-12T07:59:18Z

```
should we support the toolbar for mobile device?
```

Good question, I don't think so because the screen-size is to small to show anything useful.
Maybe a small icon to display the information as overlay, including the token so you can refer to that on a bigger screen?.

---------------------------------------------------------------------------

by johnnypeck at 2012-04-13T06:45:43Z

If your interested in a useful but not so intrusive way of providing the toolbar on mobile devices perhaps take a look at what the guys at Twitter have done with the topbar navigation converting to a semi-accordion style menu on mobile in Bootstrap. I can see the usefulness. Checkout the responsive.less which makes it easy enough to include/exclude depending on screen size. I found it quite useful in a recent project.

Regarding adding a tab for phpinfo, sure it would be useful BUT if the reasoning is that some people leave a publicly available phpinfo script therefore just include it then I would not include it. There are many more useful requirements of the toolbar rather than to insulate intro to web issues. That's like saying don't include the toolbar because someone may build an application that makes the toolbar available publicly (which will happen). I've seen too many projects in my years having no clue of versioning tools that must have been built on the server, live, with filenames like indexv1.php, indexv2.php, indexTryAgain.php, db credentials in the clear, and just hoping to find a point where it works enough. And yes, I've found those scripts were publicly available and still around years after they were created; security holes and all! I'm preaching to the choir here. You'll never stop stupid. All we can do is educate by any means we have and share our knowledge with one another. Aside from that devils advocate reasoning, I would include the phpinfo tab, it does make sense in those random "did I/they compile that in" circumstances. ;-) Sorry for the rant.

+1 for mobile
sorta+1 for phpinfo
+10 for better educating on how to include anything you need so phpinfo could be a "my first foray into adding a tool to my toolbar for Symfony" tutorial in the cookbook.

Again, sorry for the long winded rant. Cheers everyone. Goodnight.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-13T23:33:21Z

@stof I think we can remove the CSS.
6dfd832
@gencer gencer pushed a commit to gencer/Yaml that referenced this pull request Jul 29, 2014
@fabpot fabpot merged branch shiroyuki/master (PR #3850)
Commits
-------

75c7d3a Fixed the link to the method with onclick event.
ecbabec renamed 'Request handler' to 'Controller' and 'Route ID' to 'Route name'.
3c5ede4 Add a new query to display all information.
f6a866b Merged CSS for the toolbar in both embedded mode (on each page) and profiler.
306533b Updated the responsive design in addition to the scenario with authenticated users and exception notification.
4a3312b Updated the toolbar with the responsive design (normal-to-large scenario).
1eec2a2 Updated the toolbar with the responsive design (normal-to-small scenario).
03c8213 Refactored the CSS code for the toolbar out of the template.
37843b3 Updated with PHP logo (only the text).
d5e0ccc Made the toolbar to show the version, memory usage, the state of security (both a abbreviation and an associate description) and number of DB requests and request time.
37ad8a6 Removed the check for verbose and adjusted the style when the toolbar is on the top of the page.
67b0532 Redesigned the WDT.

Discussion
----------

Re-design the debugging toolbar

The toolbar is very useful and containing lots of information. However, as there are too much information, it is very distracting and the toolbar area somehow ends up taking too much space and then becomes something like a panel.

The main purpose of this pull request is to hide any information and show only whenever the user wants to see, except the status code and response time.

This is based on [the pull request #3833](symfony/symfony#3833) with the feedbacks and for 2.1 (master).

The testing app is available at http://home.shiroyuki.com.

---------------------------------------------------------------------------

by stof at 2012-04-10T06:24:36Z

@shiroyuki your testing app denies the access because of the restriction in app_dev.php

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T06:27:27Z

@stof: I'm sorry. It should be working now.

---------------------------------------------------------------------------

by stof at 2012-04-10T06:45:39Z

Moving the toolbar to the top of the page means it will hide some content of the page. You should keep it at the bottom

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T06:48:28Z

Just a moment ago, I changed the position of the toolbar via `config_dev` so I could check when WDT is on the top.

I just reverted the config file. :D

---------------------------------------------------------------------------

by fabpot at 2012-04-10T06:55:16Z

Some comments:

 * I would have kept the number of database request as this number is probably the one everybody should have a look at on every page.
 * I would have used the original PHP logo (in black and white) instead of a non-standard one

But overall, this is a very nice improvement.

---------------------------------------------------------------------------

by stloyd at 2012-04-10T06:55:43Z

There is an issue with "bubbling" at Firefox 11 (at least), when you hover `<a>` element, the hover event seems to be "launched" twice.

---------------------------------------------------------------------------

by fabpot at 2012-04-10T06:56:13Z

As the verbose mode has been removed from the template, it should also be removed from the configuration (I can do that after merging if you don't know how to do that).

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T07:05:31Z

@stloyd I noticed that too. As I couldn't find the same issue on Webkit-based browsers and all effects on this toolbar heavily relies on CSS, it could have been a glitch on Firefox.

@fabpot I'll see what I can do with the number of DB request and the logo.

---------------------------------------------------------------------------

by asm89 at 2012-04-10T07:26:28Z

Will there be options to somehow keep the debug toolbar 'expanded' or something? I guess the folding of the sf and php information makes sense, but I personally look at the request/time/memory/security and query parts of the toolbar a lot. As my browser window is big enough to show all information at once, this would be a huge step backwards imo.

---------------------------------------------------------------------------

by XWB at 2012-04-10T07:28:38Z

Agreed with @asm89, I also want the option to show all the information on my screen.

---------------------------------------------------------------------------

by fabpot at 2012-04-10T08:28:00Z

I tend to agree too with @asm89. What about reusing the `verbose` option for that. This was already its purpose anyway.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-10T14:56:45Z

How about using media query?

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T02:20:32Z

Please note that the latest commit still doesn't have the new logo for PHP.

As DoctrineBundle now has its own repository, the change to show the number of DB requests is already done via DoctrineBundle's [PR 57](doctrine/DoctrineBundle#57).

---------------------------------------------------------------------------

by guilhermeblanco at 2012-04-11T02:50:47Z

@fabpot @shiroyuki as soon as this patch is merged I will do the same on DoctrineBundle.
All you need to do is look at me over our desks' separator. =D

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T03:17:41Z

The last commit has the updated PHP logo. Unfortunately as @stloyd and @guilhermeblanco pointed out, the flicking on the toolbar when the mouse is over might have been due to the CSS issue on Firefox.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T04:46:36Z

Nice work shiroyuki. I always had the feeling the toolbar can be improved. Good that you got this one going.
I would remove the verbose option (rarely nobody changes it) and use media queries to accomplish a responsive design that shows as much information as possible. And only shows the most important facts when there is not enough space.
E.g. the symfony version could be removed if it doesn't fit on the screen because it's mostly static from request to request.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T04:48:45Z

Another idea: Add a panel "PHP Info" to the profiler that shows the output of `phpinfo()`. This panel is linked from the PHP logo in the WDT which currently has no link on it.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-11T15:47:51Z

@Tobion: It would be an overkill if `phpinfo()` was visible in the toolbar. Additionally, the toolbar doesn't fit to show that amount of information. Plus, the information released by `phpinfo()` is also static and easily obtained by a simple PHP script. I don't think that WDT should be showing this information.

Please note that the media query is not yet implement. The followings are still unknown to me:

* should we support the toolbar for mobile device?
* what is the minimum screen size?

---------------------------------------------------------------------------

by Tobion at 2012-04-11T15:52:43Z

@shiroyuki you misunderstood me. phpinfo() should be a new panel in the PROFILER, not the WDT. It is reachable from the WDT by clicking on the PHP logo. But that can be implemented in a seperate PR. It's just an idea and before I would implement it, I'd like to receive feedback if it would be accepted at all.

---------------------------------------------------------------------------

by fabpot at 2012-04-11T16:38:44Z

Displaying `phpinfo()` data is not in the scope of this PR.

---------------------------------------------------------------------------

by Tobion at 2012-04-11T16:48:50Z

@fabpot yeah. But would you accept such a PR or do you think it's not useful?

---------------------------------------------------------------------------

by fabpot at 2012-04-11T16:57:49Z

@Tobion The web profiler is mainly about information for the current request; so I'm not sure it would be useful to have such a tab in the profiler.

---------------------------------------------------------------------------

by vicb at 2012-04-11T17:06:15Z

@fabpot @Tobion what about adding it in the config panel ? Not sure if it is very useful but I have seen to many `phpinfo.php` in the web root folder. (It could be an expandable panel loaded via ajax like what is used for the Doctrine explain panel).

---------------------------------------------------------------------------

by shiroyuki at 2012-04-12T03:11:40Z

@tobian @vicb: what kind of information are you looking from `phpinfo()`?

---------------------------------------------------------------------------

by Felds at 2012-04-12T03:30:02Z

The equivalent for `phpinfo()` was extremely convenient and helped a lot in Symfony 1. It's out of scope but an optional panel could be nice.
Ini flags are of great help when debugging on a hurry.

👍 for that!

---------------------------------------------------------------------------

by Tobion at 2012-04-12T03:37:52Z

@shiroyuki I don't understand your question. Everything of it should be displayed. But don't worry about phpinfo(), I'll work on that in a seperate PR. You can focus on the responsive design. ;)

---------------------------------------------------------------------------

by vicb at 2012-04-12T06:54:35Z

@shiroyuki I am not looking for anything specific. Just saying I have seen many times customer code using a publicly accessible file to return the info and it would help to get ride of this file.

---------------------------------------------------------------------------

by sstok at 2012-04-12T07:59:18Z

```
should we support the toolbar for mobile device?
```

Good question, I don't think so because the screen-size is to small to show anything useful.
Maybe a small icon to display the information as overlay, including the token so you can refer to that on a bigger screen?.

---------------------------------------------------------------------------

by johnnypeck at 2012-04-13T06:45:43Z

If your interested in a useful but not so intrusive way of providing the toolbar on mobile devices perhaps take a look at what the guys at Twitter have done with the topbar navigation converting to a semi-accordion style menu on mobile in Bootstrap. I can see the usefulness. Checkout the responsive.less which makes it easy enough to include/exclude depending on screen size. I found it quite useful in a recent project.

Regarding adding a tab for phpinfo, sure it would be useful BUT if the reasoning is that some people leave a publicly available phpinfo script therefore just include it then I would not include it. There are many more useful requirements of the toolbar rather than to insulate intro to web issues. That's like saying don't include the toolbar because someone may build an application that makes the toolbar available publicly (which will happen). I've seen too many projects in my years having no clue of versioning tools that must have been built on the server, live, with filenames like indexv1.php, indexv2.php, indexTryAgain.php, db credentials in the clear, and just hoping to find a point where it works enough. And yes, I've found those scripts were publicly available and still around years after they were created; security holes and all! I'm preaching to the choir here. You'll never stop stupid. All we can do is educate by any means we have and share our knowledge with one another. Aside from that devils advocate reasoning, I would include the phpinfo tab, it does make sense in those random "did I/they compile that in" circumstances. ;-) Sorry for the rant.

+1 for mobile
sorta+1 for phpinfo
+10 for better educating on how to include anything you need so phpinfo could be a "my first foray into adding a tool to my toolbar for Symfony" tutorial in the cookbook.

Again, sorry for the long winded rant. Cheers everyone. Goodnight.

---------------------------------------------------------------------------

by shiroyuki at 2012-04-13T23:33:21Z

@stof I think we can remove the CSS.
38cabda
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment