New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into
base: master
from

Conversation

@ro0NL
Contributor

ro0NL commented Apr 15, 2017

Q A
Branch? 4.1
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

TLDR; imply a "forwarded" request in the profiler if one returns a response with a x-debug-token set. Otherwise dont.


Currently a forward request is implied by the WDT/profiler based on the latest sub-request made, however the main request can return it's own response, or one from a non-latest sub-request. The current behavior is a bit misleading imo.

public function indexAction(Request $request)
{
    $bar1 = $this->forward(__CLASS__.'::barAction');
    $bar2 = $this->forward(__CLASS__.'::bar2Action');
    return $bar1;
}

It shows the request was forwarded to bar2Action. This changes that, so that barAction is shown instead. No forward is implied if a new response was returned by indexAction.

image

Note we dont really need the collector in the framework bundle anymore with this approach. deprecated it.

@aitboudad

The tag kernel.event_subscriber can be removed for RequestDataCollector

Show outdated Hide outdated src/Symfony/Component/HttpKernel/Profiler/Profile.php
*
* @return Profile|null
*/
public function getChildByToken($token)

This comment has been minimized.

@aitboudad

aitboudad Apr 15, 2017

Contributor

unused method

@aitboudad

aitboudad Apr 15, 2017

Contributor

unused method

This comment has been minimized.

@ro0NL

ro0NL Apr 15, 2017

Contributor

It's used in twig ;-)

@ro0NL

ro0NL Apr 15, 2017

Contributor

It's used in twig ;-)

@aitboudad

This comment has been minimized.

Show comment
Hide comment
@aitboudad

aitboudad Apr 15, 2017

Contributor

@ro0NL the forward icon doesn't appear anymore, can you check why?
selection_084

Contributor

aitboudad commented Apr 15, 2017

@ro0NL the forward icon doesn't appear anymore, can you check why?
selection_084

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Apr 15, 2017

Contributor

Not sure.. what code are you referring to? Assuming symfony-demo.. i installed the latest version but i dont get a forward; or im missing it somewhere else in the app.

Contributor

ro0NL commented Apr 15, 2017

Not sure.. what code are you referring to? Assuming symfony-demo.. i installed the latest version but i dont get a forward; or im missing it somewhere else in the app.

@aitboudad

This comment has been minimized.

Show comment
Hide comment
@aitboudad

aitboudad Apr 15, 2017

Contributor

I see now what's happen and but I'm not sure if it's expected or not.
Before, the forward icon is displayed if at least one request is forwarded and it doesn't matter if main request is forwarded or not But Now it appear only if the main request is forwarded.
Try with this:

public function indexAction()
{
    $bar1 = $this->forward(__CLASS__.'::barAction');
    $bar2 = $this->forward(__CLASS__.'::bar2Action');
    return $this->render('index.html.twig');
}
Contributor

aitboudad commented Apr 15, 2017

I see now what's happen and but I'm not sure if it's expected or not.
Before, the forward icon is displayed if at least one request is forwarded and it doesn't matter if main request is forwarded or not But Now it appear only if the main request is forwarded.
Try with this:

public function indexAction()
{
    $bar1 = $this->forward(__CLASS__.'::barAction');
    $bar2 = $this->forward(__CLASS__.'::bar2Action');
    return $this->render('index.html.twig');
}
@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Apr 15, 2017

Contributor

Imo. the arrow / "Forwarded to" implies the request was as-is forwarded to a sub-request, this is why i think the current behavior is misleading. In your example indexAction returns it's own response, it's not forwarded.

Note the sub-requests are still available in the request panel, nothing changed with that.

Contributor

ro0NL commented Apr 15, 2017

Imo. the arrow / "Forwarded to" implies the request was as-is forwarded to a sub-request, this is why i think the current behavior is misleading. In your example indexAction returns it's own response, it's not forwarded.

Note the sub-requests are still available in the request panel, nothing changed with that.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Apr 20, 2017

@stof stof added this to WIP in Profiler panels Apr 30, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 12, 2017

Member

rebase needed
ping @javiereguiluz also since this is "profiler panel" related

Member

nicolas-grekas commented Jul 12, 2017

rebase needed
ping @javiereguiluz also since this is "profiler panel" related

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jul 12, 2017

Member

this should be done against 3.4, not master, when rebasing (no need to reopen a different PR, as Github allows changing the branch)

Member

stof commented Jul 12, 2017

this should be done against 3.4, not master, when rebasing (no need to reopen a different PR, as Github allows changing the branch)

@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 Jul 12, 2017

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Jul 31, 2017

Member

I don't get the use case, isn't "forward" meant to return "the" response?

This looks really weird to handle this. The forward feature should not be confused with making standard sub requests IMO (with were already profiled before #17589 btw).

Member

HeahDude commented Jul 31, 2017

I don't get the use case, isn't "forward" meant to return "the" response?

This looks really weird to handle this. The forward feature should not be confused with making standard sub requests IMO (with were already profiled before #17589 btw).

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Aug 6, 2017

Contributor

Well.. imo. whats weird is the profiler assumes a forward request to the last sub request, whereas we may forward to the first sub request.

Contributor

ro0NL commented Aug 6, 2017

Well.. imo. whats weird is the profiler assumes a forward request to the last sub request, whereas we may forward to the first sub request.

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Aug 6, 2017

Member

the profiler assumes a forward request to the last sub request

This is not exact. The last forwarded request is considered since "forwarded" actions could be chained.

Member

HeahDude commented Aug 6, 2017

the profiler assumes a forward request to the last sub request

This is not exact. The last forwarded request is considered since "forwarded" actions could be chained.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Aug 6, 2017

Contributor

Yet we imply a forward to bar2Action whereas you agree it should be bar1Action right?

The last forwarded request is considered since "forwarded" actions could be chained.

You mean if A > B > C the profiler currently shows C for A? Indeed this PR would show B for A (and C for B).

Contributor

ro0NL commented Aug 6, 2017

Yet we imply a forward to bar2Action whereas you agree it should be bar1Action right?

The last forwarded request is considered since "forwarded" actions could be chained.

You mean if A > B > C the profiler currently shows C for A? Indeed this PR would show B for A (and C for B).

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Aug 6, 2017

Member

Sorry for the short answer above, let me try to explain myself better.

Your case:

public function indexAction(Request $request)
{
    $bar1 = $this->forward(__CLASS__.'::barAction');
    $bar2 = $this->forward(__CLASS__.'::bar2Action');

    return $bar1;
}

is not a valid use case for forwarding IMHO, and such cannot be profiled correctly.

What I assumed when working on #17589 is:

class FooController
{
    public function indexAction(Request $request)
    {
        return $this->forward('BarController::barAction');
    }
}

class BarController
{
    public function barAction(Request $request)
    {
        return $this->forward('BazController::bazAction');
    }
}

Would show the bazAction profile in the toolbar as forwarded from barAction (with a link to its profile) and this las one as forwarded from indexAction.

So A > B > C, would show the C profile in the toolbar with a link to B that links A.

Member

HeahDude commented Aug 6, 2017

Sorry for the short answer above, let me try to explain myself better.

Your case:

public function indexAction(Request $request)
{
    $bar1 = $this->forward(__CLASS__.'::barAction');
    $bar2 = $this->forward(__CLASS__.'::bar2Action');

    return $bar1;
}

is not a valid use case for forwarding IMHO, and such cannot be profiled correctly.

What I assumed when working on #17589 is:

class FooController
{
    public function indexAction(Request $request)
    {
        return $this->forward('BarController::barAction');
    }
}

class BarController
{
    public function barAction(Request $request)
    {
        return $this->forward('BazController::bazAction');
    }
}

Would show the bazAction profile in the toolbar as forwarded from barAction (with a link to its profile) and this las one as forwarded from indexAction.

So A > B > C, would show the C profile in the toolbar with a link to B that links A.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Aug 6, 2017

Contributor

is not a valid use case for forwarding IMHO, and such cannot be profiled correctly.

Agree it's a bit edgy, but this PR does fix it...

So A > B > C, would show the C profile in the toolbar with a link to B that links A.

Tested it and currently A shows B, and B shows C. So no change there.

Contributor

ro0NL commented Aug 6, 2017

is not a valid use case for forwarding IMHO, and such cannot be profiled correctly.

Agree it's a bit edgy, but this PR does fix it...

So A > B > C, would show the C profile in the toolbar with a link to B that links A.

Tested it and currently A shows B, and B shows C. So no change there.

@HeahDude

@ro0NL you're right, this change does not harm, but I left a question and I guess this attribute is not needed anymore.

@@ -178,6 +178,10 @@ public function collect(Request $request, Response $response, \Exception $except
$profile->setIp('Unknown');
}
if ($prevToken = $response->headers->get('X-Debug-Token')) {

This comment has been minimized.

@HeahDude

HeahDude Aug 31, 2017

Member

Doesn't this make any sub request a forwarded one?

@HeahDude

HeahDude Aug 31, 2017

Member

Doesn't this make any sub request a forwarded one?

This comment has been minimized.

@ro0NL

ro0NL Aug 31, 2017

Contributor

Yes :) basically if the response has a x-debug-token header already, we assume a forwarded request. Thats the trick here.

@ro0NL

ro0NL Aug 31, 2017

Contributor

Yes :) basically if the response has a x-debug-token header already, we assume a forwarded request. Thats the trick here.

This comment has been minimized.

@ro0NL

ro0NL Aug 31, 2017

Contributor

And thus if we have a X-Previous-Debug-Token in RequestDataCollector we know it was forwarded (it had a x-debug-token previously).

@ro0NL

ro0NL Aug 31, 2017

Contributor

And thus if we have a X-Previous-Debug-Token in RequestDataCollector we know it was forwarded (it had a x-debug-token previously).

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 2, 2017

Contributor

@HeahDude good catch, ive removed the _forwarded attribute.

@javiereguiluz could you have a look? Is the change clear for you?

Contributor

ro0NL commented Sep 2, 2017

@HeahDude good catch, ive removed the _forwarded attribute.

@javiereguiluz could you have a look? Is the change clear for you?

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 12, 2017

Member

@ro0NL the example you showed in the description is very very edgy to me. If this PR solves it, that's great ... but I wouldn't mind if that remains unsolved.

My concern is this comment from @HeahDude:

Doesn't this make any sub request a forwarded one?

If that's true ... isn't it wrong? We've never considered sub-request as forwarded requests, right?

Member

javiereguiluz commented Sep 12, 2017

@ro0NL the example you showed in the description is very very edgy to me. If this PR solves it, that's great ... but I wouldn't mind if that remains unsolved.

My concern is this comment from @HeahDude:

Doesn't this make any sub request a forwarded one?

If that's true ... isn't it wrong? We've never considered sub-request as forwarded requests, right?

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 12, 2017

Contributor

but I wouldn't mind if that remains unsolved.

Same, but experienced this issue (once now :)) when additionally doing a sub request (just to trigger some logging, ignoring it's response). Yet the forwarded response was requested before that, the one we return. Might be solved by changing it's order on our side, sure :) (well.. not sure really, but i could try).

However, the case, made me think of different approach, this PR.

If that's true ... isn't it wrong?

Not really, every sub request simply gets a X-Debug-Token. Nothing new here.

Thus when we return a sub request we basically return a response which already has a X-Debug-Token (otherwise this wont happen, as the profiler assigns it).

Now the profiler is updated to check if it has X-Debug-Token beforehand, if so, that becomes X-Debug-Previous-Token and again assigns a new X-Debug-Token. So only in this case we have 2 debug tokens.

Consequently if we have a X-Debug-Previous-Token we know we returned a forwarded response.

That's robust to me.

Contributor

ro0NL commented Sep 12, 2017

but I wouldn't mind if that remains unsolved.

Same, but experienced this issue (once now :)) when additionally doing a sub request (just to trigger some logging, ignoring it's response). Yet the forwarded response was requested before that, the one we return. Might be solved by changing it's order on our side, sure :) (well.. not sure really, but i could try).

However, the case, made me think of different approach, this PR.

If that's true ... isn't it wrong?

Not really, every sub request simply gets a X-Debug-Token. Nothing new here.

Thus when we return a sub request we basically return a response which already has a X-Debug-Token (otherwise this wont happen, as the profiler assigns it).

Now the profiler is updated to check if it has X-Debug-Token beforehand, if so, that becomes X-Debug-Previous-Token and again assigns a new X-Debug-Token. So only in this case we have 2 debug tokens.

Consequently if we have a X-Debug-Previous-Token we know we returned a forwarded response.

That's robust to me.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 12, 2017

Contributor

@javiereguiluz other fix is it also works if you dont use forward(), ie dont extend from base controller. Thats nice actually.

Contributor

ro0NL commented Sep 12, 2017

@javiereguiluz other fix is it also works if you dont use forward(), ie dont extend from base controller. Thats nice actually.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 26, 2017

Member

@ro0NL rebase needed again sorry

Member

nicolas-grekas commented Sep 26, 2017

@ro0NL rebase needed again sorry

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 8, 2017

Member

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

Member

nicolas-grekas commented Oct 8, 2017

Moving to 4.1. Rebase on master might be needed, where PHP 7.1 features can be used btw.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 7, 2018

Member

LGTM. Needs to be reworked to take into account that this will be merged into 4.1 (mostly deprecation messages).

Member

fabpot commented Feb 7, 2018

LGTM. Needs to be reworked to take into account that this will be merged into 4.1 (mostly deprecation messages).

@ro0NL ro0NL changed the base branch from 3.4 to master Feb 9, 2018

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Feb 9, 2018

Contributor

@fabpot good to go. However something else is bugging me:

image

(on master in case of a forward request)

The token differs, the link goes to the actual subrequest made (which is the right one here actually :D)

image

f561d2 should be 5a8b10 here (which is what this PR solves)

So i dont understand why x-debug-token-link resolves different currently 🤔 moreover shouldnt it just be the current token instead?

Also not sure if you want me to apply fabbot.io patch..

edit: X-Debug-Token-Link is only set wrong in the profiler. THe actual HTTP header value is indeed linking to the current token.

Contributor

ro0NL commented Feb 9, 2018

@fabpot good to go. However something else is bugging me:

image

(on master in case of a forward request)

The token differs, the link goes to the actual subrequest made (which is the right one here actually :D)

image

f561d2 should be 5a8b10 here (which is what this PR solves)

So i dont understand why x-debug-token-link resolves different currently 🤔 moreover shouldnt it just be the current token instead?

Also not sure if you want me to apply fabbot.io patch..

edit: X-Debug-Token-Link is only set wrong in the profiler. THe actual HTTP header value is indeed linking to the current token.

@ro0NL ro0NL changed the title from [WebProfilerBundle] Indicate forward controller based on response headers to [WebProfilerBundle] Imply forward request by a new X-Previous-Debug-Token header Feb 9, 2018

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 12, 2018

Member

@ro0NL The WebProfiler uses new methods on the data collector class, so it looks like some Composer constraints need to be adjusted so that the web profiler does not allow using an older version of the HTTP Kernel component.

Member

fabpot commented Feb 12, 2018

@ro0NL The WebProfiler uses new methods on the data collector class, so it looks like some Composer constraints need to be adjusted so that the web profiler does not allow using an older version of the HTTP Kernel component.

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Feb 12, 2018

Contributor

Correct, i confused deps with framework bundle. Means safety checks in templates (collector.forwardtoken|default(null)) are actually not needed 🎉

Now updated. Ill have a look at wrong X-Debug-Token-Link in profiler soonish, that's unrelated.

Contributor

ro0NL commented Feb 12, 2018

Correct, i confused deps with framework bundle. Means safety checks in templates (collector.forwardtoken|default(null)) are actually not needed 🎉

Now updated. Ill have a look at wrong X-Debug-Token-Link in profiler soonish, that's unrelated.

@fabpot

fabpot approved these changes Feb 12, 2018

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 12, 2018

Member

Thank you @ro0NL.

Member

fabpot commented Feb 12, 2018

Thank you @ro0NL.

@fabpot fabpot closed this Feb 12, 2018

fabpot added a commit that referenced this pull request Feb 12, 2018

feature #22447 [WebProfilerBundle] Imply forward request by a new X-P…
…revious-Debug-Token header (ro0NL)

This PR was squashed before being merged into the 4.1-dev branch (closes #22447).

Discussion
----------

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

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

TLDR; imply a "forwarded" request in the profiler if one _returns_ a response with a x-debug-token set. Otherwise dont.
____

Currently a forward request is implied by the WDT/profiler based on the latest sub-request made, however the main request can return it's own response, or one from a non-latest sub-request. The current behavior is a bit misleading imo.

```php
public function indexAction(Request $request)
{
    $bar1 = $this->forward(__CLASS__.'::barAction');
    $bar2 = $this->forward(__CLASS__.'::bar2Action');
    return $bar1;
}
```

It shows the request was forwarded to `bar2Action`. This changes that, so that `barAction` is shown instead. No forward is implied if a new response was returned by `indexAction`.

![image](https://cloud.githubusercontent.com/assets/1047696/25064022/e24d999e-21f1-11e7-8f94-afa3fad7462f.png)

~~Note we dont really need the collector in the framework bundle anymore with this approach.~~ deprecated it.

Commits
-------

07dd09d [WebProfilerBundle] Imply forward request by a new X-Previous-Debug-Token header

@ro0NL ro0NL deleted the ro0NL:forwardrequest-check-response branch Feb 12, 2018

@stof stof moved this from WIP to DONE in Profiler panels Mar 30, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment