Rename toolbar config to profiling #105
Conversation
| $v['toolbar']['enabled'] = $this->debug; // @deprecated value auto in 1.3.0 | ||
| } | ||
|
|
||
| if (array_key_exists('profiling', $v) && is_array($v['profiling'])) { |
There was a problem hiding this comment.
imho we should consider configuration invalid if both profiling and toolbar exist. the user should decide for one.
do we have a problem at this stage with default values from the Configuration? i think they will only come after this step.
There was a problem hiding this comment.
I think the BC promise at this point is that toolbar will work at least until 2.0 which means if toolbar is configured it should always work, otherwise it's a BC break, no matter if an error is thrown or overwritten by profiling.
What about default values? you mean the auto config? I added it here so that the enabled config can be defined as boolean.
There was a problem hiding this comment.
imo the BC promise covers that you can update the bundle and it continues to work as before. but once you add new configuration that was not possible before the update, it is ok to complain when you leave the old config in place. merging old and new config feels strange, it would mean you can configure both at the same time.
with auto config i meant ->defaultValue() places. i think those come after beforeNormalization - if i am wrong and they are already present, we indeed would need to merge the configurations
There was a problem hiding this comment.
Okay, so if the new config exists: complain or simply ignore the old config?
I still don't get the default value thing: yes, defaultValue comes after the normalization, but we need to check the auto value for BC in the normalization section.
There was a problem hiding this comment.
Okay, so if the new config exists: complain or simply ignore the old config?
I agree with David. If you configure both toolbar and profiling you should get an error.
There was a problem hiding this comment.
sagikazarmark
force-pushed
the
profiling
branch
2 times, most recently
from
8fbcae2 to
cb71934
Compare
|
Added exception on colliding config and separated BC tests. |
| }) | ||
| ->then(function ($v) { | ||
| if (array_key_exists('profiling', $v)) { | ||
| throw new InvalidConfigurationException('"toolbar" config is deprecated as of version 1.3.0, please use "profiling" instead.'); |
There was a problem hiding this comment.
lets change the message: "Can't configure both "toolbar" and "profiling" section. The "toolbar" config is deprecated as of version 1.3.0, please only use "profiling"."
|
Rewritten exception message, added trigger errors. |
| @@ -69,6 +69,29 @@ public function getConfigTreeBuilder() | |||
| return $v; | |||
| }) | |||
| ->end() | |||
| ->beforeNormalization() | |||
| ->ifTrue(function ($v) { | |||
| @trigger_error('"httplug.toolbar" config is deprecated since version 1.3 and will be removed in 2.0. Use "httplug.profiling" instead.', E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
this will trigger the warning regardless of whether toolbar is used or not. should go into ->then(), probably after the InvalidConfigurationException check
There was a problem hiding this comment.
You are right, done.
|
can you please also update the documentation? with a ..versionadded note. |
What's in this PR?
Rename toolbar config to profiling as it seems to be more common.