Extracting admin classes from bundles

[fbourigault]

The goal of this issue is to track the work-in-progress in extracting the admin classes from each bundle to provide a separated sonata admin integration of the cmf bundles.

Work has started in symfony-cmf/content-bundle#150 as a proof of concept.

Extracting an Admin class

• Extract FormTypes.
• Move Admin classes inside this bundle.
• Update the sandbox.

  Extracting an AdminExtension class

• Extract FormExtensions
• Move AdminExtension class inside this bundle.

  Progress

Bundle	Class	Status	Link
content-bundle	Admin\StaticContentAdmin	Merged	symfony-cmf/content-bundle#150
routing-bundle	Admin\RouteAdmin	Todo
routing-bundle	Admin\RedirectRouteAdmin	Todo
block-bundle	Admin\Imagine\ImagineBlockAdmin	Todo
block-bundle	Admin\Imagine\SlideshowBlockAdmin	Todo
block-bundle	Admin\ReferenceBlockAdmin	Todo
block-bundle	Admin\StringBlockAdmin	Todo
block-bundle	Admin\MenuBlockAdmin	Todo
block-bundle	Admin\ActionBlockAdmin	Todo
block-bundle	Admin\ContainerBlockAdmin	Todo
block-bundle	Admin\SimpleBlockAdmin	Todo
menu-bundle	Admin\MenuNodeAdmin	Todo
menu-bundle	Admin\MenuAdmin	Todo
seo-bundle	Admin\Extension\SeoContentAdminExtension	Todo
core-bundle	Admin\Extension\ChildExtension	Todo
core-bundle	Admin\Extension\PublishableExtension	In progress	symfony-cmf/core-bundle#207
core-bundle	Admin\Extension\PublishTimePeriodExtension	In progress	symfony-cmf/core-bundle#207
routing-bundle	Admin\Extension\RouteReferrersExtension	Todo
routing-bundle	Admin\Extension\FrontendLinkExtension	Todo
block-bundle	Admin\Extension\BlockCacheExtension	Todo
menu-bundle	Admin\Extension\MenuNodeReferrersExtension	Todo
menu-bundle	Admin\Extension\MenuOptionsExtension	Todo

Regressions

• help messages are lost (https://github.com/symfony-cmf/core-bundle/pull/207#discussion_r81094866)

[fbourigault]

@dbu @wouterj for MenuOptionsExtension have you an idea about how to extends every type that implements MenuOptionsInterface? I looked how the Form component works and looks like that FormPass register FormTypeExtension into DependencyInjectionExtension grouping them by the type they extends.

An idea is about to provide a generic FormTypeExtension that have a constructor that take the type to extends and have FormTypeExtension::getExtendedType to return it. Then we could use some configuration that is prepended by dependency injection in other bundles and register the FormTypeExtension as many times as required. To avoid repeating code between bundles providing FormTypeExtensions, we could write something in the CoreBundle that provide a generic way to declare extensions that extends FormTypes based on some interface with something like:

cmf_core:
    form_extensions:
        - service: some.abstract.service.template #This abstract service class should implement an interface that enforce having a setExtendedType method
          types:
              - FormTypeAFullyQualifiedName #Configured using dependency-injection
              - FormTypeBFullyQualifiedName #Configured using dependency-injection

What do you think?

[fbourigault]

I started to write the Form\Extension\PublishableExtension. I'm targeting to move the definition of the publishable field into the extension and remove the Admin\PublishableExtension.

Does we move the responsibility to add the field to the AbstractAdmin::configureFormFields of every admin class? By doing this, we have less automatic admin configuration but a better flexibility on fields order, groups and tabs.

Does the formGroup title should be dropped and only handled by translation?

[dbu]

if we can get rid of sonata admin extensions, i am happy with that. can we keep representing this with tabs? i thought symfony forms themselves would not support tabs to split parts of the forms, but if we can do that it would be much better than doing them in sonata.

[fbourigault]

If the responsibility of fields order, groups and tabs is moved to the Admin class whether or not the field is from the main Type or a FormExtension, we can do anything. The only drawback is that, in StaticContentAdmin::configureFormFields() we will have to write

$formMapper
    ->with($this->formGroup, array('translation_domain' => 'CmfCoreBundle'))
        ->add('publishable', $builder->get('publishable'))
    ->end();

instead of relying on the Admin\PublishableExtension and write this in every Admin which have their managed class using the publishable field.

As we get the $builder using

$builder = $formMapper->getFormBuilder()->getFormFactory()->createBuilder(StaticContentType::class, null, [
    'readonly_parent_document' => (bool) $this->id($this->getSubject()),
]);

we can't have an extension as we will be unable to know how to get the builder unless we write an interface for admins that enforce the admin to have a getFormBuilder method. In such case we will be able to have an extension that commonize code and keep the formGroup title feature. This later idea could be a little hacky but can be the solution.

[dbu]

can we keep that logic in a form extension then? the nice thing about the form extensions is that you can configure where to activate them based on the interfaces the model implements. see for example https://github.com/symfony-cmf/cmf-sandbox/blob/master/app/config/config.yml#L150-L155

even if its not that much code, it avoids quite a bit of duplication and makes things convenient to work out of the box. otherwise we would re-start having a base admin that does this, which is problematic because its behaviour, not type-of relation.

do you see the idea? do you have any idea for a better solution?

[fbourigault]

can we keep that logic in a form extension then? the nice thing about the form extensions is that you can configure where to activate them based on the interfaces the model implements. see for example https://github.com/symfony-cmf/cmf-sandbox/blob/master/app/config/config.yml#L150-L155

You mean admin extension here?

I see the idea but the problem is with the new way to configure fields in https://github.com/symfony-cmf/content-bundle/pull/150 we can't know which type is used by the admin class and so we can't get the builder. I don't have a solution (yet) without having to create an abstract admin class that allow any extension to get the form builder we use (and which options are used). We can avoid the creation of an abstract admin by juste implementing an interface (FormBuilderAwareAdminInterface) that enforce the admin class to have a getFormBuilder method and so having all admin extensions checking that admin implements the FormBuilderAwareAdminInterface and then use `$builder->get('publishable') from the admin that is still responsible of the group name (and why not later tabs).

EDIT: as code is sometimes better than explanations, here is a gist with a working implementation. It show as diff but if it's not readable, I will push my branches on request.

[fbourigault]

@dbu @wouterj what do you think about my proposal?

[dbu]

clever thinking. but i am afraid this looks a lot more complicated than previously. i am particularly afraid of the prepend configuration (it creates some annoying coupling between bundles).

can't we do it like this: provide form types (not extensions) for those interfaces, but let the admin extension react on the class managed by an admin as its now. the admin extension would then just add another form type into the admin. is that doable?

when you build your form with something else than sonata, you need to manually control if you also want the handling of routes or menu or whatever. this would be fine by me, beeing explicit is good here.

[fbourigault]

clever thinking. but i am afraid this looks a lot more complicated than previously. i am particularly afraid of the prepend configuration (it creates some annoying coupling between bundles).

I agree that it's more complicated than existing solution (and I didn't like myself adding things in a "Core" named bundle!)

can't we do it like this: provide form types (not extensions) for those interfaces, but let the admin extension react on the class managed by an admin as its now. the admin extension would then just add another form type into the admin. is that doable?

As in StaticContentTypeAdmin I "steal" children from StaticContentType we definitely could do the same for the AdminExtension. Using the StaticContentType directly will, IMHO, imply work on the sonata side.

when you build your form with something else than sonata, you need to manually control if you also want the handling of routes or menu or whatever. this would be fine by me, beeing explicit is good here.

We could, for others provide a FormTypeExtension that add the RoutingType to the StaticContentType with inherit_data set to true as described in How to Reduce Code Duplication with "inherit_data". We could also provide an abstract service in the RoutingBundle and use it as parent service in the StaticContentBundle to avoid requirement for Configuration and DI work in the CoreBundle.

[dbu]

yes, i think providing form type extensions sounds right. and imho we don't need "autowiring" for those. if you build a form, just use the extensions explicitly.

with sonata, could we use sonata extensions that use the formtype extensions? eventually we want to move all sonata admins of content routing and core into one separate common bundle. so dependencies between those would be easier to manage (though it seems that part should work without explicit dependencies, thanks to the sonata extensions mechanism)

[fbourigault]

just use the extensions explicitly

Does this mean creating it's own service extending the abstract one provider by the bundle owning the extension?

with sonata, could we use sonata extensions that use the formtype extensions? eventually we want to move all sonata admins of content routing and core into one separate common bundle. so dependencies between those would be easier to manage (though it seems that part should work without explicit dependencies, thanks to the sonata extensions mechanism)

I want to write something like this:

public function configureFormFields(FormMapper $form)
{
    $builder = $formMapper->getFormBuilder()->getFormFactory()->createBuilder(RouteReferrersType::class);

    $form->with('form.group_routes', [
        'translation_domain' => 'CmfRoutingBundle',
     ])->add($builder->get('routes'));
}

The only drawback with this implementation is the same as the one in StaticContentType. Any behavior or listener attached to the RouteReferrersType will not be used as the Type is not directly used. But this may be addressed later when all the sonata stuff will be moved in this bundle and will probably require an newer fields order, groups and tabs system.

[dbu]

just use the extensions explicitly

Does this mean creating it's own service extending the abstract one provider by the bundle owning the extension?

i fear i am not deep enough into forms to know how this works exactly. maybe just make it another stand alone form type instead of an extension? having to create a custom form type service sounds wrong. i think the explicit separate type is easiest to handle (also when you want to show routing in some context of the content but not in another content)

with sonata

The only drawback with this implementation is the same as the one in |StaticContentType|. Any behavior or listener attached to the `|RouteReferrersType| will not be used as the Type is not directly used. But this may be addressed later when all the sonata stuff will be moved in this bundle.

if we make RouteReferrers a type instead of an extension, does this change anything? could we then use the service instead of our own instance?

[fbourigault]

I think there will have a FormType that provide the fields. A FormExtension that will add the FormType to the current FormBuilder using the inhertit_data option. And then the AdminExtension will not use the FormExtension but directly the FormType to add the fields into the current FormMapper.

I will start working a PR against routing-bundle to let code speak ;)

[fbourigault]

@dbu code is now speaking. The example is simple but it present all classes I imagined.

[fbourigault]

I opened an issue about display/building form logic: https://github.com/sonata-project/SonataAdminBundle/issues/4243

[dbu]

great! as we now moved the admins to this bundle without extracting the forms as first step: should we close this issue and create a new one to track extracting form types?

[fbourigault]

Probably. I also think that we should not extract things from admin classes but write in each bundle reusable FormTypes and then use those FormTypes in sonata admins. This may allow us to write better FormTypes without being restricted by sonata.

[dbu]

absolutely. the issue could still be in this repo, as the admin code is here and is a starting point for doing the forms. @fbourigault can you create the new issue with a summary of what needs to be done and the references to the sonata issue you created and such?

[fbourigault]

FormTypes extraction is now tracked in #53