Fix \Knp\Menu\MenuItem @label issue #6031

maxplatonov · 2020-04-06T22:34:23Z

Subject

I am targeting this branch, because it is a bug fix.

Changelog

### Fixed
- Call \Knp\Menu\MenuItem::getLabel() method directly in twig template to avoid a possible side effect from \ArrayAccess.

Description:

\Knp\Menu\MenuItem in knplabs/knp-menu-bundle implements \ArrayAccess and defines its offsetGet method as follows:

    /**
     * Implements ArrayAccess
     */
    public function offsetGet($name)
    {
        return $this->getChild($name);
    }

    public function getChild($name)
    {
        return isset($this->children[$name]) ? $this->children[$name] : null;
    }

It has a property $label with its getter:

    /**
     * Label to output, name is used by default
     *
     * @var string|null
     */
    protected $label;

    public function getLabel()
    {
        return (null !== $this->label) ? $this->label : $this->name;
    }

Problem :

If a MenuItem object has a child with the key label, the twig expression menu.label will trigger the offsetGet method of the \ArrayAccess and return the child instead of the $label property of the MenuItem object.

This would result in Catchable Fatal Error: Object of class Knp\Menu\MenuItem could not be converted to string.

Solution:

Call the getter of the $label property directly:

menu.getLabel()

phansys · 2020-04-07T01:41:40Z

Thank you @maxplatonov!
Could you please add a test in order to prevent regressions?
You should also update the PR's description in order to use the provided template.

VincentLanglet · 2020-04-08T20:10:41Z

Do you think this behaviour should be reported to twig repository ?

IMHO I expect menu.label to look for a getLabel method before using offsetGet.

maxplatonov · 2020-04-08T21:57:57Z

Do you think this behaviour should be reported to twig repository ?

IMHO I expect menu.label to look for a getLabel method before using offsetGet.

Hello @VincentLanglet,

I suppose it would be wrong to expect that. As per Twig (2, 3) documentation:

For convenience’s sake foo.bar does the following things on the PHP layer:

check if foo is an array and bar a valid element;

⬇️ This is where \ArrayAccess comes into play.

if not, and if foo is an object, check that bar is a valid property;

⬆️

https://github.com/twigphp/Twig/blob/2.x/src/Extension/CoreExtension.php#L1340-L1346

if not, and if foo is an object, check that bar is a valid method (even if bar is the constructor - use __construct() instead);
if not, and if foo is an object, check that getBar is a valid method;
if not, and if foo is an object, check that isBar is a valid method;
if not, and if foo is an object, check that hasBar is a valid method;
if not, return a null value.

IMHO Twig's behaviour may change in the future, I would rely on the getter.

VincentLanglet

How do you want to add a test @phansys ?

phansys · 2020-04-10T23:33:19Z

I think a functional test should be enough to cover the exposed issue.

src/Resources/views/standard_layout.html.twig

phansys

Thank you @maxplatonov.

I've left a comment about the test, since I'd like to focus the effort on ensuring the issue is solved with our real codebase.

tests/Menu/Integration/TabMenuTest.php

franmomu · 2020-04-12T16:24:55Z

Let me try to add a functional test @maxplatonov to see how hard it is, I'll make you a PR if I'll achieve something.

maxplatonov · 2020-04-12T17:17:10Z

Let me try to add a functional test @maxplatonov to see how hard it is, I'll make you a PR if I'll achieve something.

Thanks, @franmomu !

franmomu · 2020-04-12T19:13:26Z

I've just did the PR adding the test, it actually shows that there is another place to change it in standard_layout.html.twig and in sonata_menu.html.twig.

And I guess it means that it will fail in the KnpMenu library.

maxplatonov · 2020-04-20T07:56:19Z

Hello @franmomu,
Thanks for your PR! I was disconnected for a week, I'll review it today.

SonataCI · 2020-05-02T14:44:02Z

Could you please rebase your PR and fix merge conflicts?

@Label

## Description: \Knp\Menu\MenuItem in knplabs/knp-menu-bundle implements \ArrayAccess and defines its offsetGet method as follows: ```php /** * Implements ArrayAccess */ public function offsetGet($name) { return $this->getChild($name); } public function getChild($name) { return isset($this->children[$name]) ? $this->children[$name] : null; } ``` It has a property @Label with its getter: ```php /** * Label to output, name is used by default * * @var string|null */ protected $label; public function getLabel() { return (null !== $this->label) ? $this->label : $this->name; } ``` ## Problem : If a MenuItem object has a child with the key `label`, the twig expression `menu.label` will trigger the `offsetGet` method of the \ArrayAccess and return the child instead of the `label` property of the MenuItem object. ## Solution: Call the getter of the `label` property directly: ```twig menu.getLabel() ```

This reverts commit ea6fdea.

maxplatonov · 2020-05-05T13:18:51Z

Hello, @franmomu,

Could you please review? I rebased the branch on the upstream. I had to restore the \Sonata\AdminBundle\Tests\App\Model\ModelManager::getModelInstance as it broke a new test.

Thank you.

franmomu

Nice! You could do a PR to fix it in KnpMenu as well if you feel like doing it!

src/Resources/views/Core/tab_menu_template.html.twig

src/Resources/views/standard_layout.html.twig

src/Resources/views/Menu/sonata_menu.html.twig

tests/App/Model/ModelManager.php

tests/App/Model/PR6031Repository.php

Co-authored-by: Javier Spagnoletti <[email protected]>

Co-authored-by: Javier Spagnoletti <[email protected]> Co-authored-by: Fran Moreno <[email protected]>

src/Resources/views/Core/tab_menu_template.html.twig

src/Resources/views/Menu/sonata_menu.html.twig

src/Resources/views/standard_layout.html.twig

VincentLanglet · 2020-05-17T19:31:32Z

@maxplatonov Please rebase 3.x in order to have working tests.

phansys · 2020-05-17T21:27:18Z

tests/App/Model/ModelManager.php

@@ -21,11 +21,17 @@

 final class ModelManager implements ModelManagerInterface
 {
-    private $repository;
+    /**
+     * @var array array<class-string, RepositoryInterface>


Is this annotation right? (array array)

phansys · 2020-05-17T21:32:38Z

src/Resources/views/Core/tab_menu_template.html.twig

@@ -119,8 +119,9 @@
 {% endblock %}

 {% block label %}
-{{-
-    item.label|trans(
+    {# We use method accessor instead of ".label" since `item` implements `ArrayAccess` and could have a property called "label".#}


Suggested change

{# We use method accessor instead of ".label" since `item` implements `ArrayAccess` and could have a property called "label".#}

{# We use method accessor instead of ".label" since `item` implements `ArrayAccess` and could have a property called "label". #}

phansys · 2020-05-17T21:32:51Z

src/Resources/views/Menu/sonata_menu.html.twig

@@ -47,4 +47,5 @@
    {% endapply %}
 {% endblock %}

-{% block label %}{% if is_link is defined and is_link %}{{ icon|default|raw }}{% endif %}{% if options.allow_safe_labels and item.extra('safe_label', false) %}{{ item.label|raw }}{% else %}{{ item.label|trans({}, translation_domain|default('messages')) }}{% endif %}{% endblock %}
+{# We use method accessor instead of ".label" since `item` implements `ArrayAccess` and could have a property called "label".#}


Suggested change

{# We use method accessor instead of ".label" since `item` implements `ArrayAccess` and could have a property called "label".#}

{# We use method accessor instead of ".label" since `item` implements `ArrayAccess` and could have a property called "label". #}

phansys · 2020-05-17T21:33:06Z

src/Resources/views/standard_layout.html.twig

@@ -96,7 +96,8 @@ file that was distributed with this source code.
                            {% endif %}

                            {%- set translation_domain = menu.extra('translation_domain', 'messages') -%}
-                            {%- set label = menu.label -%}
+                            {# We use method accessor instead of ".label" since `menu` implements `ArrayAccess` and could have a property called "label".#}


Suggested change

{# We use method accessor instead of ".label" since `menu` implements `ArrayAccess` and could have a property called "label".#}

{# We use method accessor instead of ".label" since `menu` implements `ArrayAccess` and could have a property called "label". #}

phansys · 2020-05-17T21:37:29Z

tests/Functional/Ticket/PR6031Test.php

+
+final class PR6031Test extends WebTestCase
+{
+    public function testLabelInShowAction(): void


Since this test is not performing any direct assertion against the label, we should add a description explaining how it's related.

VincentLanglet · 2020-06-09T21:26:02Z

@maxplatonov Hi ! Do you have some time to finish this PR ? :)

SonataCI · 2020-07-12T16:12:01Z

Could you please rebase your PR and fix merge conflicts?

phansys added patch Needs: Tests labels Apr 7, 2020

phansys added the pending author label Apr 8, 2020

SonataCI removed the pending author label Apr 8, 2020

VincentLanglet previously approved these changes Apr 10, 2020

View reviewed changes

phansys reviewed Apr 10, 2020

View reviewed changes

src/Resources/views/standard_layout.html.twig Show resolved Hide resolved

maxplatonov dismissed VincentLanglet’s stale review via ea6fdea April 12, 2020 13:18

phansys requested changes Apr 12, 2020

View reviewed changes

tests/Menu/Integration/TabMenuTest.php Outdated Show resolved Hide resolved

maxplatonov and others added 7 commits May 5, 2020 11:44

add a test to prove the bug

e629522

Create WebTestCase

a853b27

Make Functional tests extendable

23e3c77

Add Functional test for PR6031

c1f701b

Fix MenuItem getLabel

551c04e

Revert "add a test to prove the bug"

02c14c5

This reverts commit ea6fdea.

maxplatonov force-pushed the patch-1 branch from f7319c7 to 02c14c5 Compare May 5, 2020 11:19

maxplatonov added 2 commits May 5, 2020 14:58

restore default model instantiation

bd11a80

Apply cs-fixer

3ee68ce

franmomu previously approved these changes May 5, 2020

View reviewed changes

phansys requested changes May 5, 2020

View reviewed changes

franmomu reviewed May 6, 2020

View reviewed changes

tests/App/Model/PR6031Repository.php Outdated Show resolved Hide resolved

Update src/Resources/views/Core/tab_menu_template.html.twig

b4a3e30

Co-authored-by: Javier Spagnoletti <[email protected]>

maxplatonov dismissed franmomu’s stale review via b4a3e30 May 6, 2020 10:55

maxplatonov and others added 2 commits May 6, 2020 12:55

Update src/Resources/views/Menu/sonata_menu.html.twig

8166ccc

Co-authored-by: Javier Spagnoletti <[email protected]>

Apply suggestions from code review

d3a1223

Co-authored-by: Javier Spagnoletti <[email protected]> Co-authored-by: Fran Moreno <[email protected]>

phansys requested changes May 6, 2020

View reviewed changes

maxplatonov added 2 commits May 7, 2020 13:12

Fix twig comments

98c419f

Apply Twig comments syntax

4021fb1

phansys requested changes May 17, 2020

View reviewed changes

phansys added the pending author label Jun 10, 2020

VincentLanglet mentioned this pull request Oct 18, 2020

Fix \Knp\Menu\MenuItem @Label issue #6498

Merged

jordisala1991 closed this in #6498 Oct 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix \Knp\Menu\MenuItem @label issue #6031

Fix \Knp\Menu\MenuItem @label issue #6031

maxplatonov commented Apr 6, 2020 •

edited

Loading

phansys commented Apr 7, 2020

VincentLanglet commented Apr 8, 2020

maxplatonov commented Apr 8, 2020

VincentLanglet left a comment

phansys commented Apr 10, 2020

phansys left a comment

franmomu commented Apr 12, 2020 •

edited

Loading

maxplatonov commented Apr 12, 2020

franmomu commented Apr 12, 2020

maxplatonov commented Apr 20, 2020

SonataCI commented May 2, 2020

maxplatonov commented May 5, 2020

franmomu left a comment

VincentLanglet commented May 17, 2020

phansys May 17, 2020

phansys May 17, 2020

phansys May 17, 2020

phansys May 17, 2020

phansys May 17, 2020

VincentLanglet commented Jun 9, 2020

SonataCI commented Jul 12, 2020

	{# We use method accessor instead of ".label" since `item` implements `ArrayAccess` and could have a property called "label".#}
	{# We use method accessor instead of ".label" since `item` implements `ArrayAccess` and could have a property called "label". #}

Fix \Knp\Menu\MenuItem @label issue #6031

Fix \Knp\Menu\MenuItem @label issue #6031

Conversation

maxplatonov commented Apr 6, 2020 • edited Loading

Subject

Changelog

Description:

Problem :

Solution:

phansys commented Apr 7, 2020

VincentLanglet commented Apr 8, 2020

maxplatonov commented Apr 8, 2020

VincentLanglet left a comment

Choose a reason for hiding this comment

phansys commented Apr 10, 2020

phansys left a comment

Choose a reason for hiding this comment

franmomu commented Apr 12, 2020 • edited Loading

maxplatonov commented Apr 12, 2020

franmomu commented Apr 12, 2020

maxplatonov commented Apr 20, 2020

SonataCI commented May 2, 2020

maxplatonov commented May 5, 2020

franmomu left a comment

Choose a reason for hiding this comment

VincentLanglet commented May 17, 2020

phansys May 17, 2020

Choose a reason for hiding this comment

phansys May 17, 2020

Choose a reason for hiding this comment

phansys May 17, 2020

Choose a reason for hiding this comment

phansys May 17, 2020

Choose a reason for hiding this comment

phansys May 17, 2020

Choose a reason for hiding this comment

VincentLanglet commented Jun 9, 2020

SonataCI commented Jul 12, 2020

maxplatonov commented Apr 6, 2020 •

edited

Loading

franmomu commented Apr 12, 2020 •

edited

Loading