[RFC] Use KnpMenu for the main backend menu #1066

sheeep · 2017-09-11T14:58:49Z

This PR refactors the way the backend menu is created.

Check hasAccess on backend modules
Remove / Deprecate old methods in Contao\BackendUser
Implement getUserNavigation hook
A lot of CS stuff

aschempp · 2017-09-12T13:53:49Z

src/Event/BackendMenuEvent.php

+use Knp\Menu\ItemInterface;
+use Symfony\Component\EventDispatcher\Event;
+
+class BackendMenuEvent extends Event


can or should we have a universal menu event? Or does such an event class maybe already exist in KnpMenu?

Or does such an event class maybe already exist in KnpMenu?

I don't find any such events in either KnpMenu and KnpMenuBundle.

can or should we have a universal menu event?

We probably could. I can't think of any reasons or possibilities for having more than the tree in this event.

aschempp · 2017-09-12T13:54:21Z

src/EventListener/BackendMenu/UserMenuListener.php

@@ -0,0 +1,65 @@
+<?php
+
+namespace Contao\CoreBundle\EventListener\BackendMenu;


Are we really using subnamespaces for event listeners?

aschempp · 2017-09-12T13:54:52Z

src/EventListener/BackendMenu/UserMenuListener.php

+
+    public function onBuild(BackendMenuEvent $event)
+    {
+        $tree = $event->getTree();


This could be moved after the if-null condition

Correct, we don't need to return the tree.

aschempp · 2017-09-12T15:08:33Z

src/EventListener/BackendMenu/UserMenuListener.php

+                $tree->addChild($node);
+            }
+
+            // Create the child nodes


I would prefer to refactor this to a method that generates the child.

aschempp · 2017-09-12T15:09:54Z

src/Resources/config/listener.yml

@@ -40,6 +40,14 @@ services:
        tags:
            - { name: kernel.event_listener, event: kernel.terminate, method: onKernelTerminate }

+    contao.listener.backend_menu.legacy_menu_listener:


same here with the namespace

aschempp · 2017-09-13T10:06:35Z

src/Event/MenuEvent.php

+{
+    private $tree;
+
+    public function __construct(ItemInterface $tree)


The event should also contain the menu factory, so it does not need to be injected into the listener.

aschempp · 2017-09-13T10:09:23Z

src/Menu/BackendMenuRenderer.php

+    public function render(ItemInterface $item, array $options = [])
+    {
+        $lang = [
+            'skipNavigation' => \StringUtil::specialchars($GLOBALS['TL_LANG']['MSC']['skipNavigation'])


This can later be solved with #1072

aschempp · 2017-09-13T10:11:33Z

src/Resources/config/services.yml

+        factory: ["@contao.menu.backend_menu_builder", create]
+        arguments: ["@request_stack"]
+        tags:
+            - { name: knp_menu.menu, alias: backend }


Not sure if we need this tag, but if then the alias is not useful.

aschempp · 2017-09-13T10:11:51Z

src/Resources/config/services.yml

+            - "@knp_menu.factory"
+            - "@event_dispatcher"
+
+    contao.menu.backend_menu:


According to discussion with @sheeep we don't need the menu as a service because of a custom renderer.

aschempp

but we should wait for #1072

… hook.

leofeyer · 2017-10-25T09:44:57Z

I'm done fixing the coding style and switching to the translator. There are, however, there is an issue that needs to be addressed in the be_menu.html.twig template:

OLD

<li class="tl_level_1_group […] AjaxRequest.toggleNavigation( […] '<?= $arrModules['ajaxUrl']; ?> […]

NEW

<li class="tl_level_1_group […] AjaxRequest.toggleNavigation( […] '{{ path('contao_backend') }} […]

Do we no longer need the ajaxUrl parameter?

leofeyer · 2017-10-26T07:20:03Z

Never mind, I just realized that the contao_backend route is the Ajax URL.

leofeyer · 2017-10-26T07:32:03Z

Thanks a lot @sheeep.

sheeep changed the title ~~[WIP ] Use KnpMenu for the main backend menu~~ [WIP] Use KnpMenu for the main backend menu Sep 11, 2017

leofeyer assigned sheeep Sep 12, 2017

leofeyer added the feature label Sep 12, 2017

leofeyer added this to the 4.5.0 milestone Sep 12, 2017

leofeyer changed the title ~~[WIP] Use KnpMenu for the main backend menu~~ [RFC] Use KnpMenu for the main backend menu Sep 12, 2017

aschempp suggested changes Sep 12, 2017

View reviewed changes

aschempp suggested changes Sep 13, 2017

View reviewed changes

aschempp approved these changes Sep 13, 2017

View reviewed changes

leofeyer force-pushed the develop branch 5 times, most recently from ed0a7e5 to 3dadd44 Compare September 28, 2017 10:39

leofeyer force-pushed the develop branch from e37e989 to dad087d Compare October 3, 2017 19:03

leofeyer force-pushed the develop branch from f131963 to e665dae Compare October 17, 2017 07:50

sheeep added 14 commits October 25, 2017 10:30

Refactored the existing backend menu to use the KnpMenu library.

c903bb0

Use BackendUser::navigation in order to support hasAccess and the old…

7429bb4

… hook.

Coding style and namespaces.

a318012

Moved services to its own namespace.

15c991f

Refactored the active state.

4d550ec

Removed old and unused icon attribute from nodes.

52e2460

Fixed node-expanded for category nodes.

494bb7f

Removed ANNOTATE event.

c04a300

Added the possibility to add your own data to the menu template.

be211a2

Tested BackendMenu classes and removed the legacy prefix.

9565bf6

Tested UserMenuLister for the case no user is present.

fb60b31

Implemented feedback.

6c7974e

Removed nested namespaces again.

d6ad6da

Feedback round 2.

fddab6f

leofeyer force-pushed the feature/knp-backend-menu branch from ea20469 to fddab6f Compare October 25, 2017 08:36

Fix the coding style.

4c3e44a

leofeyer force-pushed the feature/knp-backend-menu branch from f8d0538 to 4c3e44a Compare October 25, 2017 09:27

leofeyer force-pushed the feature/knp-backend-menu branch from 96ab909 to 486fea4 Compare October 25, 2017 10:07

Use the translator to add the MSC.skipNativation label.

8cdb072

leofeyer force-pushed the feature/knp-backend-menu branch from 486fea4 to 8cdb072 Compare October 25, 2017 10:09

Add a change log entry.

11d665e

leofeyer merged commit df66ffd into contao:develop Oct 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Use KnpMenu for the main backend menu #1066

[RFC] Use KnpMenu for the main backend menu #1066

sheeep commented Sep 11, 2017 •

edited by leofeyer

Loading

aschempp Sep 12, 2017

sheeep Sep 12, 2017

aschempp Sep 12, 2017

aschempp Sep 12, 2017

sheeep Sep 12, 2017

aschempp Sep 12, 2017

aschempp Sep 12, 2017

aschempp Sep 13, 2017

aschempp Sep 13, 2017

aschempp Sep 13, 2017

aschempp Sep 13, 2017

aschempp left a comment

leofeyer commented Oct 25, 2017 •

edited

Loading

leofeyer commented Oct 26, 2017

leofeyer commented Oct 26, 2017

		@@ -0,0 +1,65 @@
		<?php

		namespace Contao\CoreBundle\EventListener\BackendMenu;

[RFC] Use KnpMenu for the main backend menu #1066

[RFC] Use KnpMenu for the main backend menu #1066

Conversation

sheeep commented Sep 11, 2017 • edited by leofeyer Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschempp left a comment

Choose a reason for hiding this comment

leofeyer commented Oct 25, 2017 • edited Loading

leofeyer commented Oct 26, 2017

leofeyer commented Oct 26, 2017

sheeep commented Sep 11, 2017 •

edited by leofeyer

Loading

leofeyer commented Oct 25, 2017 •

edited

Loading