[3.1] [WebProfilerBundle] [DX] Feature allow forward and redirection detection in wdt #17589

HeahDude · 2016-01-28T11:09:47Z

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

This PR allows to :

track explicit forward from \Symfony\Bundle\FrameWorkBundle\Controller\Controller in the web debug toolbar.
or pass a request attribute _forwarded with the current request attributes (an instance of ParameterBag) as value to your sub request before handling it.
see if you've been redirected (require session enabled)

When redirected you will see the name of the route (if any) and a link to the profile of the original request.

In case of forwarding, the name of the controller is a file link and next to it there is a direct link to the profile of the sub request.

This works pretty well in Silex too by registering SessionServiceProvider() for redirections or by providing this method for forwarding :

class App extends \Silex\Application
//  (php7 bootstrap) $app = new class extends \Silex\Application {
{
    public function forward($controller, array $path = array(), array $query = array()
    {
        if (!$this->booted) {
            throw new LogicException(sprintf('Method %s must be called from a controller.', __METHOD__));
        }

        $this->flush();

        $request = $this['request_stack']->getCurrentRequest();
        $path['_forwarded'] = $request->attributes;
        $path['_controller'] = $controller;
        $subRequest = $request->duplicate($query, null, $path);

        return $this['kernel']->handle($subRequest, HttpKernelInterface::SUB_REQUEST);
    }
}

javiereguiluz · 2016-01-28T11:15:41Z

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

+     * Gets the parsed forward controller.
+     *
+     * @return array|bool The parsed forward controller as an array of data
+     *                    with keys 'token' the forward profile token, and


It's nice to have good comments but I think you are revealing too many implementation details in this one.

stof · 2016-01-28T11:43:47Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

@@ -64,8 +64,10 @@ protected function generateUrl($route, $parameters = array(), $referenceType = U
     */
    protected function forward($controller, array $path = array(), array $query = array())
    {
+        $request = $this->container->get('request_stack')->getCurrentRequest();
+        $path['_forwarded'] = $request;


I don't think passing the whole parent request is a good idea. It adds a bunch of unrelated info in the subrequest attributes.

@stof It appears to me as the simplest way to keep the forward trace, fortunately it's just an object reference, any suggestion ? maybe pass only the attributes bag ?

HeahDude · 2016-01-29T10:15:28Z

Made some changes to allow catch redirections as well, and use only the master request attributes (ParameterBag) instead of the whole request to handle forward interception as @stof suggested.
Also updated title and description.
Now need some reviews, thanks.

HeahDude · 2016-01-30T02:09:49Z

I wonder if it would be useful to add the redirection code to quickly see if it's the same as expected

javiereguiluz · 2016-02-01T10:50:11Z

The designer just sent me the icons which match the style of the toolbar:

redirect.svg

<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M23.06,7.83L14,0.38a1.25,1.25,0,0,0-2,.89V4.09a13.61,13.61,0,0,1-2.2.61l-1.3.47C8,5.35,7.59,5.6,7.12,5.81l-0.69.35-0.72.45a10.62,10.62,0,0,0-1.41,1A13.22,13.22,0,0,0,3,8.82a15.31,15.31,0,0,0-1.13,1.46A17.63,17.63,0,0,0,1,11.93c-0.18.58-.34,1.16-0.48,1.71S0.45,14.76.43,15.29a10.2,10.2,0,0,0,.16,1.5,5.72,5.72,0,0,0,.33,1.34c0.14,0.41.26,0.82,0.42,1.19,0.37,0.71.67,1.38,1,1.94l1,1.46c0.32,0.41.63,0.75,0.87,1s0.51,0.09.43-.22-0.23-.75-0.35-1.23L4,20.69c-0.1-.58-0.09-1.22-0.14-1.86,0-.32.05-0.65,0.08-1a3.44,3.44,0,0,1,.16-1A6.44,6.44,0,0,1,4.41,16l0.41-.8c0.2-.22.38-0.44,0.55-0.65L6,14c0.23-.14.5-0.24,0.72-0.37a7.52,7.52,0,0,1,.79-0.25,4.48,4.48,0,0,1,.84-0.15l0.41-.06H9.22c0.3,0,.56,0,0.85,0l0.72,0.07a3.77,3.77,0,0,1,1.2.21v3.17a1.25,1.25,0,0,0,2,.89l9-7.45A1.46,1.46,0,0,0,23.06,7.83Z" style="fill:#aaa"/></svg>

forward.svg

<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M23.61,11.07L17.07,4.35A1.2,1.2,0,0,0,15,5.28V9H1.4A1.82,1.82,0,0,0,0,10.82v2.61A1.55,1.55,0,0,0,1.4,15H15v3.72a1.2,1.2,0,0,0,2.07.93l6.63-6.72A1.32,1.32,0,0,0,23.61,11.07Z" style="fill:#aaa"/></svg>

HeahDude · 2016-02-01T12:05:21Z

Thanks @javiereguiluz, committed and updated description screenshots.

HeahDude · 2016-02-01T12:42:56Z

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

+        if ($request->hasSession() && $response->isRedirect()) {
+            $request->getSession()->set('sf_redirect', array(
+                'token' => $response->headers->get('x-debug-token'),
+                'route' => $request->attributes->get('_route', 'n/a'),


What about collecting controller too and making the route in wdt info panel a link to it ?

We could also catch here the redirection code if we want to display it in wdt.

HeahDude · 2016-02-01T23:00:11Z

@javiereguiluz WDYT of a redirect arrow pointing left instead of right, it would be less confusing and consistent with mail buttons definition for example.

Here's a screenshot from Mail app in OS X :

javiereguiluz · 2016-02-02T09:48:48Z

@HeahDude I don't agree with the last proposal. The reason is that our toolbar displays "redirected to" and the mail app shows "redirected/replied from". The tip of the arrow should point to the route name because that route is the result of the redirection or forward.

200 -> @route can be read as "a 200 response redirected to @route".
200 <- @route can be read as "@route redirects to 200".

HeahDude · 2016-02-02T12:43:05Z

@javiereguiluz alright. WDYT about collecting the redirected controller and/or redirect status code ? (see my notes)

HeahDude · 2016-02-15T09:46:59Z

Hi @javiereguiluz, do I have to write some tests for this PR ?

Otherwise it is finished, I'm just waiting for some feedbacks on catching the redirection status code, and/or the route name which we've been redirected from.

Any lead ?

xabbuh · 2016-02-15T12:52:29Z

src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php

@@ -64,8 +64,10 @@ protected function generateUrl($route, $parameters = array(), $referenceType = U
     */
    protected function forward($controller, array $path = array(), array $query = array())
    {
+        $request = $this->container->get('request_stack')->getCurrentRequest();
+        $path['_forwarded'] = $request->attributes;


I think this should be moved to an event listener instead. Otherwise you couldn't track subrequests triggered by user code that doesn't use the base controller class.

@xabbuh, the point is not to catch subrequests as profiler already does, we can see them in the request panel.
This feature is about catching the "main" forward", using this special method, as it should be used only once in most use case to quickly click the profile token link of the original request in the wdt.

But I can try to go further, how would you imagine the presentation of sub requests token in the wdt ?

xabbuh · 2016-02-15T12:52:58Z

What when there is more than one redirection before the WDT is shown again?

HeahDude · 2016-02-15T13:36:04Z

@xabbuh for chained redirection see my comment above but I could add a link in the top panel of the profiler for the case where intercept_redirect is false.

HeahDude · 2016-02-15T13:39:28Z

So in that it could worth to catch controller and route info from the redirection response

HeahDude · 2016-03-31T09:04:14Z

:) So there is no squashing needed here ? 4f020b5 could be "fixup" in 227ac77 if we intend to keep it.

fabpot · 2016-03-31T09:09:50Z

@HeahDude If you want to rearrange the commits before merge, please do so.

HeahDude · 2016-03-31T09:19:47Z

@fabpot The question is, is it needed to keep them separately ?

fabpot · 2016-03-31T09:28:31Z

I don't mind keeping several commits if they make sense and it seems they do :)

HeahDude · 2016-03-31T09:30:20Z

Agreed :)

So we're done here 🎉

fabpot · 2016-03-31T09:33:08Z

Thank you @HeahDude.

…nd redirection detection in wdt (HeahDude) This PR was merged into the 3.1-dev branch. Discussion ---------- [3.1] [WebProfilerBundle] [DX] Feature allow forward and redirection detection in wdt | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14358, #17501 | License | MIT | Doc PR | ? This PR allows to : - track explicit forward from `\Symfony\Bundle\FrameWorkBundle\Controller\Controller` in the web debug toolbar. - or pass a request attribute `_forwarded` with the current request attributes (an instance of `ParameterBag`) as value to your sub request before handling it. - see if you've been redirected (require session enabled) When redirected you will see the name of the route (if any) and a link to the profile of the original request. ![redirect](https://cloud.githubusercontent.com/assets/10107633/12716952/9aacdcba-c8e4-11e5-9a64-d26fe27f1cae.jpg) In case of forwarding, the name of the controller is a file link and next to it there is a direct link to the profile of the sub request. ![forward](https://cloud.githubusercontent.com/assets/10107633/12716968/ba6b1fbc-c8e4-11e5-85fc-7f71969cb372.jpg) This works pretty well in __Silex__ too by registering `SessionServiceProvider()` for redirections or by providing this method for forwarding : ```php class App extends \Silex\Application // (php7 bootstrap) $app = new class extends \Silex\Application { { public function forward($controller, array $path = array(), array $query = array() { if (!$this->booted) { throw new LogicException(sprintf('Method %s must be called from a controller.', __METHOD__)); } $this->flush(); $request = $this['request_stack']->getCurrentRequest(); $path['_forwarded'] = $request->attributes; $path['_controller'] = $controller; $subRequest = $request->duplicate($query, null, $path); return $this['kernel']->handle($subRequest, HttpKernelInterface::SUB_REQUEST); } } ``` Commits ------- 0a0e8af [WebProfilerBundle] show the http method in wdt if not 'GET' 4f020b5 [FrameworkBundle] Extends the RequestDataCollector 227ac77 [WebProfilerBundle] [FrameworkBundle] profile forward controller action 0a1b284 [WebProfiler] [HttpKernel] profile redirections

HeahDude · 2016-03-31T09:39:18Z

I would like to thank you and all the community for sharing symfony :)

I've learned so much thanks to you all since I use it, merci!

rvanlaak · 2016-04-03T13:55:29Z

Missed the development process of this feature, but it looks awesome @HeahDude ! 🚀

This PR was merged into the 3.1-dev branch. Discussion ---------- changelogs missing features after #17589 | Q | A | ------------- | --- | Branch? | master | License | MIT Commits ------- 724fd3b updated changelogs after #17589

…ollector (HeahDude) This PR was merged into the 3.1-dev branch. Discussion ---------- [HttpKernel] tweaked redirection profiling in RequestDataCollector | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ - c8ba3b2 removes duplicated code forgotten in #17589 - a47d2e8 fixes the collecting of redirect data in first sub request instead of redirected master request. Commits ------- df19c14 use a request attribute flag for redirection profile b26cb6d [HttpKernel] added RequestDataCollector::onKernelResponse() c8ba3b2 [HttpKernel] remove legacy duplicated code

This PR was merged into the 3.1 branch. Discussion ---------- [Reference] change namespace to point to new class see symfony/symfony#17589 and #6568 (comment) Commits ------- 10cf8d6 change namespace to point to new class

This PR was squashed before being merged into the 2.0.x-dev branch (closes #91). Discussion ---------- Allow symfony 3.1 As I was updating my skeleton app, I found out the webprofiler did not support symfony 3.1 in composer.json. So I tried to update it. Everything works great, except for the web-profiler-bundle. This PR broke it: symfony/symfony#17589 I don't know the best way to fix that. Maybe you have an idea? (I'm not that familiar with the silex and symfony development) Commits ------- 0a09d2a Allow symfony 3.1

…(jakzal) This PR was merged into the 3.1 branch. Discussion ---------- [HttpKernel] Fix a regression in the RequestDataCollector | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19701 | License | MIT | Doc PR | - The regression was introduced by refactoring made as part of #17589 (if/else statements where rearranged). Commits ------- 57008ea [HttpKernel] Fix a regression in the RequestDataCollector 26b90e4 [HttpKernel] Refactor a RequestDataCollector test case to use a data provider

carsonbot added the Status: Needs Review label Jan 28, 2016

HeahDude mentioned this pull request Jan 28, 2016

Show forwarded controller/sub request on the web dev toolbar #14358

Closed

javiereguiluz reviewed Jan 28, 2016
View reviewed changes

javiereguiluz added WebProfilerBundle Feature labels Jan 28, 2016

stof reviewed Jan 28, 2016
View reviewed changes

HeahDude changed the title ~~[WIP] [WDT] [HttpKernel] [WebProfilerBundle] Feature allow forward detection from controller in wdt~~ [WIP] [WebProfilerBundle] Feature allow forward and redirection detection in wdt Jan 29, 2016

HeahDude reviewed Feb 1, 2016
View reviewed changes

javiereguiluz modified the milestone: 3.1 Feb 2, 2016

HeahDude force-pushed the feature-wdt_forward_detection branch from b6f795f to 411dd48 Compare February 15, 2016 00:47

HeahDude changed the title ~~[WIP] [WebProfilerBundle] Feature allow forward and redirection detection in wdt~~ [WebProfilerBundle] Feature allow forward and redirection detection in wdt Feb 15, 2016

xabbuh reviewed Feb 15, 2016
View reviewed changes

HeahDude changed the title ~~[WebProfilerBundle] Feature allow forward and redirection detection in wdt~~ [WebProfilerBundle] [DX] Feature allow forward and redirection detection in wdt Feb 15, 2016

javiereguiluz added Status: Reviewed and removed Status: Needs Work labels Mar 31, 2016

fabpot merged commit 0a0e8af into symfony:master Mar 31, 2016

HeahDude added a commit to HeahDude/symfony that referenced this pull request Apr 2, 2016

updated changelogs after symfony#17589

7ba26c7

HeahDude added a commit to HeahDude/symfony that referenced this pull request Apr 3, 2016

updated changelogs after symfony#17589

fc66e60

HeahDude added a commit to HeahDude/symfony that referenced this pull request Apr 3, 2016

updated changelogs after symfony#17589

26da79c

HeahDude added a commit to HeahDude/symfony that referenced this pull request Apr 6, 2016

updated changelogs after symfony#17589

724fd3b

HeahDude mentioned this pull request Apr 22, 2016

[HttpKernel] tweaked redirection profiling in RequestDataCollector #18618

Merged

fabpot mentioned this pull request May 13, 2016

Release v3.1.0-BETA1 #18776

Merged

HeahDude mentioned this pull request May 14, 2016

fix RequestDataCollector class namespace symfony/symfony-docs#6568

Merged

xabbuh mentioned this pull request May 15, 2016

[Reference] change namespace to point to new class symfony/symfony-docs#6569

Merged

This was referenced May 31, 2016

Allow symfony 3.1 silexphp/Silex-WebProfiler#91

Closed

Support symfony 3.1 silexphp/Silex-WebProfiler#92

Closed

jakzal mentioned this pull request Oct 18, 2016

[HttpKernel] Fix a regression in the RequestDataCollector #20239

Merged

HeahDude mentioned this pull request Jul 31, 2017

[WebProfilerBundle] Imply forward request by a new X-Previous-Debug-Token header #22447

Closed

ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018

updated changelogs after symfony#17589

20a3bd9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.1] [WebProfilerBundle] [DX] Feature allow forward and redirection detection in wdt #17589

[3.1] [WebProfilerBundle] [DX] Feature allow forward and redirection detection in wdt #17589

HeahDude commented Jan 28, 2016

javiereguiluz Jan 28, 2016

stof Jan 28, 2016

HeahDude Jan 28, 2016

HeahDude commented Jan 29, 2016

HeahDude commented Jan 30, 2016

javiereguiluz commented Feb 1, 2016

HeahDude commented Feb 1, 2016

HeahDude Feb 1, 2016

HeahDude Feb 1, 2016

HeahDude commented Feb 1, 2016

javiereguiluz commented Feb 2, 2016

HeahDude commented Feb 2, 2016

HeahDude commented Feb 15, 2016

xabbuh Feb 15, 2016

HeahDude Feb 15, 2016

xabbuh commented Feb 15, 2016

HeahDude commented Feb 15, 2016

HeahDude commented Feb 15, 2016

HeahDude commented Mar 31, 2016

fabpot commented Mar 31, 2016

HeahDude commented Mar 31, 2016

fabpot commented Mar 31, 2016

HeahDude commented Mar 31, 2016

fabpot commented Mar 31, 2016

HeahDude commented Mar 31, 2016

rvanlaak commented Apr 3, 2016

[3.1] [WebProfilerBundle] [DX] Feature allow forward and redirection detection in wdt #17589

[3.1] [WebProfilerBundle] [DX] Feature allow forward and redirection detection in wdt #17589

Conversation

HeahDude commented Jan 28, 2016

javiereguiluz Jan 28, 2016

Choose a reason for hiding this comment

stof Jan 28, 2016

Choose a reason for hiding this comment

HeahDude Jan 28, 2016

Choose a reason for hiding this comment

HeahDude commented Jan 29, 2016

HeahDude commented Jan 30, 2016

javiereguiluz commented Feb 1, 2016

HeahDude commented Feb 1, 2016

HeahDude Feb 1, 2016

Choose a reason for hiding this comment

HeahDude Feb 1, 2016

Choose a reason for hiding this comment

HeahDude commented Feb 1, 2016

javiereguiluz commented Feb 2, 2016

HeahDude commented Feb 2, 2016

HeahDude commented Feb 15, 2016

xabbuh Feb 15, 2016

Choose a reason for hiding this comment

HeahDude Feb 15, 2016

Choose a reason for hiding this comment

xabbuh commented Feb 15, 2016

HeahDude commented Feb 15, 2016

HeahDude commented Feb 15, 2016

HeahDude commented Mar 31, 2016

fabpot commented Mar 31, 2016

HeahDude commented Mar 31, 2016

fabpot commented Mar 31, 2016

HeahDude commented Mar 31, 2016

fabpot commented Mar 31, 2016

HeahDude commented Mar 31, 2016

rvanlaak commented Apr 3, 2016