Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[3.1] [WebProfilerBundle] [DX] Feature allow forward and redirection detection in wdt#17589
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's nice to have good comments but I think you are revealing too many implementation details in this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think passing the whole parent request is a good idea. It adds a bunch of unrelated info in the subrequest attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 commentedJan 29, 2016
Made some changes to allow catch redirections as well, and use only the master request attributes ( |
HeahDude commentedJan 30, 2016
I wonder if it would be useful to add the redirection code to quickly see if it's the same as expected |
javiereguiluz commentedFeb 1, 2016
The designer just sent me the icons which match the style of the toolbar:
<svgxmlns="http://www.w3.org/2000/svg"width="24"height="24"viewBox="0 0 24 24"><pathd="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>
<svgxmlns="http://www.w3.org/2000/svg"width="24"height="24"viewBox="0 0 24 24"><pathd="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 commentedFeb 1, 2016
Thanks@javiereguiluz, committed and updated description screenshots. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about collecting controller too and making the route in wdt info panel a link to it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could also catch here the redirection code if we want to display it in wdt.
HeahDude commentedFeb 1, 2016
@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 commentedFeb 2, 2016
@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. |
HeahDude commentedFeb 2, 2016
@javiereguiluz alright. WDYT about collecting the redirected controller and/or redirect status code ? (see my notes) |
b6f795f to411dd48CompareHeahDude commentedFeb 15, 2016
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 ? |
| protectedfunctionforward($controller,array$path =array(),array$query =array()) | ||
| { | ||
| $request =$this->container->get('request_stack')->getCurrentRequest(); | ||
| $path['_forwarded'] =$request->attributes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 commentedFeb 15, 2016
What when there is more than one redirection before the WDT is shown again? |
HeahDude commentedFeb 15, 2016
@xabbuh for chained redirection see mycomment above but I could add a link in the top panel of the profiler for the case where |
HeahDude commentedFeb 15, 2016
So in that it could worth to catch controller and route info from the redirection response |
HeahDude commentedMar 31, 2016
fabpot commentedMar 31, 2016
@HeahDude If you want to rearrange the commits before merge, please do so. |
HeahDude commentedMar 31, 2016
@fabpot The question is, is it needed to keep them separately ? |
fabpot commentedMar 31, 2016
I don't mind keeping several commits if they make sense and it seems they do :) |
HeahDude commentedMar 31, 2016
Agreed :) So we're done here 🎉 |
fabpot commentedMar 31, 2016
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.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 :```phpclass 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 RequestDataCollector227ac77 [WebProfilerBundle] [FrameworkBundle] profile forward controller action0a1b284 [WebProfiler] [HttpKernel] profile redirections
HeahDude commentedMar 31, 2016
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 commentedApr 3, 2016
Missed the development process of this feature, but it looks awesome@HeahDude ! 🚀 |
…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 profileb26cb6d [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 classseesymfony/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.1As 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#17589I 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 RequestDataCollector26b90e4 [HttpKernel] Refactor a RequestDataCollector test case to use a data provider
This PR allows to :
\Symfony\Bundle\FrameWorkBundle\Controller\Controllerin the web debug toolbar._forwardedwith the current request attributes (an instance ofParameterBag) as value to your sub request before handling it.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 inSilex too by registering
SessionServiceProvider()for redirections or by providing this method for forwarding :