[2.2] added a way to enable the profiler for the very next request in a functional test #4897
Conversation
…t request in functional tests (closes symfony#4307)
…ilently when the profiler is not available (it makes it easier to write functional tests)
Commits ------- 22e9036 updated CHANGELOG bafe890 [FrameworkBundle] changed Client::enableProfiler() behavior to fail silently when the profiler is not available (it makes it easier to write functional tests) f41872b [FrameworkBundle] added a way to enable the profiler for the very next request in functional tests (closes #4307) 67b91e5 [HttpKernel] added a way to enable a disable Profiler Discussion ---------- [2.2] added a way to enable the profiler for the very next request in a functional test Bug fix: yes/no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #4307 Todo: - License of the code: MIT Documentation PR: should be done before merging After merging this PR, we need to disable the profiler in the test environment in Symfony SE.
| $this->profiler = false; | ||
|
|
||
| $this->kernel->boot(); | ||
| $this->kernel->getContainer()->get('profiler')->enable(); |
There was a problem hiding this comment.
this is not really safe to call as the profiler could be removed between enableProfiler and request.
There was a problem hiding this comment.
Client::enableProfiler() with profiler existing
profiler service is removed from the container
Client::request() -> fatal error, although it should just continue without profiling
There was a problem hiding this comment.
I just recognize an even bigger problem: $this->kernel is type hinted as HttpKernelInterface in Symfony\Component\HttpKernel\Client
But here you assume kernel is a KernelInterface which is not necessarily true. HttpKernelInterface does not provide ->getContrainer at all.
There was a problem hiding this comment.
@Tobion the code in other places is already expecting a kernel as it calls the container and the boot.
And you cannot remove a service from a container.
There was a problem hiding this comment.
Either the type hint in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Client.php#L43 is wrong.
Or this subclass must make sure that a KernelInterface is passed. Otherwise this code is wrong.
And you can remove a service from a container with Container::set('profiler', null).
There was a problem hiding this comment.
well, the client class available in HttpKernel is right about its typehint. It only expects an HttpKernelInterface (and Silex can use it for instance).
And no. Container::set('profiler', null) does not remove the service from the container. It removes the instance if it was already created, meaning requesting the service will have to create a new instance.
Bug fix: yes/no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #4307
Todo: -
License of the code: MIT
Documentation PR: should be done before merging
After merging this PR, we need to disable the profiler in the test environment in Symfony SE.