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

[WebProfiler] Expressions could get a better display in the panel #24127

Closed
stof opened this Issue Sep 7, 2017 · 10 comments

Comments

5 participants
@stof
Member

stof commented Sep 7, 2017

Currently, voting on an expression displays a panel like that:

expression_display

(This is a SerializedParsedExpression rather than just an Expression because it is the vote for an expression used in the access_control).

We could improve this display when encountering an Expression object by displaying its string representation instead (which will be the expression itself).

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot
Member

fabpot commented Sep 7, 2017

👍 /cc @javiereguiluz

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 7, 2017

Member

I agree. The attributes table cell is displayed with:

<td>
    {% if decision.attributes|length == 1 %}
        {{ decision.attributes|first }}
    {% else %}
        {{ profiler_dump(decision.attributes) }}
    {% endif %}
</td>

So this must be done in profiler_dump(). @nicolas-grekas do you think this is feasible and can be implemented without breaking other features? Thanks!

Member

javiereguiluz commented Sep 7, 2017

I agree. The attributes table cell is displayed with:

<td>
    {% if decision.attributes|length == 1 %}
        {{ decision.attributes|first }}
    {% else %}
        {{ profiler_dump(decision.attributes) }}
    {% endif %}
</td>

So this must be done in profiler_dump(). @nicolas-grekas do you think this is feasible and can be implemented without breaking other features? Thanks!

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 7, 2017

Member

this does not look like a VarDumper output in my screenshot

Member

stof commented Sep 7, 2017

this does not look like a VarDumper output in my screenshot

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 7, 2017

Member

OK, the issue is that {{ decision.attributes|first }} casts the first attribute to a string. But the attribute is not a SerializedParsedExpression anymore here, but a VarDumper Data object.

Btw, when I dump the internals of this object, the expression property seems to contain lots of crap instead of a string value, so it looks like the VarDumper is buggy there (using 3.3.8).

I think we would have to cast the Expression object to string when collecting the access decision log instead.

Member

stof commented Sep 7, 2017

OK, the issue is that {{ decision.attributes|first }} casts the first attribute to a string. But the attribute is not a SerializedParsedExpression anymore here, but a VarDumper Data object.

Btw, when I dump the internals of this object, the expression property seems to contain lots of crap instead of a string value, so it looks like the VarDumper is buggy there (using 3.3.8).

I think we would have to cast the Expression object to string when collecting the access decision log instead.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 7, 2017

Member

Another solution I found:

{% if decision.attributes|length == 1 %}
    {% set attribute = decision.attributes|first %}
    {% if attribute.type in [
        'Symfony\\Component\\ExpressionLanguage\\SerializedParsedExpression',
        'Symfony\\Component\\ExpressionLanguage\\ParsedExpression',
        'Symfony\\Component\\ExpressionLanguage\\Expression'
    ] %}
        {% set attribute = attribute.expression %}
    {% endif %}
    {{ attribute }}
{% else %}
    {{ profiler_dump(decision.attributes) }}
{% endif %}

But it is a big ugly to have to list all classes of expressions

Member

stof commented Sep 7, 2017

Another solution I found:

{% if decision.attributes|length == 1 %}
    {% set attribute = decision.attributes|first %}
    {% if attribute.type in [
        'Symfony\\Component\\ExpressionLanguage\\SerializedParsedExpression',
        'Symfony\\Component\\ExpressionLanguage\\ParsedExpression',
        'Symfony\\Component\\ExpressionLanguage\\Expression'
    ] %}
        {% set attribute = attribute.expression %}
    {% endif %}
    {{ attribute }}
{% else %}
    {{ profiler_dump(decision.attributes) }}
{% endif %}

But it is a big ugly to have to list all classes of expressions

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 8, 2017

Member

@stof can you post a reproducer for the "crap" issue? That may help solve other related ones?

Member

nicolas-grekas commented Sep 8, 2017

@stof can you post a reproducer for the "crap" issue? That may help solve other related ones?

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Feb 21, 2018

Member

@nicolas-grekas here is the output of doing a dump(decision.attributes) in this place in the code of the template (the last line in the screen is the existing {{ decision.attributes|first }}):

Internal data dump

image

And here is the output when doing profiler_dump(decision.attributes):
image

But looking more in details, this seems related to the way Data works (it always contains the whole data, and a pointer to the right place. So it may not be crap, just misunderstanding from my part.

I found a way to make the code work, with a small change to my code:

{% if decision.attributes|length == 1 %}
    {% set attribute = decision.attributes|first %}
    {% if attribute.type in [
        'Symfony\\Component\\ExpressionLanguage\\SerializedParsedExpression',
        'Symfony\\Component\\ExpressionLanguage\\ParsedExpression',
        'Symfony\\Component\\ExpressionLanguage\\Expression'
    ] %}
        {% set attribute = attribute["\x00*\x00expression"] %}
    {% endif %}
    {{ attribute }}
{% else %}
    {{ profiler_dump(decision.attributes) }}
{% endif %}

My mistake was to try to access expression, while the Data object had \x00*\x00expression as key due to being a protected property.

It then looks like this:
image

the drawback of this proposal is that it does not make it clear that it voted on an expression (depending on how it looks).

Here is an alternate proposal (with the advantage of providing a readable output even when voting on other kind of objects as attribute):

{% if decision.attributes|length == 1 %}
    {% set attribute = decision.attributes|first %}
    {% if attribute.type == 'string' %}
        {{ attribute }}
    {% else %}
         {{ profiler_dump(attribute) }}
    {% endif %}
    {{ attribute }}
{% else %}
    {{ profiler_dump(decision.attributes) }}
{% endif %}

image
capture d ecran 2018-02-21 a 13 55 00

This could then be improved with a custom caster for ParsedExpression hiding the nodes property.

A third alternative is to handle expressions in a special way at collecting time (casting them to string), but this feels weird.

I would go for the second alternative (handling any non-string attribute properly in the template instead of hardcoding a special case for expressions).
What do you think @symfony/deciders ?

Member

stof commented Feb 21, 2018

@nicolas-grekas here is the output of doing a dump(decision.attributes) in this place in the code of the template (the last line in the screen is the existing {{ decision.attributes|first }}):

Internal data dump

image

And here is the output when doing profiler_dump(decision.attributes):
image

But looking more in details, this seems related to the way Data works (it always contains the whole data, and a pointer to the right place. So it may not be crap, just misunderstanding from my part.

I found a way to make the code work, with a small change to my code:

{% if decision.attributes|length == 1 %}
    {% set attribute = decision.attributes|first %}
    {% if attribute.type in [
        'Symfony\\Component\\ExpressionLanguage\\SerializedParsedExpression',
        'Symfony\\Component\\ExpressionLanguage\\ParsedExpression',
        'Symfony\\Component\\ExpressionLanguage\\Expression'
    ] %}
        {% set attribute = attribute["\x00*\x00expression"] %}
    {% endif %}
    {{ attribute }}
{% else %}
    {{ profiler_dump(decision.attributes) }}
{% endif %}

My mistake was to try to access expression, while the Data object had \x00*\x00expression as key due to being a protected property.

It then looks like this:
image

the drawback of this proposal is that it does not make it clear that it voted on an expression (depending on how it looks).

Here is an alternate proposal (with the advantage of providing a readable output even when voting on other kind of objects as attribute):

{% if decision.attributes|length == 1 %}
    {% set attribute = decision.attributes|first %}
    {% if attribute.type == 'string' %}
        {{ attribute }}
    {% else %}
         {{ profiler_dump(attribute) }}
    {% endif %}
    {{ attribute }}
{% else %}
    {{ profiler_dump(decision.attributes) }}
{% endif %}

image
capture d ecran 2018-02-21 a 13 55 00

This could then be improved with a custom caster for ParsedExpression hiding the nodes property.

A third alternative is to handle expressions in a special way at collecting time (casting them to string), but this feels weird.

I would go for the second alternative (handling any non-string attribute properly in the template instead of hardcoding a special case for expressions).
What do you think @symfony/deciders ?

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Feb 21, 2018

Member

I added a way to dump back to "string" an expression in Symfony few years ago
cf #19722 and #19013
We could leverage this dumper to dump a string representation.

I also added in blackfire (private repository) a way to dump the expression with associated variables
example.

If @fabpot agree, we could "move" this code to the Symfony profiler.

Member

lyrixx commented Feb 21, 2018

I added a way to dump back to "string" an expression in Symfony few years ago
cf #19722 and #19013
We could leverage this dumper to dump a string representation.

I also added in blackfire (private repository) a way to dump the expression with associated variables
example.

If @fabpot agree, we could "move" this code to the Symfony profiler.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 21, 2018

Member

@lyrixx I think that makes sense. Go for it!

Member

fabpot commented Feb 21, 2018

@lyrixx I think that makes sense. Go for it!

@lyrixx

This comment has been minimized.

Show comment
Hide comment
@lyrixx

lyrixx Feb 22, 2018

Member

Actually, it's not result possible to achieve the same result as in blackfire. We don't have all required data.

But I made a "simple" patch to show the real expression. Now it's really easier to get what's going on

Member

lyrixx commented Feb 22, 2018

Actually, it's not result possible to achieve the same result as in blackfire. We don't have all required data.

But I made a "simple" patch to show the real expression. Now it's really easier to get what's going on

nicolas-grekas added a commit that referenced this issue Mar 19, 2018

bug #26273 [Security][Profiler] Display the original expression in 'A…
…ccess decision log' (lyrixx)

This PR was merged into the 3.4 branch.

Discussion
----------

[Security][Profiler] Display the original expression in 'Access decision log'

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #24127
| License       | MIT
| Doc PR        |

---

before:

![screenshot at 2018-02-22 18-22-28](https://user-images.githubusercontent.com/408368/36553752-798001ba-17fd-11e8-9539-254a25a01e60.png)

after:

![after](https://user-images.githubusercontent.com/408368/36553856-b7449fe2-17fd-11e8-94cb-ddaf4f033511.png)

Commits
-------

8f16c2e [Security][Profiler] Display the original expression in 'Access decision log'

@stof stof moved this from RFC to DONE in Profiler panels Mar 30, 2018

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