EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility #26

webhdx · 2017-10-24T13:35:41Z

JIRA: https://jira.ez.no/browse/EZP-28117

TODO

rebase after EZP-27888: Simplified eZPlatformAdminUI code and cleaned structure #27 is merged

Description

This PR removes menu definition from YAML as this approach has some major drawbacks:

no translation support
no way to remove system items
no way to reorder system items

With new approach, developers are able to use Event Listeners to do customization (i.e. add new menu items).

Example extensibility scenarios:

Let's assume below examples are an excerpt from the EventSubscriber:

<?php
namespace EzSystems\EzPlatformAdminUi\EventListener;

use EzSystems\EzPlatformAdminUi\Menu\Event\ConfigureMenuEvent;
use EzSystems\EzPlatformAdminUi\Menu\MainMenuBuilder;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

class MenuListener implements EventSubscriberInterface
{
    public static function getSubscribedEvents()
    {
        return [
            ConfigureMenuEvent::MAIN_MENU => ['onMenuConfigure', 0],
        ];
    }

    public function onMenuConfigure(ConfigureMenuEvent $event)
    {
        $menu = $event->getMenu();

        // ...
    }
}

Example 1: Add new menu item under "Content" with some custom attributes

$menu[MainMenuBuilder::ITEM_CONTENT]->addChild(
    'form_manager',
    [
        'route' => '_ezpublishLocation',
        'routeParameters' => ['locationId' => 2],
        [
            'linkAttributes' => [
                'class' => 'test_class another_class',
                'data-property' => 'value',
            ],
        ],
    ]
);

Example 2: Remove "Media" menu item from Content tab

$menu[MainMenuBuilder::ITEM_CONTENT]->removeChild(
    MainMenuBuilder::ITEM_CONTENT__MEDIA
);

Example 3: Add top level menu item with a child

$menu->addChild(
    'menu_item_1',
    ['label' => 'Menu Item 1', 'extras' => ['icon' => 'file']]
);
$menu['menu_item_1']->addChild(
    '2nd_level_menu_item',
    ['label' => '2nd level menu item', 'uri' => 'http://example.com']
);

Example 4: Add item depending on condition

$condition = true;
if ($condition) {
    $menu->addChild(
        'menu_item_2',
        ['label' => 'Menu Item 2', 'extras' => ['icon' => 'article']]
    );
}

Example 5: Top level menu item with URL redirection

$menu->addChild(
    'menu_item_3',
    [
        'label' => 'Menu Item 3',
        'uri' => 'http://example.com',
        'extras' => ['icon' => 'article'],
    ]
);

Example 6: Translatable labels

Use translation.key from messages domain:

$menu->addChild(
    'menu_item_3',
    [
        'label' => 'translation.key',
        'uri' => 'http://example.com',
        'extras' => ['icon' => 'article'],
        'translation_domain' => 'messages',
    ]
);

Example 7: Reordering menu items i.e. reversing the order

$menu->reorderChildren(
    array_reverse(array_keys($menu->getChildren()))
);

More resources

This approach is further described in Symfony's doc: https://symfony.com/doc/current/bundles/KnpMenuBundle/events.html

bdunogier · 2017-10-25T10:01:23Z

Looks nice. The yaml syntax was clearly too heavy for the target audience.

In order to see how this implementation addresses the requirements, could you write down a brief howto for the use-cases you have mentioned ?

no translation support
no way to remove system items
no way to reorder system items

I'm also still a bit unsure about the developer experience for this: while some developers will clearly have no problems with the event listener, do we want to help out a bit for the others ? We could for instance provide one or more EventSubscriber implementations. One could add a menu item, the other change an existing one, by means of abstract methods or service container arguments.

We must also consider the multi-site aspect of this, where multiple backends on the same instance may require different menu items.

bdunogier

Thank you for requesting the review, but I have actually posted a lengthy comment before you asked :)

I'd like to see the developer howto doc for this before approving or not. There are also other elements in the comment I'd like a reply to.

alongosz

Architecture-wise looks ok
One request from my side:

alongosz · 2017-10-26T12:51:31Z

src/bundle/EzPlatformAdminUiBundle.php

@@ -25,6 +25,9 @@ public function build(ContainerBuilder $container)
            new AdminFilter()
        );

+        $core->addConfigParser(new LocationIds());
+        $core->addDefaultSettings(__DIR__ . '/Resources/config', ['ezpublish_default_settings.yml']);


This is a config from this bundle, which is the new stack, so I'd call it ezplatform_default_settings.yml.
I know it was called like this in 1.x, however I think we should move forward :)

I agree. Fixed.

webhdx · 2017-10-27T10:23:44Z

Hey @bdunogier. I've prepared some code snippets for most common use cases. I hope this is enough for you to get a brief overview on developer experience. This should do as a base for the future documentation. I'm in touch with @DominikaK and we will be working on the documentation together.

@Nattfarinn @mikadamczyk @bdunogier If you want to do the code review, please review only
561d56a commit. Other commits come from #27. I wanted to base on these PR to avoid conflicts on merging later.

@bdunogier Answering your question on siteaccess dependend menus - it's doable as kernel exposes Siteaccess as a service which can be injected to the EventSubscriber.

alongosz · 2017-10-27T10:59:58Z

@webhdx please rebase (needed due to conflicts).

webhdx · 2017-10-27T15:52:34Z

@alongosz As description says it will be rebased after cleanup PR is merged.

alongosz · 2017-10-27T17:30:25Z

@webhdx if you have conflicts on files under review it's impossible to give reliable approval on something that is likely to be changed.

webhdx · 2017-10-28T09:07:09Z

@alongosz Conflicts are due to the commits from cleanup PR. I've written that in the comments above. Focus on the 561d56a commit.

alongosz

Conflicts are due to the commits from cleanup PR. I've written that in the comments above. Focus on the 561d56a commit.

Ok, this was very time consuming review due to PR not being clean, but I managed to do it.
+1 for 561d56a

I put some nitpicks and feedback request below:

alongosz · 2017-10-29T15:57:38Z

src/bundle/DependencyInjection/Configuration/Parser/LocationIds.php

+use eZ\Bundle\EzPublishCoreBundle\DependencyInjection\Configuration\SiteAccessAware\ContextualizerInterface;
+use Symfony\Component\Config\Definition\Builder\NodeBuilder;
+
+class LocationIds extends AbstractParser


Missing PhpDoc. I'd like to see here a very brief info where to put this config in an external package or meta repository. To be used when creating larger doc about extending v2 menu. // cc @DominikaK

alongosz · 2017-10-29T15:58:11Z

src/bundle/DependencyInjection/Configuration/Parser/LocationIds.php

+                ->children()
+                    ->scalarNode('content')->isRequired()->end()
+                    ->scalarNode('media')->isRequired()->end()
+                    ->scalarNode('users')->isRequired()->end()


Uh, this one just gave me a headache, but rather related to admin being a SiteAccess once again. Was about to request doc on how to override it, but finally I managed to figure this out.

The overridden config ezplatform.yml in meta-repository looks like this:

# System settings, grouped by siteaccess and/or siteaccess group system: admin: location_ids: content: 2 media: 43 users: 5

This raises a question, but rather to @Nattfarinn: does this configuration reflect only our Admin UI? In other words: isn't it necessary to make parent node like ezplatform_admin_ui for location_ids? However maybe not, since this is admin SA itself. Just asking for confirmation.

BTW. Have we already documented how new Admin SiteAccesses work @DominikaK?

As far as I remember we dynamically create corresponding admin siteaccess for every defined siteacces in the system. The best approach would be to define location_ids per groups created by siteaccess + its admin SA. We need to describe it in the documentation though.

cc @DominikaK

@alongosz not documented yet, but it is on my to-do list. I will need information on the use cases where a user may want to do anything to the config (and should they?)

There is a default configuration coming with the AdminUI which will work with our meta and demos. Configuration change is only needed when location IDs for Home, Media and Users differs from meta.

The most common use-case for this would be multi-site on one repository, where each site would have its own content, media and user root. Without going into details, allowing to change this probably requires more changes than that. For instance, it would impact the UDW, and we can probably think of other places.

While it is a good idea to make this configurable, an option would be to define the siteaccess aware settings, internally, and expose them once we have clarified the uses-cases and covered all the aspects of the feature.

alongosz · 2017-10-29T16:35:54Z

src/lib/Menu/MainMenuBuilder.php

+use Knp\Menu\ItemInterface;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
+
+class MainMenuBuilder extends AbstractBuilder implements TranslationContainerInterface


Missing PhpDoc, nothing fancy, something like:

/** * KnpMenuBundle Menu Builder service implementation. * @see https://symfony.com/doc/current/bundles/KnpMenuBundle/menu_builder_service.html */

alongosz · 2017-10-29T16:42:18Z

src/lib/Menu/Event/ConfigureMenuEvent.php

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


Missing very short PhpDoc about what this Event is used for

alongosz · 2017-10-29T16:43:09Z

src/lib/Menu/AbstractBuilder.php

+use Symfony\Component\EventDispatcher\Event;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
+
+abstract class AbstractBuilder


Nitpick: missing short PhpDoc about a responsibility of this class.

…positions Merge adjust richtext embed button positions

webhdx added the Improvement label Oct 24, 2017

webhdx self-assigned this Oct 24, 2017

webhdx added the Work in progress label Oct 24, 2017

webhdx mentioned this pull request Oct 25, 2017

EZP-28127: Create extendable left sidebar #28

Merged

webhdx requested review from lserwatka, mikadamczyk, Nattfarinn and alongosz October 25, 2017 09:29

webhdx changed the title ~~[WIP] EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility~~ EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility Oct 25, 2017

webhdx removed the Work in progress label Oct 25, 2017

webhdx requested a review from bdunogier October 25, 2017 11:33

webhdx mentioned this pull request Oct 25, 2017

EZP-28131: Create extendable right sidebar #30

Merged

lserwatka approved these changes Oct 26, 2017

View reviewed changes

bdunogier requested changes Oct 26, 2017

View reviewed changes

webhdx changed the title ~~EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility~~ [WIP] EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility Oct 26, 2017

webhdx added the Work in progress label Oct 26, 2017

alongosz requested changes Oct 26, 2017

View reviewed changes

This was referenced Oct 26, 2017

Revert "EZP-28127: Create extendable left sidebar" #31

Merged

EZP-28127: Create extendable left sidebar #32

Merged

webhdx force-pushed the menu_redesign branch from d1b0af4 to e26889b Compare October 27, 2017 10:20

webhdx force-pushed the menu_redesign branch from e26889b to 561d56a Compare October 27, 2017 10:28

ezsystems deleted a comment from ezrobot Oct 27, 2017

alongosz approved these changes Oct 29, 2017

View reviewed changes

webhdx force-pushed the menu_redesign branch from b36e6f4 to d9b55cc Compare October 30, 2017 10:30

webhdx force-pushed the menu_redesign branch from d9b55cc to 11b2199 Compare October 30, 2017 14:14

webhdx mentioned this pull request Oct 31, 2017

EZP-28055: As a v2 Editor I want to search for content items by entering a search keyword #33

Merged

EZP-28117: Refined top menu to allow event based extensibility

b371cff

webhdx force-pushed the menu_redesign branch from 11b2199 to b371cff Compare October 31, 2017 08:40

ezsystems deleted a comment from ezrobot Oct 31, 2017

webhdx changed the title ~~[WIP] EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility~~ EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility Oct 31, 2017

webhdx removed the Work in progress label Oct 31, 2017

lserwatka merged commit 9cb8ece into ezsystems:master Oct 31, 2017

webhdx deleted the menu_redesign branch November 14, 2017 08:49

mnocon pushed a commit that referenced this pull request Oct 20, 2021

Merge pull request #26 from ibexa/merge-adjust-richtext-embed-button-…

d8a115d

…positions Merge adjust richtext embed button positions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility #26

EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility #26

webhdx commented Oct 24, 2017 •

edited

Loading

bdunogier commented Oct 25, 2017

bdunogier left a comment

alongosz left a comment

alongosz Oct 26, 2017

webhdx Oct 27, 2017

webhdx commented Oct 27, 2017 •

edited

Loading

alongosz commented Oct 27, 2017

webhdx commented Oct 27, 2017

alongosz commented Oct 27, 2017

webhdx commented Oct 28, 2017 •

edited

Loading

alongosz left a comment

alongosz Oct 29, 2017

alongosz Oct 29, 2017

webhdx Oct 30, 2017

DominikaK Oct 30, 2017

webhdx Oct 31, 2017

bdunogier Nov 7, 2017

alongosz Oct 29, 2017

alongosz Oct 29, 2017

alongosz Oct 29, 2017

EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility #26

EZP-28117: Redesign menu system in AdminUI to event based approach for better extensibility #26

Conversation

webhdx commented Oct 24, 2017 • edited Loading

TODO

Description

Example extensibility scenarios:

Example 1: Add new menu item under "Content" with some custom attributes

Example 2: Remove "Media" menu item from Content tab

Example 3: Add top level menu item with a child

Example 4: Add item depending on condition

Example 5: Top level menu item with URL redirection

Example 6: Translatable labels

Example 7: Reordering menu items i.e. reversing the order

More resources

bdunogier commented Oct 25, 2017

bdunogier left a comment

Choose a reason for hiding this comment

alongosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webhdx commented Oct 27, 2017 • edited Loading

alongosz commented Oct 27, 2017

webhdx commented Oct 27, 2017

alongosz commented Oct 27, 2017

webhdx commented Oct 28, 2017 • edited Loading

alongosz left a 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

webhdx commented Oct 24, 2017 •

edited

Loading

webhdx commented Oct 27, 2017 •

edited

Loading

webhdx commented Oct 28, 2017 •

edited

Loading