Make sure we always have a stack #134
Conversation
Collector/ProfilePlugin.php
Outdated
| @@ -58,7 +58,9 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |||
| { | |||
| $profile = new Profile($this->pluginName, $this->formatter->formatRequest($request)); | |||
|
|
|||
| $this->collector->getCurrentStack()->addProfile($profile); | |||
| if (false !== $stack = $this->collector->getCurrentStack()) { | |||
There was a problem hiding this comment.
In addition to @dbu's #134 (review): as getCurrentStack() either returns a Stack or not, would
if ($stack = $this->collector->getCurrentStack()) {perhaps suffice?
There was a problem hiding this comment.
Thank you for the reivew. You are correct. I should update.
We are trying out https://app.assignees.io/. A service that automatically finds the best reviewer. Somehow it selected you.
There was a problem hiding this comment.
Because it somehow knows that I'm a nitpicker and micromanager 😂🙈. Scary.
There was a problem hiding this comment.
i would write it like jerome proposes, thats simpler to read.
the question remains if this situation can ever happen. maybe we should instead have getCurrentStack throw an exception if there is no current stack. that is more disruptive but would force to figure out why it happens.
though maybe as this is just a debugging tool, we should rather silently fail than make a noise about it.
There was a problem hiding this comment.
Silently fail will just hide details in the profiler, but it may be hard for users to find why my plugin is not in the profiler.
Throwing a LogicException may do the job unless I missed some cases while refactoring the profiler.
To be honest, I do not know. Im not sure how we can be in ProfilerPlugin without a stack. @fbourigault do you know? |
|
It should never happen as the @Nyholm I agree with those changes, but I'm a bit curious about how @dkvk ran into this issue. In https://github.com/php-http/HttplugBundle/blob/master/DependencyInjection/HttplugExtension.php#L289-L308 every plugin is decorated with a EDIT: Maybe we could log some warning here as it's not a normal situation. |
|
What if you: $plugin = $this->container->get('httplug.plugin.logger');
$client = new PluginClient($httpClient, [$plugin]);But that's not how Daniel did it... |
|
This is not a problem as the plugin doesn't get decorated. |
|
I'll merge this and tag a patch release. Having a request missing from the profile is better than causing errors |
|
thanks. agree that we better have a non-ideal workaround than nothing at
all.
|
What's in this PR?
If we (for some reason) do not have a stack, it should not crash. Just pretend everything is normal.
@dkvk, does this patch solve your issue?