[FrameworkBundle] Don't load the profiler when it is disabled #7071
Conversation
|
This is wrong. The profiler configuration has an This now forbids enabling it, even when setting it explicitly to |
|
hmm, actually, the issue is that setting it to |
|
@stof that's right. I'm sure the way I've did it is not the best possible but there should be a way to describe that we don't want to need any debugging/profiling related classes loaded at all. I'll fix the tests/patch things up once we got a direction for this. Thanks. |
|
@dlsniper I don't have time to lok at the details but the profiler is meant to be activable in prod (you can enable the profiler on a specific URL / IP in such a case) to debug an issue that would occur in prod only. |
|
@vicb but the issue is it means that collectors are still registered and so on, even if you never use them. I think we should have a way to remove the profiler entirely if we don't want them (still keeping the config of the default state of the profiler itself between enabled and disabled when the service is registered) |
|
@vicb I know but in this case I don't think anyone should panic in the other ticket about adding 'debug' classes to the compiled class file as they are loaded anyway, right? So this ticket would either provide a way to disable the profiler loading in a different way and still allow for usage in prod environment or just close this ticket and carry on. Clearly I'd prefer the first option, provide a better way to disable the loading of unnecessary collectors and so on. I'll be able to work on this during weekend hopefully. |
|
@stof I understand, it was just a kind warning
So let's add all the classes from Symfony, any of them might be use from time to times ! The goal of the cache aggregation is to aggregate the most comonly used classes - really no reason to be offensive here. |
|
@vicb the profiler and its collectors are used on all requests as the profiler is a dependency of the kernel.request listener enabling it conditionally. |
|
@vicb Actually, I think the listener should be removed when there is no RequestMatcher and the profiler is disabled by default |
|
hmm, no, it may not work as the profiler still need to run on kernel.request to be able to be activated later during the request :( |
|
@vicb I didn't wanted to sound aggressive, I apologize if it sounded like that. I'm well aware that just adding a bunch of classes to cache is not a good thing but given the current situation they are loaded anyway. That's why I said that nobody should panic :) And like @stof said, since those classes are loaded on every request, now, I think we can classify them as commonly used. Maybe we could add a new parameter in the |
|
@dlsniper no problem. I don't have time to help more these days. both Your pr address true issues but I think the solutions are not the good ones, hence my comments. |
|
I've reworked the patch to rely on the profiler enabled flag instead of kernel. I'll fix the tests asap as they are failing on my computer. |
|
I actually don't know how to fix the tests. I've set the profiler on true, removed it but the tests still fail. Thanks. |
|
The last commit should fix the tests, please confirm that the tests still make sense. |
|
Note, the tests failed on Travis because random reasons but sure not in the code I've changed :) |
|
@fabpot any feedback on this one? |
| @@ -93,7 +93,11 @@ public function load(array $configs, ContainerBuilder $container) | |||
| $this->registerValidationConfiguration($config['validation'], $container, $loader); | |||
| $this->registerEsiConfiguration($config['esi'], $container, $loader); | |||
| $this->registerFragmentsConfiguration($config['fragments'], $container, $loader); | |||
| $this->registerProfilerConfiguration($config['profiler'], $container, $loader); | |||
|
|
|||
| if (isset($config['profiler']) && isset($config['profiler']['enabled']) && $config['profiler']['enabled']) { | |||
There was a problem hiding this comment.
First isset is unnecessary, isn't it?
|
I've just open a new PR #7859 with some changes. Feedback welcome there. |
This PR was merged into the master branch. Discussion ---------- Profiler activation | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7064, #7071 | License | MIT | Doc PR | symfony/symfony-docs#2565 As stated in #7071, there is no way to disable the profiler completely. Even when the `enabled` flag is set to `false`, the profiler is still registered but the data collectors are not activated. Now, when `enabled` is `false`, the profiler is disabled. To get the old `false` behavior, you now need to set `enabled` to `true` and set the new `collect` flag to `false`. Todo: - [x] update docs - [x] update Symfony SE -- not needed Commits ------- 88ebd62 fixed the registration of the web profiler when the profiler is disabled a11f901 [FrameworkBundle] added a way to disable the profiler f675dd8 Truly disabled profiler in prod
This fixes the loading of Collector classes seen in #7064 at the cost of not loading them at all in any environment which doesn't use
enable: true.Due to the nature of the change I'm not sure if this is or not a BC break or a bug fix, I'll leave this to you to judge.
Also, I'm happy with this change as it really means that if I want to have debugging tools running in my application I should run with debugging enabled but I'm not sure about others.
Thanks.