minor profiler updates #201
Conversation
- don't display empty client tabs in profiler.
- return only collector root stacks.
- better count of client messages.
There was a problem hiding this comment.
looks sane to me. i wonder about renaming the public method however - if somebody customized the template, things will break for them. could we add a BC method for that with a deprecation? (i know the class is marked as internal, still adjusting the template is a possibility). i am fine if the BC method just returns the root stacks, just so we don't break customized templates.
|
I re-added |
CHANGELOG.md
Outdated
|
|
||
| ### Changed | ||
|
|
||
| - The `Http\HttplugBundle\Collector\Collector::getClientStack` method returns only stacks without parent. |
There was a problem hiding this comment.
Stack or Stacks in the method name?
CHANGELOG.md
Outdated
|
|
||
| ### Deprecated | ||
|
|
||
| - The `Http\HttplugBundle\Collector\Collector` `getClientStacks` method is deprecated. Use `getClientRootStacks` method instead. |
There was a problem hiding this comment.
Http\HttplugBundle\Collector\Collector::getClientStacks
There was a problem hiding this comment.
Should we really deprecate the method AND change its behaviour? If fine with deprecate it and leave its current behaviour.
|
Oh, my comments disappeared but are still valid: #201 (comment) |
|
As the class is internal I decided to keep the method as a proxy. Maybe I should not document the behavior change. Behavior changed for |
|
actually if we leave a BC method, we can just as well leave it unchanged in behaviour. might be the easiest. |
Collector/Collector.php
Outdated
| @@ -131,9 +131,13 @@ public function getFailedStacks() | |||
| */ | |||
| public function getClients() | |||
| { | |||
| $stacks = array_filter($this->data['stacks'], function (Stack $stack) { | |||
| return $stack->getParent() == null; | |||
Collector/Collector.php
Outdated
| @@ -143,12 +147,52 @@ public function getClients() | |||
| */ | |||
| public function getClientStacks($client) | |||
| { | |||
| @trigger_error(sprintf('Method %s() is deprecated since version 1.8 and will be removed in 2.0. Use getClientRootStacks() instead.', __METHOD__), E_USER_DEPRECATED); | |||
There was a problem hiding this comment.
Remove this method completely or leave it unchanged (and deprecate it). It is fine to remove it since the Collector class is @internal.
|
on the BC topic: my point was that even though the class is marked as internal, its used in the template so it might be customized. but i agree, lets drop the method. at worst, somebody gets an exception in their customized toolbar template. lets just do an entry in the changelog that the method has been renamed and only returns root stacks now. |
|
Do we really need to clutter the changelog with internal changes? |
|
the one person having a problem would probably open an issue here if they don't find what happened, so i'd say i am fine without a changelog entry. |
CHANGELOG.md
Outdated
| ### Changed | ||
|
|
||
| - The `Http\HttplugBundle\Collector\Collector` `getClientStacks` method have been renamed to `getClientRootStacks` | ||
| and now returns only stacks without any parent. |
There was a problem hiding this comment.
i am fine to remove this changelog entry if you want.
There was a problem hiding this comment.
Nice, I didn't saw the without in you previous comment!
This is part of #109 and solve some edge cases when displaying the profiler panel: