New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DataCollector for Symfony WebProfiler toolbar. #1707

Merged
merged 8 commits into from Apr 3, 2018

Conversation

Projects
None yet
6 participants
@jdeniau
Contributor

jdeniau commented Feb 14, 2018

Fixes #1701

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1701
License MIT
Doc PR ~

Add DataCollector / Profiler as discussed in #1701

Example:

Edit: good start for #102 and #1176.

<div class="sf-toolbar-info-piece">
</div>
{% endset %}
#}

This comment has been minimized.

@soyuka

soyuka Feb 14, 2018

Member

remove dead code?

This comment has been minimized.

@jdeniau

jdeniau Feb 14, 2018

Contributor

I will do this tomorrow

@@ -0,0 +1,53 @@
<svg xmlns="http://www.w3.org/2000/svg" width="{{ width|default(256.25) }}" height="{{ width|default(419.38) }}" viewBox="0 0 256.25 419.38">

This comment has been minimized.

@soyuka

soyuka Feb 14, 2018

Member

could we reference an online resource instead of adding this to the core?

I'm not sure but it feels this file doesn't belong here.

This comment has been minimized.

@jdeniau

jdeniau Feb 14, 2018

Contributor

The Symfony documentation recommend importing svg via twig, that's why I did this like that

This comment has been minimized.

@soyuka

soyuka Feb 15, 2018

Member

okay keep it that way then :)

@antograssiot

This comment has been minimized.

Member

antograssiot commented Feb 14, 2018

@jdeniau I was about to open a PR on your repo to improve formatting, attributes exposition, etc... should we wait for this to be merged first and improve it after ?

@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Feb 14, 2018

@antograssiot as you want. If you open a PR on my repo, I will report it here. But as anyone seems to like the idea in the issue, you can as well wait and open another PR here once it is merged.

It's up to you!

@antograssiot

This comment has been minimized.

Member

antograssiot commented Feb 15, 2018

that would be a great addition to 2.2
@jdeniau This is were I got in styling/update, i'll send a MR following yours or contact me on slack if you prefer
image

@soyuka

This comment has been minimized.

Member

soyuka commented Feb 15, 2018

I've optimized the svg:

<svg xmlns="http://www.w3.org/2000/svg" width="{{ width|default(256.25) }}" height="{{ width|default(419.38) }}" viewBox="0 0 256.25 419.38"><g data-name="spider"><g><path d="M127.8 99l4.47-5.12L172 141.27l-22.69 38.48a3.83 3.83 0 0 1-5.73-.76l-.71-1.08a3.83 3.83 0 0 1 .71-5l19.72-31.05z" fill="#1d1e1c"/></g><path d="M130.57 102.16s17.64-21 12.76-30.69-7-8.9-14.17-7.75-11.76 5.63-10.73 13.82a40.17 40.17 0 0 0 12.14 24.62z" fill="#1d1e1c"/><g><path d="M118.53 99l-4.47-5.12-39.77 47.39L97 177.74a3.83 3.83 0 0 0 5.73-.76l.71-1.08a3.83 3.83 0 0 0-.71-5L83 141.81z" fill="#1d1e1c"/></g><path d="M115.76 102.16S98.11 81.12 103 71.47s7-8.9 14.17-7.75 11.76 5.63 10.73 13.82a40.17 40.17 0 0 1-12.14 24.62z" fill="#1d1e1c"/><g><path fill="#1d1e1c" d="M132.8 42.02l4.47-5.12 86.77 103.36-54.99 51.39-5.02-4.58 50.31-46.26-81.54-98.79z"/></g><path d="M136.17 45s17.15-26.3 10.29-36.59-9.15-9.15-17.15-6.86-12.58 8-10.31 17.15A45.93 45.93 0 0 0 136.17 45z" fill="#1d1e1c"/><g><path fill="#1d1e1c" d="M113.53 42.02l-4.47-5.12-86.77 103.36 55 51.39 5.01-4.58L32 140.81l81.53-98.79z"/></g><path d="M110.16 45S93 18.7 99.87 8.41 109-.74 117 1.55s12.58 8 10.29 17.15A45.93 45.93 0 0 1 110.16 45z" fill="#1d1e1c"/><g fill="#1d1e1c"><path d="M40.66 321.08L23.67 248.1l31.32.56 1.77 7.43-24.21-.5 15.36 63.08-7.25 2.41z"/><path d="M36.81 331s-6.21 11.73 5.36 16.56 17-5.51 17-5.51.89-4.49-4.79-14.32-7.38-11.9-10.56-12.43-7.01 15.7-7.01 15.7z"/></g><g fill="#1d1e1c"><path d="M21.27 383.93L0 246.05l57.07-25.51-.73 7.59-47.79 22.31L28.9 384l-7.63-.07z"/><path d="M14.57 396.19s-9.28 13 3.75 20.59 21.17-3.84 21.17-3.84 1.78-5.21-3.43-17.83-6.91-15.4-10.63-16.54-10.86 17.62-10.86 17.62z"/></g><g fill="#1d1e1c"><path d="M215.59 320.08l16.99-72.98-31.32.56-1.77 7.43 24.21-.5-15.36 63.08 7.25 2.41z"/><path d="M219.44 330s6.21 11.73-5.36 16.56-17-5.51-17-5.51-.89-4.49 4.79-14.32 7.4-11.93 10.59-12.46 6.98 15.73 6.98 15.73z"/></g><g fill="#1d1e1c"><path d="M234.98 382.93l21.27-137.88-57.07-25.51.73 7.59 47.8 22.31L227.36 383l7.62-.07z"/><path d="M241.68 395.19s9.28 13-3.75 20.59-21.17-3.84-21.17-3.84-1.78-5.21 3.43-17.83 6.91-15.4 10.63-16.54 10.86 17.62 10.86 17.62z"/></g><g><path d="M207.23 219.63c0 41-35.77 62.69-79.9 62.69s-77.67-21.69-77.67-62.69 33.54-71.22 77.67-71.22 79.9 30.22 79.9 71.22z" fill="#38a9b4"/><path d="M126.33 285.72C103 285.72 83 279.93 68.58 269c-15.46-11.73-23.63-28.79-23.63-49.34 0-38.35 28.14-74.62 81.38-74.62 53.92 0 83.62 36.82 83.62 74.62 0 20.47-8.58 37.54-24.8 49.38-14.78 10.75-35.66 16.68-58.82 16.68zm0-133.91c-49.07 0-74 33.44-74 67.82 0 36.58 28.34 59.3 74 59.3 46.28 0 76.18-23.28 76.18-59.3 0-42.83-41.42-67.82-76.18-67.82z" fill="#1d1e1c"/></g><g><ellipse cx="92.29" cy="165.77" rx="37.55" ry="36.82" fill="#fff"/><path d="M91.29 205c-22 0-39.92-17.59-39.92-39.2s17.91-39.2 39.92-39.2 39.93 17.59 39.93 39.2S113.3 205 91.29 205zm0-73.65c-19.39 0-35.17 15.45-35.17 34.45s15.78 34.45 35.17 34.45 35.17-15.45 35.17-34.45-15.78-34.47-35.17-34.47z" fill="#1d1e1c"/></g><ellipse cx="95.19" cy="167.95" rx="16.99" ry="16.3" fill="#1d1e1c"/><circle cx="92.02" cy="165.04" r="4.68" fill="#fff"/><g><ellipse cx="166.29" cy="171.09" rx="45.51" ry="47.07" fill="#fff"/><path d="M165.29 220.53c-26.41 0-47.89-22.18-47.89-49.44s21.48-49.44 47.89-49.44 47.89 22.18 47.89 49.44-21.48 49.44-47.89 49.44zm0-94.13c-23.78 0-43.14 20-43.14 44.69s19.35 44.69 43.14 44.69 43.13-20 43.13-44.69-19.34-44.69-43.13-44.69z" fill="#1d1e1c"/></g><circle cx="169.13" cy="173.57" r="20.96" fill="#1d1e1c"/><circle cx="180.48" cy="170.39" r="5.78" fill="#fff"/></g></svg>
@jewome62

This comment has been minimized.

Contributor

jewome62 commented Feb 15, 2018

Maybe put the logo api platform in white ?

@soyuka

This comment has been minimized.

Member

soyuka commented Feb 15, 2018

#d2d2d2 instead of #38a9b4 :

<svg xmlns="http://www.w3.org/2000/svg" width="{{ width|default(256.25) }}" height="{{ width|default(419.38) }}" viewBox="0 0 256.25 419.38"><g data-name="spider"><g><path d="M127.8 99l4.47-5.12L172 141.27l-22.69 38.48a3.83 3.83 0 0 1-5.73-.76l-.71-1.08a3.83 3.83 0 0 1 .71-5l19.72-31.05z" fill="#1d1e1c"/></g><path d="M130.57 102.16s17.64-21 12.76-30.69-7-8.9-14.17-7.75-11.76 5.63-10.73 13.82a40.17 40.17 0 0 0 12.14 24.62z" fill="#1d1e1c"/><g><path d="M118.53 99l-4.47-5.12-39.77 47.39L97 177.74a3.83 3.83 0 0 0 5.73-.76l.71-1.08a3.83 3.83 0 0 0-.71-5L83 141.81z" fill="#1d1e1c"/></g><path d="M115.76 102.16S98.11 81.12 103 71.47s7-8.9 14.17-7.75 11.76 5.63 10.73 13.82a40.17 40.17 0 0 1-12.14 24.62z" fill="#1d1e1c"/><g><path fill="#1d1e1c" d="M132.8 42.02l4.47-5.12 86.77 103.36-54.99 51.39-5.02-4.58 50.31-46.26-81.54-98.79z"/></g><path d="M136.17 45s17.15-26.3 10.29-36.59-9.15-9.15-17.15-6.86-12.58 8-10.31 17.15A45.93 45.93 0 0 0 136.17 45z" fill="#1d1e1c"/><g><path fill="#1d1e1c" d="M113.53 42.02l-4.47-5.12-86.77 103.36 55 51.39 5.01-4.58L32 140.81l81.53-98.79z"/></g><path d="M110.16 45S93 18.7 99.87 8.41 109-.74 117 1.55s12.58 8 10.29 17.15A45.93 45.93 0 0 1 110.16 45z" fill="#1d1e1c"/><g fill="#1d1e1c"><path d="M40.66 321.08L23.67 248.1l31.32.56 1.77 7.43-24.21-.5 15.36 63.08-7.25 2.41z"/><path d="M36.81 331s-6.21 11.73 5.36 16.56 17-5.51 17-5.51.89-4.49-4.79-14.32-7.38-11.9-10.56-12.43-7.01 15.7-7.01 15.7z"/></g><g fill="#1d1e1c"><path d="M21.27 383.93L0 246.05l57.07-25.51-.73 7.59-47.79 22.31L28.9 384l-7.63-.07z"/><path d="M14.57 396.19s-9.28 13 3.75 20.59 21.17-3.84 21.17-3.84 1.78-5.21-3.43-17.83-6.91-15.4-10.63-16.54-10.86 17.62-10.86 17.62z"/></g><g fill="#1d1e1c"><path d="M215.59 320.08l16.99-72.98-31.32.56-1.77 7.43 24.21-.5-15.36 63.08 7.25 2.41z"/><path d="M219.44 330s6.21 11.73-5.36 16.56-17-5.51-17-5.51-.89-4.49 4.79-14.32 7.4-11.93 10.59-12.46 6.98 15.73 6.98 15.73z"/></g><g fill="#1d1e1c"><path d="M234.98 382.93l21.27-137.88-57.07-25.51.73 7.59 47.8 22.31L227.36 383l7.62-.07z"/><path d="M241.68 395.19s9.28 13-3.75 20.59-21.17-3.84-21.17-3.84-1.78-5.21 3.43-17.83 6.91-15.4 10.63-16.54 10.86 17.62 10.86 17.62z"/></g><g><path d="M207.23 219.63c0 41-35.77 62.69-79.9 62.69s-77.67-21.69-77.67-62.69 33.54-71.22 77.67-71.22 79.9 30.22 79.9 71.22z" fill="#d2d2d2"/><path d="M126.33 285.72C103 285.72 83 279.93 68.58 269c-15.46-11.73-23.63-28.79-23.63-49.34 0-38.35 28.14-74.62 81.38-74.62 53.92 0 83.62 36.82 83.62 74.62 0 20.47-8.58 37.54-24.8 49.38-14.78 10.75-35.66 16.68-58.82 16.68zm0-133.91c-49.07 0-74 33.44-74 67.82 0 36.58 28.34 59.3 74 59.3 46.28 0 76.18-23.28 76.18-59.3 0-42.83-41.42-67.82-76.18-67.82z" fill="#1d1e1c"/></g><g><ellipse cx="92.29" cy="165.77" rx="37.55" ry="36.82" fill="#fff"/><path d="M91.29 205c-22 0-39.92-17.59-39.92-39.2s17.91-39.2 39.92-39.2 39.93 17.59 39.93 39.2S113.3 205 91.29 205zm0-73.65c-19.39 0-35.17 15.45-35.17 34.45s15.78 34.45 35.17 34.45 35.17-15.45 35.17-34.45-15.78-34.47-35.17-34.47z" fill="#1d1e1c"/></g><ellipse cx="95.19" cy="167.95" rx="16.99" ry="16.3" fill="#1d1e1c"/><circle cx="92.02" cy="165.04" r="4.68" fill="#fff"/><g><ellipse cx="166.29" cy="171.09" rx="45.51" ry="47.07" fill="#fff"/><path d="M165.29 220.53c-26.41 0-47.89-22.18-47.89-49.44s21.48-49.44 47.89-49.44 47.89 22.18 47.89 49.44-21.48 49.44-47.89 49.44zm0-94.13c-23.78 0-43.14 20-43.14 44.69s19.35 44.69 43.14 44.69 43.13-20 43.13-44.69-19.34-44.69-43.13-44.69z" fill="#1d1e1c"/></g><circle cx="169.13" cy="173.57" r="20.96" fill="#1d1e1c"/><circle cx="180.48" cy="170.39" r="5.78" fill="#fff"/></g></svg>
@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Feb 15, 2018

@soyuka intergrated the clean svg file

@antograssiot I made some style improvements in order to look like your screenshot.
The only thing I did not get is the array rendering you are using, I only used json_encode.

@antograssiot

This comment has been minimized.

Member

antograssiot commented Feb 15, 2018

@jdeniau I used profiler_dump() but dump() is nice too and allow folding.
EDIT:
Oh and probably a nowrap class or something like that if I remember correctly...

<tbody>
{% for key, value in collector.resourceMetadata.attributes %}
{% if key != 'filters' %}

This comment has been minimized.

@antograssiot

antograssiot Feb 15, 2018

Member

You can inline the if in the for loop
{% for key, value in collector.resourceMetadata.attributes if key != 'filters' %}

@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Feb 15, 2018

@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Feb 16, 2018

@antograssiot used dump, the rendering is great, thanks !

@antograssiot

This comment has been minimized.

Member

antograssiot commented Feb 16, 2018

@jdeniau looking forward to it, nice idea/work 👏

@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Mar 23, 2018

Up @dunglas I think it's good to merge.
The error is an issue with phpstan about unrelated code.

I reproduce it on master branch

I think it may be better to include phpstan in api-platform dev dependencies, this way you avoid bug when phpstan is updated.

@meyerbaptiste

This comment has been minimized.

Member

meyerbaptiste commented Mar 23, 2018

I'm on it @jdeniau: #1789

@soyuka

This comment has been minimized.

Member

soyuka commented Mar 23, 2018

needs rebase :D

@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Mar 23, 2018

Thanks @meyerbaptiste, it's rebased cc @soyuka

@soyuka

This comment has been minimized.

Member

soyuka commented Mar 23, 2018

Doh, appveyor failed but looks weird. May you force push again to trigger the ci? (git commit --amend --no-edit && git push -f)

@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Mar 23, 2018

@soyuka build pass :)

@dunglas

Great feature, can't wait to merge it!

class RequestDataCollector extends DataCollector
{
/**
* collectionFactory.

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

All those docblocks are useless (already documented by the constructor's typehints, IDEs will infer the types). Please remove them on all properties.

use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\DataCollector\DataCollector;
class RequestDataCollector extends DataCollector

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

Can you please make this class final (we do that for every classes to ease the maintenance).

private $metadataFactory;
/**
* __construct.

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

Please drop all this doc block, it's already documented by the typehints.

$this->metadataFactory = $metadataFactory;
}
public function collect(Request $request, Response $response, \Exception $exception = null)

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

: void

public function collect(Request $request, Response $response, \Exception $exception = null)
{
$resourceClass = $request->attributes->get('_api_resource_class');
$resourceMetadata = $resourceClass ? $this->metadataFactory->create($resourceClass) : null;

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

$resourceMetadata = $request->attributes->get('_api_resource_class', $this->metadataFactory->create($resourceClass));

This comment has been minimized.

@jdeniau

jdeniau Mar 23, 2018

Contributor

Whe need the $resourceClass in the data parameter

xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
<services>
<service id="ApiPlatform\Core\Bridge\Symfony\Bundle\DataCollector\RequestDataCollector" public="false">

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

AFAIK, we don't use the id = class name convention yet in API Platform (we use aliases to enable autowiring instead, so the list of autowirable services isn't polluted). May I suggest api_platform.data_collector.request instead?

@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" width="{{ width|default(256.25) }}" height="{{ width|default(419.38) }}" viewBox="0 0 256.25 419.38"><g data-name="spider"><g><path d="M127.8 99l4.47-5.12L172 141.27l-22.69 38.48a3.83 3.83 0 0 1-5.73-.76l-.71-1.08a3.83 3.83 0 0 1 .71-5l19.72-31.05z" fill="#1d1e1c"/></g><path d="M130.57 102.16s17.64-21 12.76-30.69-7-8.9-14.17-7.75-11.76 5.63-10.73 13.82a40.17 40.17 0 0 0 12.14 24.62z" fill="#1d1e1c"/><g><path d="M118.53 99l-4.47-5.12-39.77 47.39L97 177.74a3.83 3.83 0 0 0 5.73-.76l.71-1.08a3.83 3.83 0 0 0-.71-5L83 141.81z" fill="#1d1e1c"/></g><path d="M115.76 102.16S98.11 81.12 103 71.47s7-8.9 14.17-7.75 11.76 5.63 10.73 13.82a40.17 40.17 0 0 1-12.14 24.62z" fill="#1d1e1c"/><g><path fill="#1d1e1c" d="M132.8 42.02l4.47-5.12 86.77 103.36-54.99 51.39-5.02-4.58 50.31-46.26-81.54-98.79z"/></g><path d="M136.17 45s17.15-26.3 10.29-36.59-9.15-9.15-17.15-6.86-12.58 8-10.31 17.15A45.93 45.93 0 0 0 136.17 45z" fill="#1d1e1c"/><g><path fill="#1d1e1c" d="M113.53 42.02l-4.47-5.12-86.77 103.36 55 51.39 5.01-4.58L32 140.81l81.53-98.79z"/></g><path d="M110.16 45S93 18.7 99.87 8.41 109-.74 117 1.55s12.58 8 10.29 17.15A45.93 45.93 0 0 1 110.16 45z" fill="#1d1e1c"/><g fill="#1d1e1c"><path d="M40.66 321.08L23.67 248.1l31.32.56 1.77 7.43-24.21-.5 15.36 63.08-7.25 2.41z"/><path d="M36.81 331s-6.21 11.73 5.36 16.56 17-5.51 17-5.51.89-4.49-4.79-14.32-7.38-11.9-10.56-12.43-7.01 15.7-7.01 15.7z"/></g><g fill="#1d1e1c"><path d="M21.27 383.93L0 246.05l57.07-25.51-.73 7.59-47.79 22.31L28.9 384l-7.63-.07z"/><path d="M14.57 396.19s-9.28 13 3.75 20.59 21.17-3.84 21.17-3.84 1.78-5.21-3.43-17.83-6.91-15.4-10.63-16.54-10.86 17.62-10.86 17.62z"/></g><g fill="#1d1e1c"><path d="M215.59 320.08l16.99-72.98-31.32.56-1.77 7.43 24.21-.5-15.36 63.08 7.25 2.41z"/><path d="M219.44 330s6.21 11.73-5.36 16.56-17-5.51-17-5.51-.89-4.49 4.79-14.32 7.4-11.93 10.59-12.46 6.98 15.73 6.98 15.73z"/></g><g fill="#1d1e1c"><path d="M234.98 382.93l21.27-137.88-57.07-25.51.73 7.59 47.8 22.31L227.36 383l7.62-.07z"/><path d="M241.68 395.19s9.28 13-3.75 20.59-21.17-3.84-21.17-3.84-1.78-5.21 3.43-17.83 6.91-15.4 10.63-16.54 10.86 17.62 10.86 17.62z"/></g><g><path d="M207.23 219.63c0 41-35.77 62.69-79.9 62.69s-77.67-21.69-77.67-62.69 33.54-71.22 77.67-71.22 79.9 30.22 79.9 71.22z" fill="#d2d2d2"/><path d="M126.33 285.72C103 285.72 83 279.93 68.58 269c-15.46-11.73-23.63-28.79-23.63-49.34 0-38.35 28.14-74.62 81.38-74.62 53.92 0 83.62 36.82 83.62 74.62 0 20.47-8.58 37.54-24.8 49.38-14.78 10.75-35.66 16.68-58.82 16.68zm0-133.91c-49.07 0-74 33.44-74 67.82 0 36.58 28.34 59.3 74 59.3 46.28 0 76.18-23.28 76.18-59.3 0-42.83-41.42-67.82-76.18-67.82z" fill="#1d1e1c"/></g><g><ellipse cx="92.29" cy="165.77" rx="37.55" ry="36.82" fill="#fff"/><path d="M91.29 205c-22 0-39.92-17.59-39.92-39.2s17.91-39.2 39.92-39.2 39.93 17.59 39.93 39.2S113.3 205 91.29 205zm0-73.65c-19.39 0-35.17 15.45-35.17 34.45s15.78 34.45 35.17 34.45 35.17-15.45 35.17-34.45-15.78-34.47-35.17-34.47z" fill="#1d1e1c"/></g><ellipse cx="95.19" cy="167.95" rx="16.99" ry="16.3" fill="#1d1e1c"/><circle cx="92.02" cy="165.04" r="4.68" fill="#fff"/><g><ellipse cx="166.29" cy="171.09" rx="45.51" ry="47.07" fill="#fff"/><path d="M165.29 220.53c-26.41 0-47.89-22.18-47.89-49.44s21.48-49.44 47.89-49.44 47.89 22.18 47.89 49.44-21.48 49.44-47.89 49.44zm0-94.13c-23.78 0-43.14 20-43.14 44.69s19.35 44.69 43.14 44.69 43.13-20 43.13-44.69-19.34-44.69-43.13-44.69z" fill="#1d1e1c"/></g><circle cx="169.13" cy="173.57" r="20.96" fill="#1d1e1c"/><circle cx="180.48" cy="170.39" r="5.78" fill="#fff"/></g></svg>

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

It's the spider? It's usual to load it using Twig?

This comment has been minimized.

@jdeniau

jdeniau Mar 23, 2018

Contributor

It seems to be the recommended way by Symfony. cf. cf. #1707 (comment)

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

height="{{ height|default(419.38) }}"

But IMO, these vars are useless (set directly w=24 h=24) and the file should be named api-platform.svg (without the .twig part) as Symfony recommends/does.

<span class="icon">
{{ include('@ApiPlatform/DataCollector/api-platform.svg.twig', { width: 24, height: 24 }) }}
</span>
<strong>Api Platform</strong>

This comment has been minimized.

@dunglas
<tbody>
<tr>
<td>
{{ collector.resourceClass|default('Not an api-platform resource') }}

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

API Platform

</td>
</tr>
{% endmacro %}

This comment has been minimized.

@dunglas

dunglas Mar 23, 2018

Member

Empty line to remove?

@dunglas

Thanks, awesome.

@dunglas

This comment has been minimized.

Member

dunglas commented Mar 23, 2018

Just a last minor thing. Is it possible to move the API Platform tab first in the list, or maybe just after the request/response tab?

public function __construct(ResourceNameCollectionFactoryInterface $collectionFactory, ResourceMetadataFactoryInterface $metadataFactory)
{
$this->collectionFactory = $collectionFactory;

This comment has been minimized.

@antograssiot

antograssiot Mar 23, 2018

Member

This doesn't seems to be used (yet/anymore) ?

{{ apiPlatform.operationLine('item', key, itemOperation) }}
{% endfor %}
</tbody>
<table>

This comment has been minimized.

@meyerbaptiste
@@ -0,0 +1,195 @@
{% extends '@WebProfiler/Profiler/layout.html.twig' %}

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

Can you add symfony/web-profiler-bundle as dev dependency?

@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" width="{{ width|default(256.25) }}" height="{{ width|default(419.38) }}" viewBox="0 0 256.25 419.38"><g data-name="spider"><g><path d="M127.8 99l4.47-5.12L172 141.27l-22.69 38.48a3.83 3.83 0 0 1-5.73-.76l-.71-1.08a3.83 3.83 0 0 1 .71-5l19.72-31.05z" fill="#1d1e1c"/></g><path d="M130.57 102.16s17.64-21 12.76-30.69-7-8.9-14.17-7.75-11.76 5.63-10.73 13.82a40.17 40.17 0 0 0 12.14 24.62z" fill="#1d1e1c"/><g><path d="M118.53 99l-4.47-5.12-39.77 47.39L97 177.74a3.83 3.83 0 0 0 5.73-.76l.71-1.08a3.83 3.83 0 0 0-.71-5L83 141.81z" fill="#1d1e1c"/></g><path d="M115.76 102.16S98.11 81.12 103 71.47s7-8.9 14.17-7.75 11.76 5.63 10.73 13.82a40.17 40.17 0 0 1-12.14 24.62z" fill="#1d1e1c"/><g><path fill="#1d1e1c" d="M132.8 42.02l4.47-5.12 86.77 103.36-54.99 51.39-5.02-4.58 50.31-46.26-81.54-98.79z"/></g><path d="M136.17 45s17.15-26.3 10.29-36.59-9.15-9.15-17.15-6.86-12.58 8-10.31 17.15A45.93 45.93 0 0 0 136.17 45z" fill="#1d1e1c"/><g><path fill="#1d1e1c" d="M113.53 42.02l-4.47-5.12-86.77 103.36 55 51.39 5.01-4.58L32 140.81l81.53-98.79z"/></g><path d="M110.16 45S93 18.7 99.87 8.41 109-.74 117 1.55s12.58 8 10.29 17.15A45.93 45.93 0 0 1 110.16 45z" fill="#1d1e1c"/><g fill="#1d1e1c"><path d="M40.66 321.08L23.67 248.1l31.32.56 1.77 7.43-24.21-.5 15.36 63.08-7.25 2.41z"/><path d="M36.81 331s-6.21 11.73 5.36 16.56 17-5.51 17-5.51.89-4.49-4.79-14.32-7.38-11.9-10.56-12.43-7.01 15.7-7.01 15.7z"/></g><g fill="#1d1e1c"><path d="M21.27 383.93L0 246.05l57.07-25.51-.73 7.59-47.79 22.31L28.9 384l-7.63-.07z"/><path d="M14.57 396.19s-9.28 13 3.75 20.59 21.17-3.84 21.17-3.84 1.78-5.21-3.43-17.83-6.91-15.4-10.63-16.54-10.86 17.62-10.86 17.62z"/></g><g fill="#1d1e1c"><path d="M215.59 320.08l16.99-72.98-31.32.56-1.77 7.43 24.21-.5-15.36 63.08 7.25 2.41z"/><path d="M219.44 330s6.21 11.73-5.36 16.56-17-5.51-17-5.51-.89-4.49 4.79-14.32 7.4-11.93 10.59-12.46 6.98 15.73 6.98 15.73z"/></g><g fill="#1d1e1c"><path d="M234.98 382.93l21.27-137.88-57.07-25.51.73 7.59 47.8 22.31L227.36 383l7.62-.07z"/><path d="M241.68 395.19s9.28 13-3.75 20.59-21.17-3.84-21.17-3.84-1.78-5.21 3.43-17.83 6.91-15.4 10.63-16.54 10.86 17.62 10.86 17.62z"/></g><g><path d="M207.23 219.63c0 41-35.77 62.69-79.9 62.69s-77.67-21.69-77.67-62.69 33.54-71.22 77.67-71.22 79.9 30.22 79.9 71.22z" fill="#d2d2d2"/><path d="M126.33 285.72C103 285.72 83 279.93 68.58 269c-15.46-11.73-23.63-28.79-23.63-49.34 0-38.35 28.14-74.62 81.38-74.62 53.92 0 83.62 36.82 83.62 74.62 0 20.47-8.58 37.54-24.8 49.38-14.78 10.75-35.66 16.68-58.82 16.68zm0-133.91c-49.07 0-74 33.44-74 67.82 0 36.58 28.34 59.3 74 59.3 46.28 0 76.18-23.28 76.18-59.3 0-42.83-41.42-67.82-76.18-67.82z" fill="#1d1e1c"/></g><g><ellipse cx="92.29" cy="165.77" rx="37.55" ry="36.82" fill="#fff"/><path d="M91.29 205c-22 0-39.92-17.59-39.92-39.2s17.91-39.2 39.92-39.2 39.93 17.59 39.93 39.2S113.3 205 91.29 205zm0-73.65c-19.39 0-35.17 15.45-35.17 34.45s15.78 34.45 35.17 34.45 35.17-15.45 35.17-34.45-15.78-34.47-35.17-34.47z" fill="#1d1e1c"/></g><ellipse cx="95.19" cy="167.95" rx="16.99" ry="16.3" fill="#1d1e1c"/><circle cx="92.02" cy="165.04" r="4.68" fill="#fff"/><g><ellipse cx="166.29" cy="171.09" rx="45.51" ry="47.07" fill="#fff"/><path d="M165.29 220.53c-26.41 0-47.89-22.18-47.89-49.44s21.48-49.44 47.89-49.44 47.89 22.18 47.89 49.44-21.48 49.44-47.89 49.44zm0-94.13c-23.78 0-43.14 20-43.14 44.69s19.35 44.69 43.14 44.69 43.13-20 43.13-44.69-19.34-44.69-43.13-44.69z" fill="#1d1e1c"/></g><circle cx="169.13" cy="173.57" r="20.96" fill="#1d1e1c"/><circle cx="180.48" cy="170.39" r="5.78" fill="#fff"/></g></svg>

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

height="{{ height|default(419.38) }}"

But IMO, these vars are useless (set directly w=24 h=24) and the file should be named api-platform.svg (without the .twig part) as Symfony recommends/does.

{% endif %}
{% endblock %}
{% macro operationLine(itemOrCollection, key, operation) %}

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

Can you move the macro tag before the import tag?

public function getMethod(): string
{
return $this->data['method'];

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

What if this index does not exist? (same for others getters)

This comment has been minimized.

@jdeniau

jdeniau Mar 28, 2018

Contributor

They always exists (if collect has been called), as Request::getMethod returns a string

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 28, 2018

Member

if collect has been called

That is what I wanted to point out! Perhaps an exception would be more appropriate than a fatal error in this particular case.

This comment has been minimized.

@meyerbaptiste

This comment has been minimized.

@jdeniau

jdeniau Mar 29, 2018

Contributor

Oops, I missed that one today :), will be OK in the next return fixes in a few minutes :)

This comment has been minimized.

@jdeniau

jdeniau Mar 29, 2018

Contributor

@meyerbaptiste done, But I preferred having a default value to '' because it's seems really overkill to throw an exception for something that should never happened.

Yes, theoretically, collect() may not have been called, but in that case, the developer probably made something really unusual. If Symfony manage the datacollectors, then everything is OK.

/**
* {@inheritdoc}
*/
public function reset(): void

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

void is for PHP >= 7.1!

$this->metadataFactory = $metadataFactory;
}
public function collect(Request $request, Response $response, \Exception $exception = null): void

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

void is for PHP >= 7.1!

Add {@inheritdoc}.

return $this->data['resource_metadata'];
}
public function getName(): string

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

{@inheritdoc}

* {@inheritdoc}
*/
public function reset(): void
{

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

$this->data = [];?

{% block menu %}
{# This left-hand menu appears when using the full-screen profiler. #}
<span class="label">
<span class="icon">

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 26, 2018

Member

WDYT about adding a disabled class when there is no data?

@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Mar 28, 2018

*/
private function registerDataCollector(ContainerBuilder $container, array $config, XmlFileLoader $loader)
{
if (!$container->getParameter('kernel.debug')) {

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 28, 2018

Member

After reflection, this is the bad way. In fact, kernel.debug does not affect the profiler and the data collection. The only way to disable this is with the FrameworkBundle:

framework:
  profiler:
    enabled: false

So, you can remove this condition but IMO it would be nice to add configuration to enable our data collector (ArrayNodeDefinition::canBeEnabled()).

This comment has been minimized.

@jdeniau

jdeniau Mar 29, 2018

Contributor

@meyerbaptiste done, I added an enable_profiler key in the configuration

@@ -101,6 +101,7 @@ public function getConfigTreeBuilder()
->booleanNode('enable_swagger_ui')->defaultValue(class_exists(TwigBundle::class))->info('Enable Swagger ui.')->end()
->booleanNode('enable_entrypoint')->defaultTrue()->info('Enable the entrypoint')->end()
->booleanNode('enable_docs')->defaultTrue()->info('Enable the docs')->end()
->booleanNode('enable_profiler')->defaultTrue()->info('Enable Symfony profiler')->end()

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 29, 2018

Member

Enable the data collector and the WebProfilerBundle integration.?

{% block menu %}
{# This left-hand menu appears when using the full-screen profiler. #}
<span class="label{{ collector.resourceClass? '' : ' disabled' }}">

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 29, 2018

Member

Can you add a space before ?.

*/
private function registerDataCollector(ContainerBuilder $container, array $config, XmlFileLoader $loader)
{
if (false === $config['enable_profiler']) {

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 29, 2018

Member

!$config['enable_profiler']

public function getResourceClass()
{
return $this->data['resource_class'];

This comment has been minimized.

@meyerbaptiste
public function getResourceMetadata()
{
return $this->data['resource_metadata'];

This comment has been minimized.

@meyerbaptiste
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\DataCollector\DataCollector;
final class RequestDataCollector extends DataCollector

This comment has been minimized.

@meyerbaptiste

meyerbaptiste Mar 29, 2018

Member

Can you add unit tests for this class?

This comment has been minimized.

@jdeniau

jdeniau Mar 29, 2018

Contributor

@meyerbaptiste unit test added

@meyerbaptiste

Good job @jdeniau and sorry about the long review!

@jdeniau

This comment has been minimized.

Contributor

jdeniau commented Mar 30, 2018

@soyuka

soyuka approved these changes Mar 30, 2018

@soyuka

This comment has been minimized.

Member

soyuka commented Mar 30, 2018

Looks really good 👍 can't wait to use it :D

@dunglas dunglas merged commit 9d62eba into api-platform:master Apr 3, 2018

4 checks passed

Scrutinizer 1 new issues, 10 updated code elements
Details
brigade Brigade build passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dunglas

This comment has been minimized.

Member

dunglas commented Apr 3, 2018

Awesome! Thank you very much @jdeniau.

@jdeniau jdeniau deleted the jdeniau:jd-feat-addSymfonyDataCollector branch Apr 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment