Profiling might not work with CachePlugin #175
Conversation
|
As a solution, we could move the ProfileClient code into the ProfilePlugin and let the ProfilePlugin fill all those data if not yet set. By doing so, the last executed plugin would fill the request data. |
|
I also added a failing test case when the plugin throw an Exception. |
|
I think the exception case can be handled with a try/catch block in the |
|
👍 |
|
Tests are now green 🎉. I think I should work Also I'm open to any suggestion about not duplicating code between |
There was a problem hiding this comment.
Im happy to merge. But I do not understand the changes in ProfilePlugin to 100%. But I do undertand the tests and they are all passing.
Fabien, If you want a third set of eyes on this then we should wait for David or Mark (or someone else?). If you are confident you can merge this.
(I really need to study Promises..)
| "symfony/phpunit-bridge": "^3.2", | ||
| "symfony/twig-bundle": "^2.8 || ^3.0", | ||
| "symfony/twig-bridge": "^2.8 || ^3.0", | ||
| "symfony/web-profiler-bundle": "^2.8 || ^3.0", | ||
| "symfony/finder": "^2.7 || ^3.0", | ||
| "symfony/cache": "^3.1", |
There was a problem hiding this comment.
Oh, I see. php-cache is not good enough for you? :)
There was a problem hiding this comment.
Jokes aside, can you require this on sf2.8?
There was a problem hiding this comment.
Yes if you use PHP 5.5.9+
Tell me I will try to explain :) |
Collector/ProfilePlugin.php
Outdated
| return $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst)->then(function (ResponseInterface $response) use ($profile) { | ||
| try { | ||
| $promise = $this->plugin->handleRequest($request, $wrappedNext, $wrappedFirst); | ||
| } catch (Exception $e) { |
There was a problem hiding this comment.
From where is this exception thrown?
There was a problem hiding this comment.
It's thrown from any plugin or even the client.
Collector/ProfilePlugin.php
Outdated
|
|
||
| /** | ||
| * Collect request information when not already done by the HTTP client. This happens when using the CachePlugin | ||
| * and the cache is hit without re-validation. |
There was a problem hiding this comment.
What do you mean with "without re-validation"?
There was a problem hiding this comment.
I mean when the plugin does not add If-Modified-Since or If-None-Match header.
| * @param RequestInterface $request | ||
| * @param Stack|null $stack | ||
| */ | ||
| private function collectRequestInformation(RequestInterface $request, Stack $stack = null) |
There was a problem hiding this comment.
So running this does only have an impact when cache plugin and "without re-validation"?
There was a problem hiding this comment.
If the request goes through the whole chain, request information will be already collected by the ProfileClient.
Collector/ProfilePlugin.php
Outdated
|
|
||
| return $response; | ||
| }, function (Exception $exception) use ($profile) { | ||
| }, function (Exception $exception) use ($profile, $request, $stack) { | ||
| $profile->setFailed(true); |
There was a problem hiding this comment.
From where is this exception thrown?
How does it differ from https://github.com/php-http/HttplugBundle/pull/175/files#r124504876
There was a problem hiding this comment.
This exception can be thrown from anywhere too.
But, as some Http\Promise\Promise implementation does not catch all exceptions, we have to use both to catch exceptions in any case. See Http\Promise\FulfilledPromise and Http\Client\Promise\HttpfulfilledPromise for implementation differences.
|
Thank you for reviewing this PR. |
This a a failing functional test about profiling when the
CachePluginis involved.When the cache is hit and the
CachePlugindoes not perform any cache validation, theProfileClientis never reached. This cause the profiled stack about not being complete and will display empty boxes in the profiler panel.