Use the Knp menu for the back end header menu

[leofeyer]

This PR makes the necessary changes so the back end header menu is built via Knp menu and the debug button can be added in the manager bundle (this was an open TODO).

The PR also "fixes" the back end main menu, which does not adhere to the Knp menu standards and thus is only partially extendable.

TODOs

• Rework the main menu
• Fix the existing unit tests
• Add unit tests for the new classes

[aschempp]

I think we should keep the back end menu naming as-is, meaning the be_menu.twig and the respective event. And use a new template and a new event name for the be_header_menu, as devs listening to the BACKEND_MENU_BUILD event are for sure not checking the root node name atm.

[leofeyer]

I guess you are right, although it is ugly to have two different events for basically the same thing in the same context. But changed in d0b9a00 now.

[leofeyer]

I am still not convinced that using two events is the right thing to do. Also, I doubt that it would cause any problems, because extensions listening to the BACKEND_HEADER_BUILD event in Contao 4.8 (e.g. the manager bundle), will do something like this:

$categoryNode = $event->getTree()->getChild('system');

if (null === $categoryNode) {
    return;
}

And since there is no intersection of child nodes, this should never cause any problems:

Main menu

• content
• design
• accounts
• system

Header menu

• alerts
• preview
• submenu
• burger

@contao/developers What do you think? Can we use the same event for both menus?

[ausi]

If I can differentiate between header and main navigation easily (like $event->getTree()->getName() === 'headerMenu'?) and it doesn’t break existing code I’m in favor of one event.

[Toflar]

I'm with @ausi except for the fact that I would like to compare to a class constant rather than a regular string.

[leofeyer]

like $event->getTree()->getName() === 'headerMenu'?

Exactly like this. But in Yoda style. 😂

[aschempp]

The changes in the config.php files across all bundles are also not related to this PR at all. Maybe you can create a separate one (according to what we just discussed yesterday) to change that across all bundles. I'm not sure about the implications, but we should discuss that in the separate PR.

[leofeyer]

Of course they are related to this PR. The PR reworks the back end menus and the config.php changes affect the menu item order.

[Toflar]

Neat work here! Well done! I have my concerns regarding BC breaks and added them as comments so we can discuss. Otherwise this PR is in a good shape 👍

[Toflar]

As stated in inline comments, this currently introduces unnessary BC breaks which could be nicely solved using separate listeners for the menu items, just like it's done e.g. for the debug link in the manager-bundle.

[leofeyer]

I ask myself why it builds the previewUrl, the logoutUrl and then dispatches the event?

The menu builder does not build these URLs – the injected generators do.

BC breaks which could be nicely solved using separate listeners for the menu items, just like it's done e.g. for the debug link in the manager-bundle.

I disagree. The manager bundle uses an event listener, because that is the only way for other bundles to add menu links.

There is no point in using separate event listeners except for keeping the constructor arguments the way they are now. Other than that, it is just more complicated and more confusing for anyone who needs to work with it.

[aschempp]

I kinda agree with @Toflar. The builder is responsible for providing an event to build the menu, not to actually build the menu. That's what the event listeners are for. If the core menu items are not added by an event listener, I can't use priorities to be before/after that listener.

[aschempp]

Still standing by my statement (this is to shut up PR reminders 😂 )

[leofeyer]

I have extracted the main menu changes into a separate PR: #1101

[leofeyer]

The PR has been rebased and changed according to what we have discussed. Please review again. 😊

[Toflar]

So much cleaner now, with proper responsibilities! I like this 😄 👍
Should have more than one PR approval here before merge though.

[aschempp]

Generally looks good. I think there's one optimization possible:
Same as the debug button, the preview and logout button should be added in their own menu listener. These service listener should contain the code from PreviewUrlGenerator and LogoutUrlGenerator, because the url generators don't make much sense to me. They're never being reused, they are solely for the purpose of generating that button. It would also make DI for the regular menu builder much simpler, because it wouldn't contain multiple services only needed for one button each.

[aschempp]

The button should have a label or title, so we should probably not remove MSC.burgerTitle

[leofeyer]

I don't think so, because the button is only visible on mobile devices and neither title nor label would be shown.

[aschempp]

I would assume it is visible to screen readers, and most likely to an iPhone screen reader as well

[leofeyer]

It would also make DI for the regular menu builder much simpler, because it wouldn't contain multiple services only needed for one button each.

The services do not generate the buttons but only their URLs. This means whenever you want to generate the logout URL or the preview URL, you can use the service.

As soon as #989 is merged, we can reuse the preview URL service in the tl_page DCA to generate the URLs for the direct links (when you click the icon on the left).

[aschempp]

This means whenever you want to generate the logout URL or the preview URL, you can use the service.

As soon as #989 is merged, we can reuse the preview URL service in the tl_page DCA to generate the URLs for the direct links (when you click the icon on the left).

No you cannot. The service is explicitly bound to the CURRENT_ID and only works for exactly that button use case. That's what I mean, there's never a case for using that service standalone. You can't pass the current page ID or thelike.

[leofeyer]

I have implemented the requested changes. Please review again.

[aschempp]

Looks very good now to me! Just a few minor notes 👍