Skip to content
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

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

Merged
merged 1 commit into from
Oct 18, 2020

Conversation

VincentLanglet
Copy link
Member

Subject

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

Closes #6031

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()

By using menu.getLabel() instead of menu.label, we're avoiding a conflict
between the getLabel method and a possible child called with the key 'label'
@jordisala1991 jordisala1991 merged commit be6b0ed into sonata-project:3.x Oct 18, 2020
@jordisala1991
Copy link
Member

Thank you @VincentLanglet

@VincentLanglet VincentLanglet deleted the feature/getLabel branch October 18, 2020 20:39
@franmomu
Copy link
Member

IMHO it would be nice to add the functional tests added in the original PR https://github.com/sonata-project/SonataAdminBundle/pull/6031/files

@VincentLanglet
Copy link
Member Author

IMHO it would be nice to add the functional tests added in the original PR https://github.com/sonata-project/SonataAdminBundle/pull/6031/files

I added one, without creating a whole class/admin called PR6301.
The configureTabMenu I added was failing without this PR. And doesn't fail now. Isn't that enough ?

@franmomu
Copy link
Member

Well, the current test do not test twig calls, #6031 (review). In fact, when the test was added, it found another place where was wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants