Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
DoctrineDataCollector: taught sanitizeParam to support classes with __toString implemented.#20680
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
nicolas-grekas commentedDec 1, 2016
I think we need to dump the class name all the time. At least this is what Monolog does in this case: |
FractalizeR commentedDec 2, 2016
Monolog in that source does both. It uses class name as a key, and __toString() call result as a value. |
nicolas-grekas commentedDec 6, 2016
@FractalizeR that's what I meant :) And I think that's wise. |
| if (is_object($var)) { | ||
| returnarray(sprintf('Object(%s)',get_class($var)),false); | ||
| returnmethod_exists($var,'__toString') ? | ||
| array($var->__toString(),false) : |
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.
I agree that the class name should be dumped here as well. The way it is implemented will cause confusing results.
fabpot commentedFeb 16, 2017
What's the status of this PR? |
fabpot commentedMar 1, 2017
@FractalizeR Any news? Are still interested in finishing this PR? |
FractalizeR commentedMar 1, 2017
Yeah, sure. But I am at lost who makes a decision about how this PR should really look like. I'm ready to make desired modifications, but someone responsible needs to tell me what needs to be adjusted. |
fabpot commentedMar 1, 2017
I think we all agree that we need to keep the class name, even when there is a string representation. |
FractalizeR commentedMar 1, 2017
Can we, then, use the following representation for objects, implementing ? |
fabpot commentedMar 1, 2017
@FractalizeR I think that makes sense. |
14e2631 tod3f9d16CompareFractalizeR commentedMar 2, 2017
Done. Some tests fail, but this is not due my PR, but several other components. Looks like some PR was merged without ensuring tests pass. |
| publicfunction__toString() | ||
| { | ||
| return$this->representation; |
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.
Can we simplify this class by removing therepresentation property and hardcode the text here directly?
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.
see26c54c7
fabpot commentedMar 2, 2017
Thank you@FractalizeR. |
This PR teaches \Symfony\Bridge\Doctrine\DataCollector\DoctrineDataCollector::sanitizeParam support objects, which implement __toString and therefore can be represented as a string with more sense, than "(object) ClassName". It also includes test for the feature.