IBX-9697: Added profiler panel #2
Conversation
| use Symfony\Component\HttpFoundation\Response; | ||
| use Symfony\Component\HttpKernel\DataCollector\DataCollector; | ||
|
|
||
| class TwigComponentCollector extends DataCollector |
There was a problem hiding this comment.
| class TwigComponentCollector extends DataCollector | |
| final class TwigComponentCollector extends DataCollector |
|
How does it look ? Could you please share screenshot ? |
| @@ -0,0 +1,42 @@ | |||
| {% if collector.renderedComponents is not empty %} | |||
|
|
|||
ViniTou
left a comment
There was a problem hiding this comment.
compact always cause a little headache tho 😅
| Ibexa\TwigComponents\DataCollector\TwigComponentCollector: | ||
| tags: | ||
| - name: ibexa.debug.data_collector | ||
| id: "ibexa.twig_components" |
There was a problem hiding this comment.
| id: "ibexa.twig_components" | |
| id: 'ibexa.twig_components' |
alongosz
left a comment
There was a problem hiding this comment.
This looks good, but I'm missing some minimal unit test coverage, maybe at least for that TwigComponentCollector, because I understand DefaultRenderer is more of e2e behat scope.
@ViniTou I added phpunit for this so you feel better :) |
| ) { | ||
| $this->registry = $registry; | ||
| $this->eventDispatcher = $eventDispatcher; | ||
| $this->collector = $collector; |
There was a problem hiding this comment.
Consider using events to collect data, as this would allow detaching this functionality from the DefaultRenderer instance (single responsibility pricinple and all that 😅).
Alternatively, consider creating TraceableDefaultRenderer that would maintain it's calls/data.
See Symfony\Component\Form\Extension\DataCollector\EventListener\DataCollectorListener for Symfony forms & event listening-based collection.
See Symfony\Component\HttpClient\TraceableHttpClient & Symfony\Component\HttpClient\DataCollector\HttpClientDataCollector for Traceable variant.
| use Ibexa\TwigComponents\Component\Event\RenderSingleEvent; | ||
| use Ibexa\TwigComponents\DataCollector\TwigComponentCollector; | ||
|
|
||
| final class TwigComponentCollectorListener |
There was a problem hiding this comment.
You can - should - use EventSubscriberInterface.
A general rule of thumb when picking whether you want to use subscriber or listener is flexibility. Listeners should be used if they need to be dynamically added / are generic enough to be attached to more events.
Also, consider using higher priority than normal (since you can use priorities for event listeners). We want to collect data early to prevent event propagation being stopped from impacting data collection.
Related PRs:
Description:
Added profiler panel to twig-components