Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Clone messages to show in profiler#26650
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
| {%blocktoolbar %} | ||
| {%setcolor_code='normal' %} | ||
| {%setmessage_count=0 %} |
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.
@sroze Any reason why this was zero?
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.
Nop, very likely a mistake 🙊
| <tr> | ||
| <td>{{message.message.type }}</td> | ||
| <td> | ||
| {{message.message.type }} |
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 don't believe we still need to display the type anymore if we use the dumper, as the FQCN is available as overlay :)
| {{message.exception.type }} | ||
| {%endif %} | ||
| {%ifmessage.result.objectisdefined %} |
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.
Same idea. If defined, I'd display this instead of themessage.result.type.
| {%blocktoolbar %} | ||
| {%setcolor_code='normal' %} | ||
| {%setmessage_count=0 %} |
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.
Nop, very likely a mistake 🙊
Nyholm commentedMar 23, 2018
Thank you for the review
|
| 'type' =>get_class($result), | ||
| 'object' =>$this->cloneVar($result), | ||
| ); | ||
| }else { |
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.
should be cloned as well in case it is an array, as it might contain objects inside the array
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 template does not seem to use the value in this case when rendering the info, so why storing it ?
stof commentedMar 23, 2018
IMO, messages should not be stored directly in |
Nyholm commentedMar 23, 2018
Thank you for the review |
sroze left a comment
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.
Looks good to me 👍 (after squashing the commits, obsly)
sroze commentedMar 26, 2018
5700de0 to52f4da9CompareNyholm commentedMar 26, 2018
Done |
sroze left a comment
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.
Failure in un-related 👍
| } | ||
| $this->data[] =$debugRepresentation; | ||
| $this->data['messages'][] =$debugRepresentation; |
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.
collect must ensure that$this->data['messages'] is always an array, even when no message was handled there, to makegetMessages work without notice aftercollect has been called.
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.
Thank you
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.
@Nyholm your changes did not fix this btw 😉
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.
Doesn't it?
Cant I trust that$this->data always to be null or an array?
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.
Actually, you're right, it won't "throw" a PHP notice.
| } | ||
| $this->data[] =$debugRepresentation; | ||
| $this->data['messages'][] =$debugRepresentation; |
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.
@Nyholm your changes did not fix this btw 😉
| } | ||
| $this->data[] =$debugRepresentation; | ||
| $this->data['messages'][] =$debugRepresentation; |
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.
Actually, you're right, it won't "throw" a PHP notice.
| publicfunctiongetMessages():array | ||
| { | ||
| return$this->data; | ||
| if (!isset($this->data['messages'])) { |
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.
return$this->data['messages'] ??array();
(note thearray(), this will make fabbot happy)
Nyholm commentedMar 26, 2018
Thank you |
ff354df to359a658CompareNyholm commentedMar 27, 2018
I've rebased this PR now. Im ready for a final review. |
fabpot commentedMar 27, 2018
Thank you@Nyholm. |
This PR was squashed before being merged into the 4.1-dev branch (closes#26650).Discussion----------[Messenger] Clone messages to show in profiler| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | na| License | MIT| Doc PR | naWe should make the profiler page more pretty by using the cloner.Commits-------4d1be87 [Messenger] Clone messages to show in profiler
Uh oh!
There was an error while loading.Please reload this page.
We should make the profiler page more pretty by using the cloner.