[WIP][WebProfilerBundle] Show AJAX requests in the symfony profiler toolbar #8896

Closed
wants to merge 32 commits into
from

Conversation

Projects
None yet
Contributor

Burgov commented Aug 30, 2013

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

Applications make more and more use of AJAX calls to get partials or JSON data, and whenever something goes wrong or I wish to know more about DB queries executed, I have to find the profile in the profiler, which is rather anoying and sometimes can even be somewhat difficult. This PR adds a section to the debugger toolbar which shows the amount of AJAX calls executed during the current page show, and if you hover over it, gives more information on the calls, like the method, the URI, the time it took, the status, and a link to the profile of that specific request.

Todo:

  • create an actual icon for this, instead of using a copy of the "memory" icon and fix the layout of the data
  • now only JQuery and AngularJS calls are logged, perhaps more libraries should be added
  • refactor the JS (my pure JS skills are a little rusty since the Netscape era...
  • either prevent the icon from linking (is that possible?) or actually show some information in the profiler itself (that means actually registering the calls in the profiler server side)
  • Generate the url to the profiler instead of the way it is done now (url = "/_profile/"+token)

You can clone https://github.com/Burgov/profile_ajax_requests to see a working example

Contributor

GromNaN commented Aug 30, 2013

This is awesome.

In your current implementation, the JS of the toolbar is the last of the page to be executed (append just before </body>). You will miss every AJAX call that are done during page rendering. The JS must be just after <head> to ensure that every AJAX calls are caught.

To do that, you need to be independent from any library. You can do something generic like this : https://gist.github.com/suprememoocow/2823600

@stof stof commented on an outdated diff Aug 30, 2013

...ofilerBundle/Resources/views/Collector/ajax.html.twig
@@ -0,0 +1,17 @@
+{% extends '@WebProfiler/Profiler/layout.html.twig' %}
+
+{% block toolbar %}
+ {% set icon %}
+ <span>
+ <img width="13" height="28" alt="AJAX requests" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAA0AAAAcBAMAAABITyhxAAAAJ1BMVEXNzc3///////////////////////8/Pz////////////+NjY0/Pz9lMO+OAAAADHRSTlMAABAgMDhAWXCvv9e8JUuyAAAAQ0lEQVQI12MQBAMBBmLpMwoMDAw6BxjOOABpHyCdAKRzsNDp5eXl1KBh5oHBAYY9YHoDQ+cqIFjZwGCaBgSpBrjcCwCZgkUHKKvX+wAAAABJRU5ErkJggg==">
+ <span id="sf-toolbar-ajax-requests">0</span>
@stof

stof Aug 30, 2013

Member

The toolbar was designed to be able to be rendered several times by avoiding using ids in it. It should be avoided here too

@stof stof commented on an outdated diff Aug 30, 2013

...filerBundle/Resources/views/Profiler/toolbar.css.twig
@@ -271,6 +271,24 @@
line-height: 19px;
}
+.ajax-request-error {
+ color: red;
+}
+.ajax-request-loading {
+ -webkit-animation: blink .5s linear infinite;
+ animation: blink .5s linear infinite;
+}
+@keyframes blink {
+ 0% { color: black; }
+ 50% { color: #bbb; }
+ 100% { color: black; }
+}
+@-webkit-keyframes blink {
@stof

stof Aug 30, 2013

Member

what about non-wekbit browsers (firefox for instance) ?

@stof stof commented on an outdated diff Aug 30, 2013

...rBundle/Resources/views/Profiler/toolbar_js.html.twig
+ if (jQuery) {
+ jQuery(document).ajaxSend(function(e, xhr, config) {
+ requestStack.push({
+ loading: true,
+ error: false,
+ config: xhr,
+ url: config.url,
+ method: config.type,
+ source: 'jquery',
+ start: new Date()
+ });
+ updateAjaxRequests();
+ }).ajaxComplete(function(e, xhr) {
+ for (var i in requestStack) {
+ var request = requestStack[i];
+ if (request.config === xhr) {
@stof

stof Aug 30, 2013

Member

you should also remove the reference to the xhr object to let it be garbage collected

@stof stof commented on an outdated diff Aug 30, 2013

...rBundle/Resources/views/Profiler/toolbar_js.html.twig
+ div.appendChild(pathSpan);
+
+ if (request.duration) {
+ var durationSpan = document.createElement('span');
+ durationSpan.innerText = request.duration + "s";
+ div.appendChild(durationSpan);
+ }
+
+ if (request.token) {
+ var profilerA = document.createElement('a');
+ profilerA.setAttribute('href', '/_profiler/' + request.token);
+ profilerA.innerText = 'profiler';
+ div.appendChild(profilerA);
+ }
+ div.className = request.error ? 'ajax-request-error' : (request.loading ? 'ajax-request-loading' : 'ajax-request-ok');
+ list.appendChild(div);
@stof

stof Aug 30, 2013

Member

As you are building a list, wouldn't it be better to have a <ul> with several <li> than a <div> with several <div> ?

@stof stof commented on an outdated diff Aug 30, 2013

...rBundle/Resources/views/Profiler/toolbar_js.html.twig
+ document.getElementById('sf-toolbar-ajax-requests').innerHTML = requestStack.length;
+ }
+ var list = document.getElementById('sf-toolbar-ajax-request-list');
+ if (list) {
+ list.innerHTML = '';
+ for(var i in requestStack) {
+ var request = requestStack[i];
+
+ var div = document.createElement('div');
+
+ var pathSpan = document.createElement('span');
+ pathSpan.innerHTML = request.method + " " + request.url;
+ div.appendChild(pathSpan);
+
+ if (request.duration) {
+ var durationSpan = document.createElement('span');
@stof

stof Aug 30, 2013

Member

you should use a <time> element to represent a duration rather than a <span>

@stof stof commented on an outdated diff Aug 30, 2013

...rBundle/Resources/views/Profiler/toolbar_js.html.twig
+ }
+ var list = document.getElementById('sf-toolbar-ajax-request-list');
+ if (list) {
+ list.innerHTML = '';
+ for(var i in requestStack) {
+ var request = requestStack[i];
+
+ var div = document.createElement('div');
+
+ var pathSpan = document.createElement('span');
+ pathSpan.innerHTML = request.method + " " + request.url;
+ div.appendChild(pathSpan);
+
+ if (request.duration) {
+ var durationSpan = document.createElement('span');
+ durationSpan.innerText = request.duration + "s";
@stof

stof Aug 30, 2013

Member

The second may not be the best unit. Request duration is displayed in ms in the time collector template

@stof stof commented on an outdated diff Aug 30, 2013

...rBundle/Resources/views/Profiler/toolbar_js.html.twig
+
+ var div = document.createElement('div');
+
+ var pathSpan = document.createElement('span');
+ pathSpan.innerHTML = request.method + " " + request.url;
+ div.appendChild(pathSpan);
+
+ if (request.duration) {
+ var durationSpan = document.createElement('span');
+ durationSpan.innerText = request.duration + "s";
+ div.appendChild(durationSpan);
+ }
+
+ if (request.token) {
+ var profilerA = document.createElement('a');
+ profilerA.setAttribute('href', '/_profiler/' + request.token);
@stof

stof Aug 30, 2013

Member

the /_profiler prefix is something coming from the routing import in routing_dev.yml, so you cannot hardcode it in WebProfilerBundle

Contributor

Burgov commented Aug 31, 2013

@GromNaN i've updated the script inspired by the example you showed. There's just one problem with this sollution, and that is that is could break if someone decides to override the open() method himself. But I don't think many people do this, and even in that case, it would probably still be proxied, so the script remains working.

I've replaced the script to be executed as soon as possible. It now even catches the call to fetch the profiler toolbar. I can't really place it earlier, in the head for instance, as that would require users to implement it into their templates.

@stof thanks for all the comments. I've fixed all.
The XHR object garbage collection problem was fixed by the refactoring of the logging methods. I used the route _profiler_home and appended it with the token. I can't actually use the _profiler route since the token is not yet available at this point and it would require a lib like FOSJSRoutingBundle to generate it correctly.

@stloyd stloyd and 1 other commented on an outdated diff Aug 31, 2013

...e/FrameworkBundle/DataCollector/AjaxDataCollector.php
@@ -0,0 +1,22 @@
+<?php
+
+namespace Symfony\Bundle\FrameworkBundle\DataCollector;
@stloyd

stloyd Aug 31, 2013

Contributor

Missing license header.

@Burgov

Burgov Sep 2, 2013

Contributor

fixed

@stloyd stloyd and 1 other commented on an outdated diff Aug 31, 2013

...ilerBundle/Resources/views/Profiler/base_js.html.twig
+ stackElement.duration = new Date() - stackElement.start;
+ stackElement.loading = false;
+ stackElement.error = self.status < 200 || self.status >= 400;
+ stackElement.token = self.getResponseHeader("X-Debug-Token");
+
+ updateAjaxRequests();
+ }
+ }, false);
+
+ updateAjaxRequests();
+ }
+
+ proxied.apply(this, [].slice.call(arguments));
+ };
+
+ var updateAjaxRequests = function() {
@stloyd

stloyd Aug 31, 2013

Contributor

Maybe detectAjaxRequests?

@Burgov

Burgov Sep 2, 2013

Contributor

I've renamed it to "renderAjaxRequests". I think that should be an appropriate name

@Baachi Baachi and 1 other commented on an outdated diff Sep 2, 2013

...ilerBundle/Resources/views/Profiler/base_js.html.twig
+
+ var pathSpan = document.createElement('span');
+ pathSpan.innerHTML = request.method + " " + request.url;
+ li.appendChild(pathSpan);
+
+ if (request.duration) {
+ li.appendChild(document.createTextNode(' '));
+ var durationSpan = document.createElement('time');
+ durationSpan.innerText = request.duration + "ms";
+ li.appendChild(durationSpan);
+ }
+
+ if (request.token) {
+ li.appendChild(document.createTextNode(' '));
+ var profilerA = document.createElement('a');
+ profilerA.setAttribute('href', {{ path('_profiler_home') }} + request.token);
@Baachi

Baachi Sep 2, 2013

Contributor

You forgot the apostrophes here.

@Burgov

Burgov Sep 2, 2013

Contributor

What apostrophes do you mean?

@Baachi

Baachi Sep 2, 2013

Contributor

It should be:

profilerA.setAttribute('{{ path('_profiler_home') }}' + request.token);
@Burgov

Burgov Sep 2, 2013

Contributor

Ah! Indeed it should. Even after rereading the line 3 times I still managed to overlook it :)

@Baachi

Baachi Sep 2, 2013

Contributor

No problem :)

Contributor

GromNaN commented Sep 2, 2013

Thinking about the use case, I found that using your browser debugging tool (firebug / Chrome Dev Tools ...) you can already get the profiler token for each request by looking at the response header "X-Debug-Token".

If you use FirePHP, the profiler URL could be logged at "info" level to get it directly in your console.

@stof stof commented on an outdated diff Sep 2, 2013

...filerBundle/Resources/views/Profiler/toolbar.css.twig
@@ -271,6 +271,36 @@
line-height: 19px;
}
+.ajax-request-error {
+ color: red;
+}
+.ajax-request-loading {
+ animation: blink .5s ease-in-out infinite;
@stof

stof Sep 2, 2013

Member

the W3C alternative should come last, to overwrite the prefixed version in browsers supporting both instead of the opposite

@stof stof and 1 other commented on an outdated diff Sep 2, 2013

...ilerBundle/Resources/views/Profiler/base_js.html.twig
+ stackElement.duration = new Date() - stackElement.start;
+ stackElement.loading = false;
+ stackElement.error = self.status < 200 || self.status >= 400;
+ stackElement.token = self.getResponseHeader("X-Debug-Token");
+
+ renderAjaxRequests();
+ }
+ }, false);
+
+ renderAjaxRequests();
+ }
+
+ proxied.apply(this, [].slice.call(arguments));
+ };
+
+ var renderAjaxRequests = function() {
@stof

stof Sep 2, 2013

Member

Please avoid leaking global variables. You have 3 of them currently: requestStack, proxied and renderAjaxRequests. Everything you need to expose should be added on the Sfjs variable defined below, and anything else should be kept hidden inside the closure (requestStack and proxies have no reason to be exposed)

@Burgov

Burgov Sep 2, 2013

Contributor

I've moved stuff around a bit. Is it OK like this?

@stof stof and 1 other commented on an outdated diff Sep 2, 2013

...ilerBundle/Resources/views/Profiler/base_js.html.twig
load: function(selector, url, onSuccess, onError, options) {
+ createXHRLoadProxy();
@stof

stof Sep 2, 2013

Member

This is wrong. the XHR proxy should not be created each time you call Sfjs.load it should be created only once (like you did previously. the entire proxying code was right except that it needed to be inside the closure to hide it from the global scope)

@stof

stof Sep 2, 2013

Member

thus, calling Sfjs.load several times would now log duplicates in the toolbar (look at your Doctrine panel when you explain queries for instance)

@Burgov

Burgov Sep 2, 2013

Contributor

I didn't realise the load method could be called more than once. I've moved the creating of the proxy outside the load method

@stof stof commented on an outdated diff Sep 2, 2013

...ilerBundle/Resources/views/Profiler/base_js.html.twig
};
+ createXHRLoadProxy();
@stof

stof Sep 2, 2013

Member

I don't think you need to create a function enclosing this logic inside the closure function

@stof stof commented on an outdated diff Sep 2, 2013

...filerBundle/Resources/views/Profiler/toolbar.css.twig
@@ -271,6 +271,36 @@
line-height: 19px;
}
+.ajax-request-error {
@stof

stof Sep 2, 2013

Member

All classes used in the styles of the toolbar should start with sf- to avoid conflicting with the styles of the page. We don't control the page as this is injected into the app (the same is true for the name of the keyframes btw)

@staabm staabm and 1 other commented on an outdated diff Sep 3, 2013

...ilerBundle/Resources/views/Profiler/base_js.html.twig
+ var requestState = 'ok';
+ if (request.error) {
+ requestState = 'error';
+ if (state != "loading" && i > requestStack.length - 4) {
+ state = 'error';
+ }
+ } else if (request.loading) {
+ requestState = 'loading';
+ state = 'loading'
+ }
+ li.className = 'sf-ajax-request-' + requestState;
+ ul.appendChild(li);
+ }
+
+ list[0].innerHTML = '';
+ list[0].appendChild(ul);
@staabm

staabm Sep 3, 2013

Contributor

this could be slow in case a lot of items are appended... you might consider using a DocumentFragment, see e.g. http://ejohn.org/blog/dom-documentfragments/

@Burgov

Burgov Nov 6, 2013

Contributor

I've updated the code. Is this the way to go?

@staabm staabm commented on the diff Sep 3, 2013

...filerBundle/Resources/views/Profiler/toolbar.css.twig
@@ -271,6 +271,36 @@
line-height: 19px;
}
+.sf-ajax-request-error {
+ color: red;
+}
+.sf-ajax-request-loading {
+ -webkit-animation: sf-blink .5s ease-in-out infinite;
+ -o-animation: sf-blink .5s ease-in-out infinite;
+ -moz-animation: sf-blink .5s ease-in-out infinite;
+ animation: sf-blink .5s ease-in-out infinite;
+}
+@-webkit-keyframes sf-blink {
@staabm

staabm Sep 3, 2013

Contributor

instead of repeating all those values you could just separate the selectors by comma

@-webkit-keyframes sf-blink,
@-moz-keyframes sf-blink,
@-o-keyframes sf-blink,
@keyframes sf-blink {
    0% { color: black; }
    50% { color: #bbb; }
    100% { color: black; }
}
@stof

stof Sep 3, 2013

Member

@staabm I don't think it works. Browsers are ignoring rules that don't understand, and webkit will not understand a rule containing @-moz-keyframes

Contributor

Burgov commented Nov 6, 2013

If anyone could help me with the last two TODO's, it'd be much appreciated :)

Contributor

Burgov commented Apr 28, 2014

OK, I finished the PR. The icon now looks like this: ajax

@fabpot do you want me to squash the commits?

@staabm staabm and 1 other commented on an outdated diff Apr 28, 2014

...ilerBundle/Resources/views/Profiler/base_js.html.twig
};
+ var proxied = XMLHttpRequest.prototype.open;
+
+ XMLHttpRequest.prototype.open = function(method, url, async, user, pass) {
+ var self = this;
+
+ /* prevent logging AJAX calls to static and inline files, like templates */
+ if (url.charAt(0) === '/' && !url.substr(0, 8) !== '/bundles') {
@staabm

staabm Apr 28, 2014

Contributor

.charAt() is not avail in IE <= 8. .substr(0, 1) works x-browser

@Burgov

Burgov Apr 28, 2014

Contributor

changed, thx

Owner

fabpot commented May 4, 2014

@burgov Can you share some screenshots of this feature?

Contributor

Burgov commented May 4, 2014

@fabpot sure. Here they are:

  1. Initial state: it shows the number "1" because Symfony has already made an AJAX request for the WDT toolbar itself
    image
  2. I just clicked on "200", the number is now blinking and in the dialog the currently running request is blinking as wel. Some request details are displayed in the dialog.
    image
  3. I clicked on 400 and waited for the response. The number stopped blinking, but turned red, and in the dialog a link appears next to the URL to go directly to the profile page of that request (by now that same link has appeared for the previous request, which has also finished)
    image
Member

wouterj commented May 4, 2014

I would suggest to make some design changes:

  • Put the number in the same bubble as the other numbers in logs, hash, security & request
  • Instead of a blinking number, I propose to use a green bubble for a succesful request, a grey (like the hash) when it is in the queue and a red bubble for a failed request
  • The icon is too gray and much smaller than the others. Can't we simply use 2 black arrows, with some distance between them?
  • I would move them to the other request stuff, just after the block with 200

Apart from that: 👍 for doing this!

Contributor

hice3000 commented May 4, 2014

I wonder about the parameters printed behind the ampersand, are those real GET attributes or the data collected by the profiler?
Anyway, I totally agree to all points made by @wouterj , and I am 👍 on this, too.

Contributor

Burgov commented May 4, 2014

@wouterj I did * 1, 2 and 4 (except I kept the blinking part, I found it just more clearly to be saying "hey, I'm loading stuff")

About the 3rd *, I'm really bad at image manipulation. The one you see now took me a long time to make and to be honest, I don't really want to touch it anymore :) If anyone wants to make a PR for a better image, your contribution is much welcome!

@hice3000 it's showing the real GET attributes

Contributor

Burgov commented May 4, 2014

New screenshot:

image

Contributor

stloyd commented May 4, 2014

@burgov I think that simple "table" will be a bit cleaner for this toolbar panel.

Something like:

Method URL Time Profile
GET /wdt/54c535 703ms 4daf33
GET /blabla?xxx 1105ms 099d50
Contributor

Burgov commented May 4, 2014

@stloyd you're right, that does look better. I wonder why I didn't do that right away... New screenshot:

image

Member

wouterj commented May 4, 2014

Hmm, I think it's too much markup for such a small box.

Contributor

Burgov commented May 4, 2014

@wouterj I'm really the worst designer ever so I can't really tell... What would your suggestion be?

@stof stof commented on an outdated diff May 5, 2014

...ilerBundle/Resources/views/Profiler/base_js.html.twig
};
+ var proxied = XMLHttpRequest.prototype.open;
+
+ XMLHttpRequest.prototype.open = function(method, url, async, user, pass) {
+ var self = this;
+
+ /* prevent logging AJAX calls to static and inline files, like templates */
+ if (url.substr(0, 1) === '/' && url.substr(0, 8) !== '/bundles') {
@stof

stof May 5, 2014

Member

it would be good to allow users to configure the excluded patterns here. In my project, the assets are not placed in the bundle but in a dedicated folder of the web directory for instance.

Contributor

Burgov commented May 5, 2014

@stof like this? Burgov/symfony@de176ff

I'm not so sure about the option name though... Any suggestions?

Contributor

hice3000 commented May 5, 2014

Blacklist/Whitelist?

@wouterj wouterj commented on the diff May 5, 2014

...bProfilerBundle/DependencyInjection/Configuration.php
@@ -45,6 +45,7 @@ public function getConfigTreeBuilder()
->end()
->end()
->booleanNode('intercept_redirects')->defaultFalse()->end()
+ ->scalarNode('excluded_ajax_paths')->defaultValue('^/bundles|^/_wdt')->end()
@wouterj

wouterj May 5, 2014

Member

I would allow an array and then implode using |. E.g:

->scalarNode('excluded_ajax_paths')
    ->beforeNormalization()
        ->ifArray()->end()
        ->then(function ($v) { return implode('|', $v); }
    ->end()
    ->defaultValue('^/bundles|^/_wdt')->end()
->end()
@fabpot

fabpot Sep 22, 2014

Owner

I would keep the regex but rename the configuration option to something along the lines of excluded_ajax_pattern

@fabpot

fabpot Sep 22, 2014

Owner

Do we really need this configuration anyway? Excluding the web debug toolbar could be done by default.

@Burgov

Burgov Sep 22, 2014

Contributor

I added this because when using angular, it would show the loading of all templates in the table (which are often in the /bundles dir), which is rather useless

Contributor

raziel057 commented May 7, 2014

+1 for this excellent new debugging feature! Could you add a little space between the tooltip title and the table?

I'm totally agree with @wouterj with this remark concerning the icon.

I propose to replace with this one: ajax_icon or this one: ajax_icon_alt or even: ajax_icon_alt_2

Do you limit the number of request visible in the box?

Thanks for your work!

Thanks for that great feature
Does it log ajax request made by hinclude.js ?http://symfony.com/doc/current/book/templating.html#asynchronous-content-with-hinclude-js

Contributor

staabm commented May 8, 2014

it monkey patches XHR, so technically everything which produces XHR Requests should be logged:

see https://github.com/symfony/symfony/pull/8896/files#diff-de00cc2fe56e59a48a4d26e7a0dccca3R169

thx for clarification @staabm

Contributor

Burgov commented May 8, 2014

@raziel057 thanks for the images. I personally prefer the last one. Can you supply the png with a transparent BG please?

There's no limit on the requests yet, but I do think a limit of around 20 should make sense

Contributor

hice3000 commented May 8, 2014

Maybe you should try to remove the space between the table and the pop-up border, that should look a lot nicer.

----- Reply message -----
Von: "Bart van den Burg" notifications@github.com
An: "symfony/symfony" symfony@noreply.github.com
Cc: "Malte N" malte.n@live.de
Betreff: [symfony] [WIP][WebProfilerBundle] Show AJAX requests in the symfony profiler toolbar (#8896)
Datum: Do., Mai 8, 2014 13:44

@raziel057 thanks for the images. I personally prefer the last one. Can you supply the png with a transparent BG please?

There's no limit on the requests yet, but I do think a limit of around 20 should make sense


Reply to this email directly or view it on GitHub:
#8896 (comment)

Contributor

sstok commented May 8, 2014

I personally prefer the second one, it indicates a round-trip which is basically what an Ajax request is.
The last one looks more like an independent download/upload.

Nice work.

@sstok sstok commented on an outdated diff May 8, 2014

...ilerBundle/Resources/views/Profiler/base_js.html.twig
+
+ renderAjaxRequests = function() {
+ var requestCounter = document.getElementsByClassName('sf-toolbar-ajax-requests');
+ if (!requestCounter.length) {
+ return;
+ }
+
+ var tbodies = document.getElementsByClassName('sf-toolbar-ajax-request-list');
+ var state = 'ok';
+ if (tbodies.length) {
+ var tbody = tbodies[0];
+
+ var rows = document.createDocumentFragment();
+
+ if (requestStack.length) {
+ for (var i = 0; i < requestStack.length; i++) {
@sstok

sstok May 8, 2014

Contributor

this should be remembered for iterations.

for (var i = 0, requestStackCount = requestStack.length; i < requestStackCount ; i++) {

Contributor

raziel057 commented May 8, 2014

Please find the icons with a transparent background: ajax_icon ajax_icon_alt ajax_icon_alt_2

Contributor

sstok commented May 9, 2014

@raziel057 There all transparent right?

Contributor

raziel057 commented May 9, 2014

@sstok yes

Contributor

Burgov commented May 9, 2014

@hice3000 like this?

image

Contributor

Burgov commented May 9, 2014

@sstok thinking about it I agree with you. I've updated the image (see screenshot above)

Member

wouterj commented May 9, 2014

Hmm, maybe I get annoying, but isn't it too high? :) (higher than the others?)

Contributor

hice3000 commented May 9, 2014

It depends on the amount of requests, doesn't it? I'd like this more than a scrollbar on the right.

----- Reply message -----
Von: "Wouter J" notifications@github.com
An: "symfony/symfony" symfony@noreply.github.com
Cc: "Malte N" malte.n@live.de
Betreff: [symfony] [WIP][WebProfilerBundle] Show AJAX requests in the symfony profiler toolbar (#8896)
Datum: Fr., Mai 9, 2014 13:38

Hmm, maybe I get annoying, but isn't it too high? :) (higher than the others?)


Reply to this email directly or view it on GitHub:
#8896 (comment)

Contributor

raziel057 commented May 9, 2014

@wouterj I thought it was the good size but it can be prettiest with this smaller image:

ajax_icon

Contributor

yguedidi commented May 9, 2014

@raziel057 👍

@Taluu Taluu and 4 others commented on an outdated diff May 9, 2014

...filerBundle/EventListener/WebDebugToolbarListener.php
- public function __construct(\Twig_Environment $twig, $interceptRedirects = false, $mode = self::ENABLED, $position = 'bottom', UrlGeneratorInterface $urlGenerator = null)
+ public function __construct(\Twig_Environment $twig, $interceptRedirects = false, $mode = self::ENABLED, $position = 'bottom', $excludedAjaxPaths = '^/bundles|^/_wdt', UrlGeneratorInterface $urlGenerator = null)
@Taluu

Taluu May 9, 2014

Contributor

This is a BC Break ; you should add it at the end of the parameters

@wouterj

wouterj May 9, 2014

Member

Even then it's a BC break.

@Taluu

Taluu May 9, 2014

Contributor

How would it be a BC Break if it is at the end of the parameters ? As it has a default value... Other applications (or extensions) may still call it the usual way

@wouterj

wouterj May 9, 2014

Member

If you extend the class, you need to have the same signature. So you basically can't change any signature.

But I just looked and saw that this particular BC break is allowed here, since it's not an API method. (and I mean your suggestion, the current state is not allowed) However, it needs to be documented in the UPGRADE file. http://symfony.com/bc

@Taluu

Taluu May 9, 2014

Contributor

Not as a constructor. You may have a different constructor for your child classes :

<?php

class Foo {
     private $a, $b;

    public function __construct($a, $b) {
        $this->a = $a;
        $this->b = $b;
    }
}

class Bar extends Foo {
     public function __construct($a) {
         parent::__construct($a, 'foo');
     }
}

Works perfectly fine

@Burgov

Burgov May 26, 2014

Contributor

so what's the conclusion? Move the parameter to the end and that's enough?

@fabpot

fabpot Sep 22, 2014

Owner

Yes, you need to move it to the end.

Contributor

hice3000 commented May 9, 2014

@burgov that's exactly what I meant. Looks better, doesn't it?

Contributor

raziel057 commented May 17, 2014

Can we hope to have this PR merged for 2.5?

Owner

fabpot commented May 17, 2014

@raziel057 No, as this is a new feature and feature freeze for 2.5 happened a month and a half ago.

Contributor

raziel057 commented May 17, 2014

@fabpot ok so I hope it will be for 2.6 ;)

Contributor

Burgov commented May 26, 2014

@hice3000 I can't say I prefer one over the other, so I'm fine with it

@mykiwi mykiwi commented on an outdated diff Aug 8, 2014

...e/FrameworkBundle/DataCollector/AjaxDataCollector.php
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\DataCollector\DataCollector;
+
+/**
+ * AjaxDataCollector.
+ *
+ * @author Bart van den Burg <bart@burgov.nl>
+ */
+class AjaxDataCollector extends DataCollector
+{
+
+ public function collect(Request $request, Response $response, \Exception $exception = null)
+ {
+ // all collecting is done client side
@mykiwi

mykiwi Aug 8, 2014

Contributor

You should write all collecting is done on client side.

Member

weaverryan commented Aug 8, 2014

This should get a DX label - I really like this feature and would love to see it for 2.6.

Contributor

mykiwi commented Aug 8, 2014

👍 Agree with you @weaverryan

Member

wouterj commented Aug 9, 2014

@weaverryan I think that's misusing the DX label. We shouldn't tag all features which one of the DX people like as DX, we should only tag issues as DX when the really improve the experience. And that's a really vague term, since each new feature improves the experience... Maybe we have to make a clear definition of DX.

Contributor

ricardclau commented Aug 21, 2014

Just wanted to thank everyone involved in this amazing feature. Really like it! Well done!

barryvdh referenced this pull request in maximebf/php-debugbar Sep 1, 2014

Closed

Override XMLHttpRequest.prototype.open for ajax calls #156

Member

weaverryan commented Sep 22, 2014

Are there any pending issues or concerns with this PR? If there's agreement on it, I'd love to see this for 2.6.

Thanks!

Owner

fabpot commented Sep 22, 2014

I'm going to play with it tonight...

@fabpot fabpot commented on an outdated diff Sep 22, 2014

...e/FrameworkBundle/DataCollector/AjaxDataCollector.php
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\DataCollector;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\DataCollector\DataCollector;
+
+/**
+ * AjaxDataCollector.
+ *
+ * @author Bart van den Burg <bart@burgov.nl>
+ */
+class AjaxDataCollector extends DataCollector
+{
+
@fabpot

fabpot Sep 22, 2014

Owner

empty line should be removed.

@fabpot fabpot commented on an outdated diff Sep 22, 2014

...e/FrameworkBundle/DataCollector/AjaxDataCollector.php
+ *
+ * @author Bart van den Burg <bart@burgov.nl>
+ */
+class AjaxDataCollector extends DataCollector
+{
+
+ public function collect(Request $request, Response $response, \Exception $exception = null)
+ {
+ // all collecting is done client side
+ }
+
+ public function getName()
+ {
+ return 'ajax';
+ }
+
@fabpot

fabpot Sep 22, 2014

Owner

empty line should be removed.

@Burgov Burgov commented on the diff Sep 22, 2014

...e/WebProfilerBundle/Controller/ProfilerController.php
@@ -108,6 +108,7 @@ public function panelAction(Request $request, $token)
'request' => $request,
'templates' => $this->getTemplateManager()->getTemplates($profile),
'is_ajax' => $request->isXmlHttpRequest(),
+ 'excluded_ajax_paths' => null
@Burgov

Burgov Sep 22, 2014

Contributor

I had to add this line just now to fix the fact that the profiler itself could no longer be opened. Probably not the cleanest solution, but works for now... The whole AJAX icon has no business in the toolbar in the profiler itself anyway, but removing it can't be done easily, that is without rewriting stuff

Suggestions?

@fabpot

fabpot Sep 23, 2014

Owner

fixed in my fork

Bart van den Burg and others added some commits Aug 30, 2013

@Burgov Bart van den Burg WIP 761a172
@Burgov Burgov complete the css for all modern browsers bf9cda2
@Burgov Burgov - don't use id's in favor of classes
- use native Javascript listener
- include JS as soon as possible
- show ms instead of s
- fixes to the output HTML
- generate the URL to the profiler
280fce1
@Burgov Burgov show the number as "loading" if there is at least one request loading.
show the number as "error" if any of the last three requests has failed.
f817f62
@Burgov Bart van den Burg added license header 12c994b
@Burgov Bart van den Burg renamed updateAjaxRequests => renderAjaxRequests 46e0810
@Burgov Bart van den Burg added apostrophes e01526f
@Burgov Bart van den Burg got rid of global variables 8662309
@Burgov Bart van den Burg put W3C alternative last to overwrite prefixed version in browsers su…
…pporting both instead of the opposite
aed75ee
@Burgov Bart van den Burg requestStack needs no exposure f18473e
@Burgov Bart van den Burg moved createProxy logic outside load method 4e1d915
@Burgov Bart van den Burg prefix css stuff with "sf-" 396fdf2
@Burgov Bart van den Burg minor fixes 49f8c13
@Burgov Bart van den Burg make use of the new X-Debug-Token-Link response header 938de6b
@Burgov Bart van den Burg implement documentFragment in JS 31ca896
@Burgov Burgov Created an icon for the toolbar 5205a3b
@Burgov Burgov stop icon from linking 0645428
@Burgov Burgov used substr instead of charAt for older browsers 2d5aef7
@Burgov Burgov max Method+URL width: 300px 889c921
@Burgov Burgov WouterJ's suggestions 171794b
@Burgov Burgov stloyd's suggestions 3d94ff8
@Burgov Burgov safer method of iterating 059a63d
@Burgov Burgov fixed typo 3038b6d
@Burgov Burgov more verbose prototype method call 42f85cd
@Burgov Burgov fix rendering when there are no requests yet
and more logical variable name
72d2d9b
@Burgov Burgov as per @stof: make exclusion path configurable f3364a4
@Burgov Burgov limit to displaying 20 requests and add some spacing around text abov…
…e table
5eed50e
@Burgov Burgov updated the image 15b7925
@Burgov Burgov remove spacing around the table dde4061
@Burgov Burgov cs fixes ce10fac
@Burgov Burgov fix error in profiler c0f2938
@Burgov Burgov fixed tests
bfd22b6
Owner

fabpot commented Sep 23, 2014

I've tweaked the CSS a bit to make things more consistent with the other toolbar items.

Before

image

After

image

Owner

fabpot commented Sep 23, 2014

We are almost there! I've created #12000 to finish this PR with some minor tweaks. Thanks @burgov

fabpot closed this Sep 23, 2014

Contributor

Burgov commented Sep 23, 2014

👍!

@fabpot fabpot added a commit that referenced this pull request Sep 24, 2014

@fabpot fabpot feature #12000 [WebProfilerBundle] Show AJAX requests in the symfony …
…profiler toolbar (Burgov, fabpot, stof)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[WebProfilerBundle] Show AJAX requests in the symfony profiler toolbar

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

Adds AJAX requests in the web debug toolbar.
See #8896 for the original discussion.

![image](https://cloud.githubusercontent.com/assets/47313/4384087/43d1feb2-43b0-11e4-99c9-3e50e19e623f.png)

Commits
-------

16d1b35 optimized JS for the AJAX section of the toolbar
2e708d7 made minor tweaks to JS code
8e4c603 replaced the AJAX icon with a smaller one
b66f39a removed hack
9c74fcc removed uneeded web_profiler.debug_toolbar.excluded_ajax_paths parameter in the container
d43edaf [WebProfilerBundle] improved the ajax section of the WDT
37f7dd7 [WebProfilerBundle] Show AJAX requests in the symfony profiler toolbar
c2e3ee8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment