Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form][EventDispatcher] Fix VarDumper usage related to perf regression#19986
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
992bf19 tob31562dComparenicolas-grekas commentedSep 20, 2016
Not that this PR will also fix the remaining failure on Travis on 3.1. |
23a5e1f to2be77f5Compare| }, | ||
| )); | ||
| }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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this will say that usingFormDataCollector::cloneVar without the component is deprecated, but this is a private method. You should probably talk about the public API triggering it instead
nicolas-grekasSep 20, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This has to be done this way: the implementation is changed when the component is not loaded: a string is returned. This mirrors theprofiler_dump function that is able to cope with bothData and strings (deprecated). There is nothing related on the public surface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Btw, the same is donein the baseDataCollector for BC.
| returnarray( | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't you include the message too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just checkedin the original code. This just mirrors it. LGTM.
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| protectedfunctioncloneVar($var) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
must be protected (as in parent)
nicolas-grekas commentedSep 20, 2016
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 | ||
| */ | ||
| privatefunctiongetListenerInfo($listener,$eventName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 cleanupalready 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.
| if (null ===$this->cloner) { | ||
| if (class_exists(ClassStub::class)) { | ||
| $this->cloner =newVarCloner(); | ||
| $this->cloner->setMaxItems(250); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the default is 2500, too much for here
fabpot commentedSep 21, 2016
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/ANot used anymore after#19986EDIT: add missing `use` too.Commits-------0708003 [Form] Remove unused var cloner property
Uh oh!
There was an error while loading.Please reload this page.