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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Messenger] Clone messages to show in profiler #26650

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@Nyholm
Member

Nyholm commented Mar 23, 2018

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 na

We should make the profiler page more pretty by using the cloner.

screen shot 2018-03-23 at 11 08 02

@@ -50,10 +60,9 @@
{% block toolbar %}
{% set color_code = 'normal' %}
{% set message_count = 0 %}

This comment has been minimized.

@Nyholm

Nyholm Mar 23, 2018

Member

@sroze Any reason why this was zero?

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

Nop, very likely a mistake 🙊

@@ -31,7 +31,13 @@
<tbody>
{% for message in collector.messages %}
<tr>
<td>{{ message.message.type }}</td>
<td>
{{ message.message.type }}

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

I don't believe we still need to display the type anymore if we use the dumper, as the FQCN is available as overlay :)

@@ -40,6 +46,10 @@
{% if message.exception.type is defined %}
{{ message.exception.type }}
{% endif %}
{% if message.result.object is defined %}

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

Same idea. If defined, I'd display this instead of the message.result.type.

@@ -50,10 +60,9 @@
{% block toolbar %}
{% set color_code = 'normal' %}
{% set message_count = 0 %}

This comment has been minimized.

@sroze

sroze Mar 23, 2018

Member

Nop, very likely a mistake 🙊

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 23, 2018

Thank you for the review

screen shot 2018-03-23 at 11 27 31

@@ -63,6 +64,7 @@ public function handle($message, callable $next)
if (is_object($result)) {
$debugRepresentation['result'] = array(
'type' => get_class($result),
'object' => $this->cloneVar($result),
);
} else {

This comment has been minimized.

@stof

stof Mar 23, 2018

Member

should be cloned as well in case it is an array, as it might contain objects inside the array

This comment has been minimized.

@stof

stof Mar 23, 2018

Member

btw, the template does not seem to use the value in this case when rendering the info, so why storing it ?

@chalasr chalasr added this to the 4.1 milestone Mar 23, 2018

@chalasr chalasr added the Messenger label Mar 23, 2018

@stof

This comment has been minimized.

Member

stof commented Mar 23, 2018

IMO, messages should not be stored directly in $this->data in the collector, as this forbids us to store anything else in the future in this collector in case we need to collect more things. You should probably move this to $this->data['messages'] instead.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 23, 2018

Thank you for the review

@sroze

sroze approved these changes Mar 24, 2018

Looks good to me 👍 (after squashing the commits, obsly)

@sroze

This comment has been minimized.

Member

sroze commented Mar 26, 2018

@Nyholm could you squash your commits?
@stof can you have another look please?

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 26, 2018

Done

@sroze

sroze approved these changes Mar 26, 2018

Failure in un-related 👍

@@ -77,7 +84,7 @@ public function handle($message, callable $next)
);
}
$this->data[] = $debugRepresentation;
$this->data['messages'][] = $debugRepresentation;

This comment has been minimized.

@stof

stof Mar 26, 2018

Member

collect must ensure that $this->data['messages'] is always an array, even when no message was handled there, to make getMessages work without notice after collect has been called.

This comment has been minimized.

@Nyholm

Nyholm Mar 26, 2018

Member

Thank you

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

@Nyholm your changes did not fix this btw 😉

This comment has been minimized.

@Nyholm

Nyholm Mar 26, 2018

Member

Doesn't it?
Cant I trust that $this->data always to be null or an array?

https://3v4l.org/tubTL

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

Actually, you're right, it won't "throw" a PHP notice.

@@ -77,7 +84,7 @@ public function handle($message, callable $next)
);
}
$this->data[] = $debugRepresentation;
$this->data['messages'][] = $debugRepresentation;

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

@Nyholm your changes did not fix this btw 😉

@@ -77,7 +84,7 @@ public function handle($message, callable $next)
);
}
$this->data[] = $debugRepresentation;
$this->data['messages'][] = $debugRepresentation;

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member

Actually, you're right, it won't "throw" a PHP notice.

@@ -88,6 +95,10 @@ public function handle($message, callable $next)
public function getMessages(): array
{
return $this->data;
if (!isset($this->data['messages'])) {

This comment has been minimized.

@sroze

sroze Mar 26, 2018

Member
return $this->data['messages'] ?? array();

(note the array(), this will make fabbot happy)

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 26, 2018

Thank you

Nyholm added some commits Mar 23, 2018

cs
@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 27, 2018

I've rebased this PR now. Im ready for a final review.

@fabpot

fabpot approved these changes Mar 27, 2018

@stof

stof approved these changes Mar 27, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 27, 2018

Thank you @Nyholm.

@fabpot fabpot closed this Mar 27, 2018

fabpot added a commit that referenced this pull request Mar 27, 2018

feature #26650 [Messenger] Clone messages to show in profiler (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        | na

We should make the profiler page more pretty by using the cloner.

![screen shot 2018-03-23 at 11 08 02](https://user-images.githubusercontent.com/1275206/37823687-816a373a-2e8a-11e8-824e-ac7f96a51e3b.png)

Commits
-------

4d1be87 [Messenger] Clone messages to show in profiler

@Nyholm Nyholm deleted the Nyholm:message-profiler-clone branch Mar 27, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment