-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[DebugBundle][HttpKernel] Collect dumps when console profiling is enabled #62027
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
Conversation
nicolas-grekas
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.
Can't this be dealt with outside of the class? Inside the injected dumper?
I'm asking because this makes the constructor a bit weird.
Some explanations could be helpful.
src/Symfony/Component/HttpKernel/EventListener/DumpListener.php
Outdated
Show resolved
Hide resolved
@nicolas-grekas I've considered several approaches. The main obstacle I came across was having access to the I also thought about creating a new implementation of |
b144bb7 to
26f1f11
Compare
|
I wonder if not-instantiating the dump-collector is really a concern? Would it make things simpler if not? Otherwise, let's add some explanation about the argument in an |
@nicolas-grekas It might not be a real concern, but it doesn't simplify things either. I could always inject the
Regarding the changelog/upgrade part, should I retarget 7.4 and mark it as a feature? |
26f1f11 to
d94360f
Compare
|
Yes, let's target 7.4. |
d94360f to
292adba
Compare
292adba to
c4f8091
Compare
@nicolas-grekas Done. Thanks for the feedback. |
|
I thought the logic was that if the dump had somewhere to be displayed on it wouldn’t appear in the profiler: that matched Twig’s |
|
@MatTheCat With this PR, the The Twig |
| 7.4 | ||
| --- | ||
|
|
||
| * Wire the `$profilerDumper` argument in `DumpListener` |
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's the outcome of this change? is this related to the console profiling? if not, is that a bugfix for another branch? Sorry I forgot how this works :)
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.
@nicolas-grekas It wires the newly added second dumper argument so that the DumpListener can "choose" which dumper to use based on whether the --profile option is set. The listener is only triggered for console commands. Like I mentioned earlier, it feels more like a missing feature to me than a bug fix.
|
Thank you @HypeMC. |
|
Thanks for solving it @HypeMC 🙇 |
Currently, when profiling a console command, calls to
dump()aren't collected.With this PR, they'll be collected when profiling is enabled, or output directly when it's not.
Not entirely sure whether this should be considered a bug or simply a missing feature.