Remove profiler storages #15944
Remove profiler storages #15944
Conversation
javiereguiluz
commented
Sep 27, 2015
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | - |
| License | MIT |
| Doc PR | - |
|
Please add the |
|
You also need to add |
|
|
|
Added Deprecation of config options has been done in |
|
LGTM |
|
IMO the |
|
|
||
| The web development toolbar has been completely redesigned. This update has | ||
| introduced some changes in the HTML markup of the toolbar items. | ||
| The web development toolbar has been completely redesigned. This update has |
There was a problem hiding this comment.
this should be a different bullet point. It is not the same change the deprecating import and export
|
status: needs work |
| @@ -27,6 +31,8 @@ class MongoDbProfilerStorage implements ProfilerStorageInterface | |||
| */ | |||
| public function __construct($dsn, $username = '', $password = '', $lifetime = 86400) | |||
| { | |||
| @trigger_error('The '.__CLASS__.' class is deprecated since Symfony 2.8 and will be removed in 3.0. Use FileProfilerStorage instead.', E_USER_DEPRECATED); | |||
5fac47d to
94f5e98
Compare
|
I've moved deprecation notices at the beginning of the classes. |
| ->defaultValue('file:%kernel.cache_dir%/profiler') | ||
| ->beforeNormalization() | ||
| ->always() | ||
| ->thenInvalid('The profiler.dsn configuration key must start with "file:" because all the storages except the filesystem are deprecated since version 2.8 and will be removed in 3.0.') |
There was a problem hiding this comment.
That's too strong. We should still accept other storage engines, but trigger a deprecation notice in such cases (same for the username and password).
You can see an example in the same file for csrf_protection.
|
👍 |
| ->scalarNode('dsn') | ||
| ->defaultValue('file:%kernel.cache_dir%/profiler') | ||
| ->beforeNormalization() | ||
| ->always() |
There was a problem hiding this comment.
this should not be always. configuring a DSN is not deprecated entirely. It should not be triggered when it starts with file
There was a problem hiding this comment.
You are right. Changed by:
->ifTrue(function ($v) { return 'file:' !== substr($v, 0, 5); })| @@ -11,11 +11,16 @@ | |||
|
|
|||
| namespace Symfony\Component\HttpKernel\Profiler; | |||
|
|
|||
| @trigger_error('The '.__NAMESPACE__.'\PdoProfilerStorage class is deprecated since Symfony 2.8 and will be removed in 3.0. Use FileProfilerStorage instead.', E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
this should also be triggered for child classes IMO, as they are also deprecated.
|
👍 |
|
Thank you @javiereguiluz. |
|
I'm sorry for commenting on such old PR (especially I'm almost a year late here), but what were the reasons for deprecating and removing non-filesystem storages? I went through issues backlog/changelog/update files and only found PRs doing so without justification. Reason I'm asking is that currently colleague of mine is working on moving us from Sf 2.8 to 3.x and I was a bit dazzled by the fact he added almost the same class as removed |
|
@fabpot @javiereguiluz @malarzm @alexgurrola We have the exact same issue with load balancers. Anyone who uses Symfony in an enterprise context (with more than one server) can no longer use the profiler as it's intended to be used. And why was this removed in the first place? There doesn't seem to be any explanation of the rational for the removal of this extremely helpful feature. |
|
If you dug into the tickets referenced by this PR you would find #15939 which describes why it was done. While I don't agree with the rational, if you are looking for the explanation, it's already described there. |
|
Now that we'll have Symfony Flex soon, we may rethink some past decisions, like this one, and release those features as small packages (e.g. |