[Form][EventDispatcher] Fix VarDumper usage related to perf regression #19986
Conversation
nicolas-grekas
commented
Sep 20, 2016
•
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #19978 |
| License | MIT |
| Doc PR | - |
78bb423 to
a51935a
Compare
a51935a to
97f06d3
Compare
992bf19 to
b31562d
Compare
|
Not that this PR will also fix the remaining failure on Travis on 3.1. |
23a5e1f to
2be77f5
Compare
| }, | ||
| )); | ||
| } else { | ||
| @trigger_error(sprintf('Using the %s() method without the VarDumper component is deprecated since version 3.2 and won\'t be supported in 4.0. Install symfony/var-dumper version 3.2 or above.', __METHOD__), E_USER_DEPRECATED); |
There was a problem hiding this comment.
this will say that using FormDataCollector::cloneVar without the component is deprecated, but this is a private method. You should probably talk about the public API triggering it instead
There was a problem hiding this comment.
This has to be done this way: the implementation is changed when the component is not loaded: a string is returned. This mirrors the profiler_dump function that is able to cope with both Data and strings (deprecated). There is nothing related on the public surface.
There was a problem hiding this comment.
Btw, the same is done in the base DataCollector for BC.
| return array( | ||
| Caster::PREFIX_VIRTUAL.'root' => $v->getRoot(), | ||
| Caster::PREFIX_VIRTUAL.'path' => $v->getPropertyPath(), | ||
| Caster::PREFIX_VIRTUAL.'value' => $v->getInvalidValue(), |
There was a problem hiding this comment.
shouldn't you include the message too ?
There was a problem hiding this comment.
Just checked in the original code. This just mirrors it. LGTM.
2be77f5 to
57a9a85
Compare
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protected function cloneVar($var) |
There was a problem hiding this comment.
must be protected (as in parent)
57a9a85 to
224a8e0
Compare
224a8e0 to
294868e
Compare
|
Now limiting to max-depth=2, which looks more generic than 50 items to me. Internal cache also added to prevent re-cloning the same structures several times. Enhances both perf and serialize payload size. |
| * | ||
| * @return array Information about the listener | ||
| */ | ||
| private function getListenerInfo($listener, $eventName) |
There was a problem hiding this comment.
This method is removed for two reasons:
- computed values are not used anymore in the profiler panel, only the data-clones are (same king of cleanup already done in
FormDataExtractorand inDebugAccessDecisionManager); - to prevent computing the same info several times, it needs to be persistent, and this responsibility belongs to the
WrappedListenerclass, that knows best about its wrapped listener.
| @@ -67,6 +67,7 @@ protected function cloneVar($var) | |||
| if (null === $this->cloner) { | |||
| if (class_exists(ClassStub::class)) { | |||
| $this->cloner = new VarCloner(); | |||
| $this->cloner->setMaxItems(250); | |||
There was a problem hiding this comment.
the default is 2500, too much for here
|
Thank you @nicolas-grekas. |
…f regression (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [Form][EventDispatcher] Fix VarDumper usage related to perf regression | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19978 | License | MIT | Doc PR | - Commits ------- 294868e [Form][EventDispatcher] Fix VarDumper usage related to perf regression
This PR was merged into the 3.2 branch. Discussion ---------- [Form] Remove unused var cloner property | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Not used anymore after #19986 EDIT: add missing `use` too. Commits ------- 0708003 [Form] Remove unused var cloner property