[Profiler][Validator] Add a validator panel in profiler #22554
Conversation
| xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> | ||
|
|
||
| <services> | ||
| <service id="debug.validator" decorates="validator" class="Symfony\Component\Validator\Validator\TraceableValidator"> |
There was a problem hiding this comment.
this service should be private, as you should never get it at runtime explicitly as debug.validator
| {% endblock %} | ||
|
|
||
| {% block head %} | ||
| {{ parent() }} |
There was a problem hiding this comment.
useless as you don't add anything. Skip overwriting the block
| <th>Violation</th> | ||
| </tr> | ||
| </thead> | ||
| {% for violationDate in call.violations %} |
There was a problem hiding this comment.
Should be data. Good catch.
| */ | ||
| public function startContext() | ||
| { | ||
| $this->validator->startContext(); |
|
(about the icon, we'll ask to tweak it to the designer who created the rest of the profiler icons) |
|
@stof: I forgot to mention this is a POC/WIP (even if I think I'm almost done for the panel itself). |
|
Looks great to me :) |
|
PR updated:
|
| public function lateCollect() | ||
| { | ||
| foreach ($this->validator->getCollectedData() as $collected) { | ||
| $this->data['calls'][] = $this->cloneVar($collected); |
There was a problem hiding this comment.
Doing a single clone for all calls would improve perf and size of the resulting serialized payload.
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protected function cloneVar($var, $isClass = false) |
There was a problem hiding this comment.
Anywayto reuse the parent?
There was a problem hiding this comment.
I don't think so? I need to add a specific caster for FormInterface, and the static $cloner property in the parent is private. Unless we want to open it? (I based this method on FormDataCollector::cloneVar)
There was a problem hiding this comment.
Would it be worth adding an empty DataCollector::getCasters() method to implement in children when needed, or a DataCollector::configureCloner(VarCloner $cloner) one? (making the property non-static anymore btw)
| @@ -22,6 +22,8 @@ | |||
| }, | |||
| "require-dev": { | |||
| "symfony/http-foundation": "~2.8|~3.0", | |||
| "symfony/http-kernel": "~2.8|~3.0", | |||
| "symfony/var-dumper": "~2.8|~3.0", | |||
There was a problem hiding this comment.
@nicolas-grekas : Do you know what would be the proper version constraint to avoid this issue with the var-dumper component?
There was a problem hiding this comment.
NVM. Anyway 3.3 is required for #21638.
|
@ogizanagi This looks wonderful! |
| </service> | ||
|
|
||
| <!-- DataCollector --> | ||
| <service id="data_collector.validator" class="Symfony\Component\Validator\DataCollector\ValidatorDataCollector"> |
There was a problem hiding this comment.
Updated to use <defaults public="false" />
| @@ -434,6 +435,10 @@ private function registerProfilerConfiguration(array $config, ContainerBuilder $ | |||
| $loader->load('form_debug.xml'); | |||
| } | |||
|
|
|||
| if ($this->validatorConfigEnabled) { | |||
There was a problem hiding this comment.
We also need to ensure that the Symfony\Component\Validator\Validator\TraceableValidator does exist to prevent issue when FrameworkBundle 3.4 is used with older versions of the Validator component.
There was a problem hiding this comment.
simpler solution is to add a conflict rule in FrameworkBundle to forbid using an older version of symfony/validator when using FrameworkBundle 3.4
There was a problem hiding this comment.
I've updated the conflict rule.
| </div> | ||
| <div class="sf-toolbar-info-piece"> | ||
| <b>Number of violations</b> | ||
| <span class="sf-toolbar-status sf-toolbar-status-{{ collector.data.nb_violations > 0 ? 'red' }}">{{ collector.data.nb_violations }}</span> |
There was a problem hiding this comment.
{{ collector.data.nb_violations > 0 ? 'sf-toolbar-status-red' }} (otherwise there will be a sf-toolbar-status- class)
| {% set icon %} | ||
| {{ include('@WebProfiler/Icon/validator.svg') }} | ||
| <span class="sf-toolbar-value"> | ||
| {{ collector.data.nb_violations ?: collector.data.calls|length }} |
There was a problem hiding this comment.
Not sure if it is a good idea that the displayed property may vary.
There was a problem hiding this comment.
I agree with @xabbuh here. It looks weird to switch between violation and calls.
There was a problem hiding this comment.
I agree. I started the template from the Forms collector one. It's a mistake due to fact I overlooked at it in the first place. 😄 (but I wonder if it really make more sense for forms to switch between errors and forms count)
We should always show the number of violations instead.
| }, 0); | ||
| } | ||
|
|
||
| public function getData() |
There was a problem hiding this comment.
Other collectors do not expose the $data property, but provide concrete getter methods. I think we should do the same here.
There was a problem hiding this comment.
Not true for the form collector for instance. But I guess getCalls() and getViolationsCount() makes sense :)
| use Symfony\Component\VarDumper\Caster\Caster; | ||
| use Symfony\Component\VarDumper\Caster\ClassStub; | ||
| use Symfony\Component\VarDumper\Cloner\Data; | ||
| use Symfony\Component\VarDumper\Cloner\VarCloner; |
There was a problem hiding this comment.
We should probably exit early if the VarDumper component is not installed.
There was a problem hiding this comment.
Not sure it's needed. It seems to me the symfony/var-dumper is a hard requirement to use the profiler since 3.3 (and required by the WebProfilerBundle): https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/DataCollector/DataCollector.php#L69
| xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> | ||
|
|
||
| <services> | ||
| <service id="debug.validator" decorates="validator" class="Symfony\Component\Validator\Validator\TraceableValidator" public="false"> |
There was a problem hiding this comment.
Should we define a priority here to avoid issues if the validator is decorated somewhere else?
There was a problem hiding this comment.
I think it's a good idea. Which priority would you use? "Highest" one (i.e 255 even if there is no real limit)? So it'll be applied first and no one relying on decorating the validator with its own class would get surprised.
|
@javiereguiluz I think it would be good to work on the icons. |
|
I might be able to throw an SVG icon together, but what kind of icon would be appropriate? The check mark used in the example seems too generic. |
|
@javiereguiluz : Any update about the new SVG icon? :) |
|
@ogizanagi thanks for reminding me about this. I've just asked for this to our designer. I'll share here his proposal. Thanks! |
|
This is the icon created by our designer: And this is how it looks:
|
|
The new icon "looks" bigger than the existing one (perhaps you can also add a screenshot when it is not selected?). |
|
Here is unhighlighted:
|
|
I would make it a bit smaller if possible. |
|
Here is the new version: <svg xmlns="http://www.w3.org/2000/svg" x="0px" y="0px" width="24" height="24" viewBox="0 0 24 24" enable-background="new 0 0 24 24" xml:space="preserve"><path fill="#aaaaaa" d="M19.54,22.5H4.29a2.88,2.88,0,0,1-2.87-2.87V4.37A2.88,2.88,0,0,1,4.29,1.5H18.54a1,1,0,0,1,0,2H4.29a.88.88,0,0,0-.87.87V19.63a.88.88,0,0,0,.87.87H19.54a.88.88,0,0,0,.87-.87V11.29a1,1,0,1,1,2,0v8.33A2.88,2.88,0,0,1,19.54,22.5ZM13,17.29,22.88,6a1.5,1.5,0,1,0-2.26-2L12,14,8,9.11A1.5,1.5,0,0,0,5.65,11l5.1,6.25a1.5,1.5,0,0,0,1.14.55h0A1.5,1.5,0,0,0,13,17.29Z"/></svg>And this is how it looks:
|
|
@ogizanagi I think you can update this PR with the new icon. |
|
Updated with the new icon. Many thanks to you and your designer. ;) |
|
(deps=high failure should be fixed once merged, in order for the FrameworkBundle to be able to see the validator component's collector) |
|
Thank you @ogizanagi. |
…r (ogizanagi) This PR was merged into the 3.4 branch. Discussion ---------- [Profiler][Validator] Add a validator panel in profiler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A I'm exploring the possibility of having a validator panel in the profiler. The integration in the form panel is great, but there are a lot of other use-cases where you're likely to call the validator. The idea of this panel is to reference every calls made to the validator (`ValidatorInterface::validate()` at least) along with detailed informations. Dealing with apis and a mobile app, it's not always easy to get the response body within the app to get what's wrong with the call. So now with this panel, I'm able to get the details without the api response. In action with Symfony demo (on the admin new post form):   On another app, by calling the validator elsewhere: |No violations|With violations| |--|--| ||| What do you think ? --- Note: the SVG icon used should be changed. If anyone is willing to contribute and provide one, I'll be glad to add it! Commits ------- ac5e884 [Profiler][Validator] Add a validator panel in profiler
|
@ogizanagi OT: I love the gifs. What do you use to make them? |
|
I'm using Giphy Capture. Before, I used to make a screen cast video (using Quicktime) and convert it using a custom alias calling ffmpeg (alias which I don't have anymore :/). But I'm sure there are tons of other solutions, for instance https://github.com/phw/peek, http://www.cockos.com/licecap/ (but the last one not for linux AFAIK), ... |
|
Thanks. I thought that might be some nice Mac tool. I'm using Peek now, but it is very simple and doesn't have that "click circle" feature in it. |
…th the form one (ogizanagi) This PR was merged into the 3.4 branch. Discussion ---------- [Profiler] Make the validator toolbar item consistent with the form one | Q | A | ------------- | --- | Branch? | 3.4 <!-- see comment below --> | Bug fix? | no | New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget updating UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | N/A <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | N/A After more time experiencing it, I changed my mind about #22554 (review). I do think replicating the form toolbar item behavior is the best thing to do. Displaying a different property under some conditions may seem to be a bad idea, but the difference is clearly expressed by the background color used: |-|-| |--|--| |<img alt="screenshot 2017-08-09 a 15 59 56" src="https://user-images.githubusercontent.com/2211145/29125368-de340da4-7d1b-11e7-9b04-8ff395e118f0.PNG">| Red: something wrong happened. Shows the relevant information: the number of violations.| |<img alt="screenshot 2017-08-09 a 16 00 05" src="https://user-images.githubusercontent.com/2211145/29125374-e28dd6a0-7d1b-11e7-8ed6-b0d7634a5b21.PNG">| Grey: Everything went fine. Acknowledge the validator calls were made and passed by showing the number of calls.| In any case, everything is clear when hovering the toolbar item (and most users are probably already used to this behavior due to the form item): |Violations|No violations| |--|--| |<img width="173" alt="screenshot 2017-08-09 a 15 51 18" src="https://user-images.githubusercontent.com/2211145/29125023-e35ca602-7d1a-11e7-82d5-0ff3c0d9c46c.PNG">|<img width="172" alt="screenshot 2017-08-09 a 15 51 31" src="https://user-images.githubusercontent.com/2211145/29125037-ebbf0614-7d1a-11e7-8cd2-de6cb963a282.PNG">| Commits ------- b622d26 [Profiler] Make the validator toolbar item consistent with the form one
I'm exploring the possibility of having a validator panel in the profiler.
The integration in the form panel is great, but there are a lot of other use-cases where you're likely to call the validator. The idea of this panel is to reference every calls made to the validator (
ValidatorInterface::validate()at least) along with detailed informations.Dealing with apis and a mobile app, it's not always easy to get the response body within the app to get what's wrong with the call. So now with this panel, I'm able to get the details without the api response.
In action with Symfony demo (on the admin new post form):
On another app, by calling the validator elsewhere:
What do you think ?
Note: the SVG icon used should be changed. If anyone is willing to contribute and provide one, I'll be glad to add it!