Adding static methods for creating standard ToolbarActions

[mamazu]

Q	A
Bug fix?	no
New feature?	yes (at least for Devs)
BC breaks?	no
Deprecations?	no
Fixed tickets	-
Related issues/PRs	-
License	MIT
Documentation PR	TBA

What's in this PR?

In this pull requests I created static methods on the ToolbarAction class to easily create ToolbarAction objects.

Why?

There are a few reasons why. It's shorter and more concise to read. But the main reason is that you don't have to rely on magic strings anymore. So the user doesn't have to know that the delete button is the sulu_admin.delete button string.

And since the old feature isn't touched at all, it does not break any backwards compatibility.

Example Usage

// Before
$viewBuilder
    ->addToolbarActions([new ToolbarAction('sulu_admin.add'), new ToolbarAction('sulu_admin.delete')])
;

// After
$viewBuilder
    ->addToolbarActions([ToolbarAction::ADD(), ToolbarAction::DELETE()])
;

To Do

• Create a documentation PR
• Add breaking changes to UPGRADE.md

[alexander-schranz]

@mamazu Nice idea, like it that the toolbar action are also represented in PHP 👍.

I personally would even go the way of have an own class per ToolbarAction, like we already have for the DropdownToolbarAction. That make future refractoring or deprecating of toolbar each action easier. A class itself could define required options if possible. But lets wait here what the other @sulu/core-team say about it.

For views itself we did have an own class and then even did go with a Factory, so a ToolbarActionFactory could also be something. What currently is mostly the biggest problem I see is the Form vs. List Toolbaractions. Which could be a little bit strange as it is not very transparent currently. So I would maybe not go with a factory here at current state, but as said own class would I think not hurt and make things easier.

[niklasnatter]

I think the idea of using a Factory service for the views was that it is easy to discover what type of views are available and that it is possible to overwrite all views (eg. you can overwrite all list views by decorating the service and changing the implementation of the createListViewBuilder method)

I think I would implement it in the same way as we did for views to keep things consistent. For the moment, I think I would just implement two factories - ListToolbarActionFactory and FormToolbarActionFactory. This makes it easy to udnerstand what types of actions are available for lists and what are available for forms.

[wachterjohannes]

i would go with the same as we have for the views. own classes and factories. E.g.:

class SaveFormToolbarAction extends FormToolbarAction{...}
class SaveFormToolbarActionFactory extends FormToolbarActionFactory{...}

and

$viewBuilder
    ->addToolbarActions([new SaveFormToolbarActionFactory()->setWhatever('Sulu is awesome')])
;

The resolving of the classes itself from the factory should then be done later when the view-factory will be resolved.

[alexander-schranz]

The (new SaveFormToolbarActionFactory()) would be inconsistent to the exist Factory (ViewBuilderFactory) which are services and provide multiple factory methods. I would not introduce a new type of factory here if we want to go with new then we should not go with a factory.

[mamazu]

I mean creating seperate classes for all the actions definitely sounds like a good idea.

But for the factory structure I am not really sure if this is the best way. One thing that I like to keep in mind (and also one of the reasons I have created this pull request) was to simplify the view creation process for "simple" pages that only have a list view and an edit form.

So maybe something like

class ToolbarFactory {
    public function createForForm(array $buttonNames): array {
        $buttons = [];
        foreach($buttonNames as $buttonName) {
            $button = new ToolbarAction($buttonName);
            if ($toolbarAction->isSuitableForForm()) {
                $buttons[] = $button;
            }
        }
        return $buttons;
    }

    public function createForList(array $buttonNames): array {
        $buttons = [];
        foreach($buttonNames as $buttonName) {
            $button = new ToolbarAction($buttonName);
            if ($toolbarAction->isSuitableForList()) {
                $buttons[] = $button;
            }
        }
        return $buttons;
    }
}

and usage would be like:

$this->toolbarFactory->generateForList([ToolbarAction::ADD, ToolbarAction::DELETE, ToolbarAction::EXPORT]);
$this->toolbarFactory->generateForForm([ToolbarAction::SAVE, ToolbarAction::DELETE]);

And for more complex cases the user can still add their own buttons to the list.

[mamazu]

I think I have an idea that might work. So taking the idea from alexander to make the buttons into classes has two advantages:

• You can have a better DX because you can easily document their options there
• You could have interfaces that define if the button is a list or a form button.

An example:

interface FormToolbarAction {}
interface ListToolbarAction {}

class SaveFormToolbarAction implements FormToolbarAction
{
    public function __construct(array $options) {
    }

    // ... code goes here
}


class ExportListToolbarAction implements ListToolbarAction
{
    public function __construct(array $options) {
    }

    // code goes here
}

Then you can have a ToolbarBuilderFactory (similar to the ViewBuilderFactory) that creates either a ListToolbarActionBuilder or a FormToolbarActionBuilder and usage would be like so.

$listToolbarActions = $this->toolbarBuilder->createListToolbar()
    ->add(new ExportListToolbarAction())
//  ->add(...)
    ->toArray()
;

$listToolbarActions = $this->toolbarBuilder->createFormToolbar()
    ->add(new SaveFormToolbarAction())
//  ->add(...)
    ->toArray()
;

If the buttons then also encode what permissions they need to be displayed then this should also drastically reduce the complexity of the generation of the toolbar.

Because in the current system it is easy to

• forget checking for permissions
• Check for the wrong permissions (eg. you accidentally typed delete instead of Edit for the save button)