[Profiler] Ajax #3340

vicb · 2012-02-12T20:34:52Z

The first commit should be merged as app is not always accessible in the twig template due to the ways the templating system is used. Then there is currently no way to check if we are dealing with an ajax request in the view.

The second commit use ajax to load the panels. This should make the interface more responsive as you don't have to load the layout each time + the panels are cached. Loading via AJAX would also work if your panel does not extend the ajax layout (legacy support) - this would be less efficient though as you would load the layout and filter it out afterwards.

I am not sure if the second commit is worth merging, maybe it is useless ?

stof · 2012-02-12T20:40:16Z

@vicb please rebase

stof · 2012-02-12T20:41:54Z

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

@@ -53,6 +55,7 @@ public function panelAction($token)
            'panel'     => $panel,
            'page'      => $page,
            'templates' => $this->getTemplates($profiler),
+            'isajax'    => $request->isXmlHttpRequest()


please use a camel cased name: isAjax

@vicb this has not been fixed

it has been during my forced push: https://github.com/symfony/symfony/pull/3340/files#L1R59

ah, you changed it in the controller only, not in the templates

It was hard to refrain myself from doing this but the CS seem different there collector.calledlisteners, collector.notcalledlisteners, ...

really ? We use lowercased names in Twig everywhere instead of respecting the case ? @fabpot ?

My personal preference would be to rename this to is_ajax.

ah yeah, the Twig official CS are about using underscores.

stof · 2012-02-13T17:48:48Z

@vicb just FYI, this conflicts with master so you will need to rebased it before it can be merged.

Otherwise, what are the remaining points ?

vicb · 2012-02-13T17:57:27Z

I am still wondering if the second commit is a good idea or not ?

vicb · 2012-02-13T18:28:17Z

@stof isn't the branch based on the latest master ?

stof · 2012-02-13T19:32:52Z

Well, github tells me it cannot be merged automatically. so either there is a conflict, either their conflict detection failed last time you pushed.

vicb · 2012-02-13T22:20:06Z

I did fail.
Should be ok now.

fabpot · 2012-02-14T23:27:08Z

I'm -1 on the second commit.

vicb · 2012-02-15T07:44:25Z

Thanks all for the feedback.

@fabpot ready !

stof · 2012-02-15T07:46:53Z

@vicb not ready: you reverted all use of is_ajax in the templates (and you did not renamed it to the underscored name preferred by @fabpot)

vicb · 2012-02-15T07:54:30Z

Well I did revert the use of "isajax" (prefer not to mix CS here, the scope of this PR is not to fix CS) because it is not used (this should be applied to the Doctrine profiler).

What I mean is that isajax in all the Sf templates w/o the associated js is useless, basically all or nothing

vicb · 2012-02-15T08:26:41Z

btw @fabpot it makes me wonder if underscored variable names is a good idea, this will force us to mix (i.e. is_ajax vs request.isxmlhttprequest). What do you think ?

fabpot · 2012-02-15T10:09:20Z

I still prefer is_ajax as it makes things more readable.

vicb · 2012-02-15T10:16:13Z

At a larger scale how do fix the inconsistency described in my previous message ?
Options are:

fix twig cs
create twig cs specific to sf2
don't fix (= keep & live with some inconsistency)

stof · 2012-02-15T10:22:13Z

@vicb we also use underscores for variables used in the form themes. the official Twig CS are basically the one used by Sf2 in the form theme

fabpot · 2012-02-15T10:24:46Z

I don't see any inconsistencies here. One a variable name and the other is a method call/property name. So, my vote is a don't fix.

vicb · 2012-02-15T10:28:53Z

I agree but then we loose one advertised benefit a twig: "Easy to learn: The syntax is easy to learn and has been optimized to allow web designers to get their job done fast without getting in their way".

The designers should now be aware of the underlying implementation (i.e. Am I dealing with a variable or a function ?)

Edit: race condition here... I agree with @stof

stof · 2012-02-15T10:45:49Z

@vicb they see that isXmlHttpRequest is not a variable. They are accessing it on the request variable (well, recurse here to reach the variable)

fabpot · 2012-02-15T10:46:57Z

variables and functions are underscored.

vicb · 2012-02-15T10:51:28Z

I think that the beauty of Twig comes from the fact that designers do not have to wonder if "something" is an array / an object / a variable / a method / a property.
But never mind, I'll update the PR.

vicb · 2012-02-15T10:55:06Z

@fabpot would you mind if I open a PR against twig to check for existence of collector::getNotCalledListeners() when a designer writes collector.not_called_listeners, then we are all happy ?

vicb · 2012-02-15T11:21:55Z

ready !

… panels

fabpot · 2012-02-15T11:31:50Z

The problem is that the Twig_Template::getAttribute() is already the bottleneck

@vicb

Commits ------- ed028d5 [WebProfilerBundle] Made is_ajax available to the view when rendering panels Discussion ---------- [Profiler] Ajax The first commit should be merged as `app` is not always accessible in the twig template due to the ways the templating system is used. Then there is currently no way to check if we are dealing with an ajax request in the view. The second commit use ajax to load the panels. This should make the interface more responsive as you don't have to load the layout each time + the panels are cached. Loading via AJAX would also work if your panel does not extend the ajax layout (legacy support) - this would be less efficient though as you would load the layout and filter it out afterwards. I am not sure if the second commit is worth merging, maybe it is useless ? --------------------------------------------------------------------------- by stof at 2012-02-12T20:40:16Z @vicb please rebase --------------------------------------------------------------------------- by stof at 2012-02-13T17:48:48Z @vicb just FYI, this conflicts with master so you will need to rebased it before it can be merged. Otherwise, what are the remaining points ? --------------------------------------------------------------------------- by vicb at 2012-02-13T17:57:27Z I am still wondering if the second commit is a good idea or not ? --------------------------------------------------------------------------- by vicb at 2012-02-13T18:28:17Z @stof isn't the branch based on the latest master ? --------------------------------------------------------------------------- by stof at 2012-02-13T19:32:52Z Well, github tells me it cannot be merged automatically. so either there is a conflict, either their conflict detection failed last time you pushed. --------------------------------------------------------------------------- by vicb at 2012-02-13T22:20:06Z I did fail. Should be ok now. --------------------------------------------------------------------------- by fabpot at 2012-02-14T23:27:08Z I'm -1 on the second commit. --------------------------------------------------------------------------- by vicb at 2012-02-15T07:44:25Z Thanks all for the feedback. @fabpot ready ! --------------------------------------------------------------------------- by stof at 2012-02-15T07:46:53Z @vicb not ready: you reverted all use of ``is_ajax`` in the templates (and you did not renamed it to the underscored name preferred by @fabpot) --------------------------------------------------------------------------- by vicb at 2012-02-15T07:54:30Z Well I did revert the use of "`isajax`" (prefer not to mix CS here, the scope of this PR is not to fix CS) because it is not used (this should be applied to the Doctrine profiler). _What I mean is that `isajax` in all the Sf templates w/o the associated js is useless, basically all or nothing_ --------------------------------------------------------------------------- by vicb at 2012-02-15T08:26:41Z btw @fabpot it makes me wonder if underscored variable names is a good idea, this will force us to mix (i.e. `is_ajax` vs `request.isxmlhttprequest`). What do you think ? --------------------------------------------------------------------------- by fabpot at 2012-02-15T10:09:20Z I still prefer `is_ajax` as it makes things more readable. --------------------------------------------------------------------------- by vicb at 2012-02-15T10:16:13Z At a larger scale how do fix the inconsistency described in my previous message ? Options are: * fix twig cs * create twig cs specific to sf2 * don't fix (= keep & live with some inconsistency) --------------------------------------------------------------------------- by stof at 2012-02-15T10:22:13Z @vicb we also use underscores for variables used in the form themes. the official Twig CS are basically the one used by Sf2 in the form theme --------------------------------------------------------------------------- by fabpot at 2012-02-15T10:24:46Z I don't see any inconsistencies here. One a variable name and the other is a method call/property name. So, my vote is a don't fix. --------------------------------------------------------------------------- by vicb at 2012-02-15T10:28:53Z I agree but then we loose one advertised benefit a twig: _"Easy to learn: The syntax is easy to learn and has been optimized to allow web designers to get their job done fast without getting in their way"_. The designers should now be aware of the underlying implementation (i.e. Am I dealing with a variable or a function ?) Edit: race condition here... I agree with @stof --------------------------------------------------------------------------- by stof at 2012-02-15T10:45:49Z @vicb they see that ``isXmlHttpRequest`` is not a variable. They are accessing it on the ``request`` variable (well, recurse here to reach the variable) --------------------------------------------------------------------------- by fabpot at 2012-02-15T10:46:57Z variables and functions are underscored. --------------------------------------------------------------------------- by vi 10069 cb at 2012-02-15T10:51:28Z I think that the beauty of Twig comes from the fact that designers do not have to wonder if "something" is an array / an object / a variable / a method / a property. But never mind, I'll update the PR. --------------------------------------------------------------------------- by vicb at 2012-02-15T10:55:06Z @fabpot would you mind if I open a PR against twig to check for existence of `collector::getNotCalledListeners()` when a designer writes `collector.not_called_listeners`, then we are all happy ? --------------------------------------------------------------------------- by vicb at 2012-02-15T11:21:55Z ready ! --------------------------------------------------------------------------- by fabpot at 2012-02-15T11:31:50Z The problem is that the `Twig_Template::getAttribute()` is already the bottleneck

vicb · 2012-02-15T11:47:09Z

@fabpot I understand and as I am not a dsesigner I can leave with the current CS.

@vicb

Commits ------- ed028d5 [WebProfilerBundle] Made is_ajax available to the view when rendering panels Discussion ---------- [Profiler] Ajax The first commit should be merged as `app` is not always accessible in the twig template due to the ways the templating system is used. Then there is currently no way to check if we are dealing with an ajax request in the view. The second commit use ajax to load the panels. This should make the interface more responsive as you don't have to load the layout each time + the panels are cached. Loading via AJAX would also work if your panel does not extend the ajax layout (legacy support) - this would be less efficient though as you would load the layout and filter it out afterwards. I am not sure if the second commit is worth merging, maybe it is useless ? --------------------------------------------------------------------------- by stof at 2012-02-12T20:40:16Z @vicb please rebase --------------------------------------------------------------------------- by stof at 2012-02-13T17:48:48Z @vicb just FYI, this conflicts with master so you will need to rebased it before it can be merged. Otherwise, what are the remaining points ? --------------------------------------------------------------------------- by vicb at 2012-02-13T17:57:27Z I am still wondering if the second commit is a good idea or not ? --------------------------------------------------------------------------- by vicb at 2012-02-13T18:28:17Z @stof isn't the branch based on the latest master ? --------------------------------------------------------------------------- by stof at 2012-02-13T19:32:52Z Well, github tells me it cannot be merged automatically. so either there is a conflict, either their conflict detection failed last time you pushed. --------------------------------------------------------------------------- by vicb at 2012-02-13T22:20:06Z I did fail. Should be ok now. --------------------------------------------------------------------------- by fabpot at 2012-02-14T23:27:08Z I'm -1 on the second commit. --------------------------------------------------------------------------- by vicb at 2012-02-15T07:44:25Z Thanks all for the feedback. @fabpot ready ! --------------------------------------------------------------------------- by stof at 2012-02-15T07:46:53Z @vicb not ready: you reverted all use of ``is_ajax`` in the templates (and you did not renamed it to the underscored name preferred by @fabpot) --------------------------------------------------------------------------- by vicb at 2012-02-15T07:54:30Z Well I did revert the use of "`isajax`" (prefer not to mix CS here, the scope of this PR is not to fix CS) because it is not used (this should be applied to the Doctrine profiler). _What I mean is that `isajax` in all the Sf templates w/o the associated js is useless, basically all or nothing_ --------------------------------------------------------------------------- by vicb at 2012-02-15T08:26:41Z btw @fabpot it makes me wonder if underscored variable names is a good idea, this will force us to mix (i.e. `is_ajax` vs `request.isxmlhttprequest`). What do you think ? --------------------------------------------------------------------------- by fabpot at 2012-02-15T10:09:20Z I still prefer `is_ajax` as it makes things more readable. --------------------------------------------------------------------------- by vicb at 2012-02-15T10:16:13Z At a larger scale how do fix the inconsistency described in my previous message ? Options are: * fix twig cs * create twig cs specific to sf2 * don't fix (= keep & live with some inconsistency) --------------------------------------------------------------------------- by stof at 2012-02-15T10:22:13Z @vicb we also use underscores for variables used in the form themes. the official Twig CS are basically the one used by Sf2 in the form theme --------------------------------------------------------------------------- by fabpot at 2012-02-15T10:24:46Z I don't see any inconsistencies here. One a variable name and the other is a method call/property name. So, my vote is a don't fix. --------------------------------------------------------------------------- by vicb at 2012-02-15T10:28:53Z I agree but then we loose one advertised benefit a twig: _"Easy to learn: The syntax is easy to learn and has been optimized to allow web designers to get their job done fast without getting in their way"_. The designers should now be aware of the underlying implementation (i.e. Am I dealing with a variable or a function ?) Edit: race condition here... I agree with @stof --------------------------------------------------------------------------- by stof at 2012-02-15T10:45:49Z @vicb they see that ``isXmlHttpRequest`` is not a variable. They are accessing it on the ``request`` variable (well, recurse here to reach the variable) --------------------------------------------------------------------------- by fabpot at 2012-02-15T10:46:57Z variables and functions are underscored. --------------------------------------------------------------------------- by vicb at 2012-02-15T10:51:28Z I think that the beauty of Twig comes from the fact that designers do not have to wonder if "something" is an array / an object / a variable / a method / a property. But never mind, I'll update the PR. --------------------------------------------------------------------------- by vicb at 2012-02-15T10:55:06Z @fabpot would you mind if I open a PR against twig to check for existence of `collector::getNotCalledListeners()` when a designer writes `collector.not_called_listeners`, then we are all happy ? --------------------------------------------------------------------------- by vicb at 2012-02-15T11:21:55Z ready ! --------------------------------------------------------------------------- by fabpot at 2012-02-15T11:31:50Z The problem is that the `Twig_Template::getAttribute()` is already the bottleneck

stof reviewed Feb 12, 2012
View reviewed changes

[WebProfilerBundle] Made is_ajax available to the view when rendering…

ed028d5

… panels

fabpot merged commit ed028d5 into symfony:master Feb 15, 2012

stof mentioned this pull request Jan 8, 2013

[WIP][WebProfilerBundle] Load web profiler panels asynchronously #6616

Closed

Uh oh!

[Profiler] Ajax #3340

[Profiler] Ajax #3340

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!