Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] AddStackInterface, allowing to unstack the call stack#28943
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
72ecf16 toc3002e0Compare| /** | ||
| * @param callable|NextInterface $next | ||
| */ | ||
| publicfunctionhandle(Envelope$envelope,callable$next):void; |
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.
worth to describe the callable signature / different modes in the phpdoc?
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.
ah it's pretty clear in NextInterface,@see NextInterface maybe?
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.
that's what the@param annotation already links to to me
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.
sorry i missed that
c3002e0 to732be7fCompare
sroze left a comment
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.
As discussed IRL, this adds a lot of complexity to the way the middleware are called. I don't think that it's worth it, just for a few items in the stack trace (suggestion in a comment).
sroze commentedOct 25, 2018
Maybe we can simplify the publicfunctionhandle(Envelope$envelope,StackInterface$stack):Envelope{// ...return$stack->next()->handle($envelope,$stack);} |
| return$middleware; | ||
| } | ||
| publicfunctionhandle(Envelope$envelope,callable$next):void |
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.
why should this class implement MiddlewareInterface ? Calling it as a middleware does not make sense
38326a7 toccce2b8Comparenicolas-grekas commentedOct 25, 2018
I love that ;)@stof that fixes also your comment. |
NextInterface, allowing to unstack the call stackStackInterface, allowing to unstack the call stackccce2b8 to09eff3dCompare09eff3d to2bc7d11Comparesroze left a comment
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.
As discussed IRL, I like it. It's much more explicit. And it solves our stack-trace burden.
sroze commentedOct 25, 2018
Thank you@nicolas-grekas. |
…the call stack (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Add `StackInterface`, allowing to unstack the call stack| Q | A| ------------- | ---| Branch? | 4.2| Bug fix? | no| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -At the moment, debugging an exception coming from a middleware is not as friendly as it could: each middleware is preceded by a noisy frame added by the `$next` callable indirection.This PR allows removing this frame by using `$next()->handle($envelope, $next);` instead of `$next($envelope);` in a middleware. Note that this is optional so that the later continues to work. But if one wants to opt-in for the former, then the stack trace of exceptions will be freed from `$next`.All core middleware should do this, so here they are, updated.Commits-------2bc7d11 [Messenger] Add `StackInterface`, allowing to unstack the call stack
…agi)This PR was merged into the 4.2-dev branch.Discussion----------[Messenger] Move MiddlewareTestCase in Test ns| Q | A| ------------- | ---| Branch? | master <!-- see below -->| Bug fix? | no| New feature? | yes <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes (AppVeyor failure unrelated) <!-- please add some, will be required by reviewers -->| Fixed tickets | N/A <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | N/AJust a quick suggestion after#28943 being merged. Wouldn't it make sense to move this in `Test` namespace to be used by anyone wanting to unit test their middleware?Commits-------215c895 [Messenger] Move MiddlewareTestCase in Test ns
At the moment, debugging an exception coming from a middleware is not as friendly as it could: each middleware is preceded by a noisy frame added by the
$nextcallable indirection.This PR allows removing this frame by using
$next()->handle($envelope, $next);instead of$next($envelope);in a middleware. Note that this is optional so that the later continues to work. But if one wants to opt-in for the former, then the stack trace of exceptions will be freed from$next.All core middleware should do this, so here they are, updated.