Splitting the admin class or how to stop violating the SRP

[greg0ire]

The AbstractAdmin class is huge, with many unrelated responsabilities. We need plans and ideas to make it smaller, or even make it disappear (and any other class that does not seem to have a precise, focused goal). This should lead by also splitting the AdminInterface into something.

For instance, we could start by moving all security-related methods to a new service, and create a new Twig method called admin_is_granted instead of having people call admin.is_granted directly.

Also, I think we should think very hard before merging any new methods / properties in the Admin class. For instance, I probably should not have merged #3822 .

Of course, it will be quite hard to create components that are completely independent from the admin, so we should settle for tightly-coupled solutions like in #3791 , that should become less and less tighly coupled as we extract functionality into small services that we can inject into other, higher level services.

[tim96]

For instance, I probably should not have merged #3822 .

We should stop add new functions because admin class is big, it is awesome reason @greg0ire :cry:

[greg0ire]

We should stop add new functions

… to the admin class. Adding them in dedicated classes would be the good solution. See #3943

And, to go even further : the AbstractAdmin class could even eventually disappear if we do a good job.

[tim96]

to the admin class. Adding them in dedicated classes would be the good solution. See #3943

Instead of big admin class we will have big admin extension class. Am I understand you correctly?

[greg0ire]

Am I understand you correctly?

I don't think you are.

What I mean is we will have several small services (that are not admin extensions) dedicated to one and only one task (could be exporting things, or checking access rights, etc…, a datagrid view like @ju1ius suggested #3943 , doesn't matter as long as you can tell what it is used for simply by reading its name), and make sure they are still easy to use from Twig templates and so on. BTW, AdminExtension is too big too : it contains hooks for doing things before / after update, but also methods to create or alter default objects, and methods to alter mappers. Too many responsabilities.

[tim96]

Too many responsabilities.

@greg0ire Do you have real cases(examples maybe) how to used some sonata admin components without admin class? Maybe slelect2 autocomplete, dashboard, datagridview, table with sort? For example, this method #3822 will not work without sonata admin, so I don't understand what benefits you will have, if move this method in another class.

[greg0ire]

If we do what @ju1ius says, you would create a DatagridView class with the showMosaicButton method (or something more generic). Then in your template or wherever you need that, you use datagrid_view.showMosaicButton.

The main benefit of not violating the SRP this is separation of concerns:

• less responsabilities means less dependencies, which should make several classes easier to unit test (and thus maintain), than one big god object
• less edits on the same class by everyone means less conflicts

We should be careful to keep this usable though, by introducing new variables in our twig templates, because if we force people to write things admin.datagrid_view.showMosaicButton, it is not more simple.

[tim96]

you would create a DatagridView class with the showMosaicButton method

Why I need to do it? This button doesn't connected with DatagridView, or maybe I miss something?

[tim96]

The main benefit of not violating the SRP this is separation of concerns:

Do you have plan what components you want to separate?

[greg0ire]

Why I need to do it?

It can be you or anyone else, I'm not saying you specifically should do that.

This button doesn't connected with DatagridView, or maybe I miss something?

Uuuuh… doesn't clicking on this button have to do with how the datagridView is displayed ?

Do you have plan what components you want to separate?

Not yet, I would like everyone to help with this, not sure which methods should go with which yet, I need to take a deeper look.

From the top of my head though, here is a starting list of the responsabilities I can see :

• translation
• security
• routing
• exporting
• creating a tab menu
• generating a listing
• generating a create/edit form
• generating a read only view

[tim96]

Uuuuh… doesn't clicking on this button have to do with how the datagridView is displayed ?

This button doesn't connected with datagridview only with listview. datagridview connectid with filters.

From the top of my head though, here is a starting list of the responsabilities I can see :

Nice list

[greg0ire]

This button doesn't connected with datagridview only with listview. datagridview connectid with filters.

I think you're thinking datagridview == filters because of the configureDatagridFilters method, which uses a DatagridMapper. But the data grid is a grid, not a bunch of filters. The data grid is the list.

Nice list

Thanks :) Feel free to suggest more!

[ju1ius]

@greg0ire I haven't put much toughts about it, but using events a bit more might help.

[greg0ire]

@greg0ire I haven't put much toughts about it, but using events a bit more might help.

Not sure we want that level of decoupling, but in extreme cases, it might indeed (for replacing postUpdate() & Co)

[soullivaneuh]

Didn't get time to read all your comments, but the exporting should definitely be optional and deactivated by default (on next major) IMHO.

We nearly never use it on our projects. Am I the only one?

I can understand this is useful for some people, but not as default.

[ju1ius]

We nearly never use it on our projects.

I always enable and configure exporters, but users never use them :smile:

[greg0ire]

I use the export on some views, in csv only, and after heavy tweaking

[tim96]

The data grid is the list.

The list it is a table - <table class="table table-bordered table-striped sonata-ba-list"> ?

We nearly never use it on our projects. Am I the only one?

@Soullivaneuh we use export to csv, on multiple projects. But if you will try to export more than 10000 records with left join multiple tables(status, priority and etc. for example) you will have an error on not very powerful server. At this moment query is not completely optimize.

It will be very cool if we can enable/disable export function using admin.

[soullivaneuh]

It will be very cool if we can enable/disable export function using admin.

And be disable by default on next major.

This kind of feature is great and maybe more used than I think, but it's still an optional feature.

I really think a fresh install of sonata should contains only common things to be clear.

It makes more sense to activate feature we need than take time to disable all features we don't want to have.

[greg0ire]

Found a method that should obviously not be part of the admin class : https://github.com/sonata-project/SonataAdminBundle/blob/3.x/Admin/AbstractAdmin.php#L1263 . This will probably be my next target. Shouldn't it be named attachAdminClassAndAlsoRequest :trollface: ?

[core23]

What about completly refactor the Admin to a kind of AdminConfiguration class? This new class would only contain all configure* methods and all public extension points for a concreate admin. All getters like getFilterParameters, getBaseRouteName, isCurrentRoute, ... which contains more logic could be moved to some other place (AdminFactory?).

I know this would take much time and could brake many projects, but it would help a lot to reduce this class.

[core23]

Added the related issues to the 4.0 milestone, so we can focus on the open cases what should be done to decouple the admin.

[neo-james]

Hi everyone, I'm new to symfony and of course new to SonataAdmin, So hope I'm suggesting a good thing.

How about making a bit featured/thin AbstractAdmin using following ideas: 1 Make use of configuration by doing:

1. Make a new AbstractAdmin (let's call it AbstractAdmin2) to load the (FormFields / $formMapper) , 'DatagridFilters' and other a like from configuration files like yaml or xml.
2. Loading (FormFields / $formMapper) in AbstractAdmin2 is done by a new Class for example named "ViewLoader" and it provide AbstractAdmin2 with the result.
3. A developer can override the "ViewLoader" class with his own code/implementation of providing the "ViewLoader" result to AbstractAdmin2
4. "ViewLoader" will load the config from files like "SampleBundle\Resources\config\EntityAdmin\list_view.yml"
5. Optional : Making class named "FastAdmin" extends "AbstractAdmin2" with no adding code.

Note: This what came up in my mind instantly and sure I agree it needs better naming and review but allow me share my thought.

What is so good about this:

1. It's not a breaking current code.
2. Many simple scenarios the "FastAdmin" can be used in service with no changes.For example : We can allow developer to not fill "class" in service.

What you think?

`

[neo-james]

2. Current methodology using current "AbstractAdmin" limit the views by mean of what if multiple list views is required for different scenarios like:

1. Having one list view for desktop and one for mobile.
2. Having one list view (grid) and view shows entities on map So by allowing/introducing ViewLoader and in future ViewPresenters one entity can have unlimited views and presenters (a new bundles can be done to introduce map view, heat view, chart view and ...etc)

[neo-james]

3 Suggestion to add a config file ( yaml or xml) per entity to define UI related attributes in config file. Like doctrine-mapping file but with more info that help in UI like validations Ex: a yml file might look like

Sonata\NewsBundle\Entity\BasePost:
    icon: '<i class="fa fa-users"></i>'
    fields:
        title:
            type: string
            label: 'Title'
            template: 'xyz'
            help: 'Set the title of a web page'
            validations:
                - MaxLength:32

[neo-james]

4 using the above idea about having a config file ( yaml or xml) per entity will help to:

1. Eliminate the dependency of the 'AdminBuilder' in #4103 on "AbstractAdmin" which I see is good to remove
2. Eliminate the redundancy in defining the List view and Form view

[greg0ire]

Thanks for caring @neo-james! I think there already is bundle that uses configuration file, I'll try to find it again. I think we should start by making it easier to do, and then maybe support one format (XML?) then maybe several

[greg0ire]

Found it. So the first step would be to look at this bundle and see what we can do to make their life easier (i.e. define an interface that they could implement) cc @marcoslibre

[neo-james]

Thanks for sharing the bundle, I'm looking at it right now. But do you think the ability to construct a view (like listview, fromview and others) from yml/xml should be in core bundle, I mean by default, And so the default path for new apps.

[greg0ire]

I personally think it would be a great improvement, but I'm not sure if we should support several formats: for instance Doctrine is planning on dropping yml support b/c maintaining it is a pain. Anyway, it does not change step 0 : making life easier for the bundle I referred to.

[neo-james]

Thanks for your reply, I agree with you regarding it's 'a great improvement'. Regarding 'yml support' ? Sorry to say but I don't know, I'm still new symfony so I can't judge.

making life easier for the bundle I referred to.

I agree the bundle is good, But I was thinking about one yml per view per entity and as I seen so far in the bundle 'SymfonyLibrinfoCoreBundle' it has configuration in one file

[greg0ire]

But I was thinking about one yml per view per entity and as I seen so far in the bundle 'SymfonyLibrinfoCoreBundle' it has configuration in one file

Not such a big deal… in the end you will still have something that loads metadata from a yaml block, whether it is in one or several files will not change much on our end.

[neo-james]

you will still have something that loads metadata from a yaml block, whether it is in one or several files will not change much on our end.

Agree

[sagikazarmark]

I like the idea of splitting logic from the abstract admin class, but not sure it will reduce complexity. Basically AbstractAdmin would become a facade...though reducing complexity was not the intent here.

[greg0ire]

@sagikazarmark the ultimate goal would be not to have that class at all in the end. IMO, the admin should just a series of services grouped together by a code (currently, the admin code), and you would only override (or, more accurately) replace some of them. The real issue with the abstract admin is that people keep doing PRs that want to add more methods to it. This is not viable in the long term.

[sagikazarmark]

How exactly do you imagine "grouping" those services?

As far as I can see either way you need some sort of control layer (which is the Admin class ATM). Whether you register services into it or extend it is an implementation detail from the library perspective.

To be a bit more specific: if I want to add feature X to my admin which does something with the listing (let's not be too specific, otherwise you could easily say it's already implemented 😄 ) how would I let sonata know where and what should it do with my service X?

[greg0ire]

@sagikazarmark a tag could be one way to do it. You would end up with something like this:

services:
    Custom\Listing\DedicatedService:
        tags: 
            - 'sonata_admin_service': 'listing'
            - 'admin_code': 'my_admin_code'

Only the second tag would actually be required if you use autoconfiguration

[sagikazarmark]

So you would still not allow anything to the user, there would still be a set of features which the user can alter with custom services, but adding new features would still require adding logic to the core admin, no?

[greg0ire]

adding logic to the core admin

As I said, ideally, there would be no core admin at all.

So you would still not allow anything to the user

What do you mean by that? Do we not allow things to the user currently?

there would still be a set of features which the user can alter with custom services

Ideally, you would alter said features by defining a service that would be used where needed instead of the default one. Inheritance would be discouraged by making default services final.

[sagikazarmark]

My point is: if there is no core admin, what manages the whole thing? If there is, and some feature is not supported, users would still be forced to add functionality ("new methods") to the core admin, otherwise being able to register custom services won't help.

I'm not saying your approach is not working, I'm just not sure it addresses the reason why people want to add new things to the core admin (yet). (Or of course I'm missing something entirely about the proposed solution or the intent of it)

[greg0ire]

if there is no core admin, what manages the whole thing?

services may depend on other services, but there should not be a big glue class that gives access to other services with the same admin_code. That's the job of the DIC. So when building the admin listing, you may be able to give it access to the admin routing service with the same admin code. Most services could be generic enough that you wouldn't actually need one service per admin code, but you could reuse the same instance.

The if you want to add new functionality, I guess you could create a new admin service, replace depending admin services so that you can inject your new service in them, and let the DIC do the actual injection.

[sagikazarmark]

Sorry, I don't understand. "admin service", "admin services", "admin_code", "depending services" is just not clear terminology to me. It's quite hard to think about too abstract things, so I'm probably missing the concrete plan from this initiative.

The way I see things are now: there are "admin controllers" which are responsible for the request-response handling. Bellow them, there is "the admin service" at the moment (either the core admin or an extended version), which handles all possible admin features: listing, editing, creating and deleting items.

There are additional features in this "core admin service", like adding buttons to a list, replacing templates, etc.

Currently when someone wants to add a new feature to "all admin instances", those PRs are opened which you were talking about adding a new feature X which needs to be done on the "core admin level", and can' be done at the moment via some sort of extension.

Compared to the state above, what are you suggesting? What should be broken into smaller services? How would that work with "admin controllers" (what would you inject instead of the admin class? several smaller services?)? How would that solve the "missing feature X" problem for which people opened PRs for (is it the intent at all to make that sort of problem go away?)?

[greg0ire]

Compared to the state above, what are you suggesting? What should be broken into smaller services?

all the functionality inside AbstractAdmin, which should ultimately no longer exist

How would that work with "admin controllers" (what would you inject instead of the admin class? several smaller services?)?

Yes, but only the one that are actually needed

How would that solve the "missing feature X" problem for which people opened PRs for (is it the intent at all to make that sort of problem go away?)?

That does not mean the admin suddenly becomes feature complete. What that means is that people will make PRs against the smaller services, hopefully for only 2 things if we respect the OCP:

• fixing bugs;
• adding extension points.

Ideally, when you miss a feature, the final service that misses it has an extension point (a constructor argument, for instance, or a setter) that allows you to change the default behavior if it needs a change. But also, you might no need to change any admin service. If the thing is well architectured, a new feature produces only new files, and a few small modifications to make sure the new services are available in the controllers that need it.

[greg0ire]

If you want an example, have a look at the BreadcrumbsBuilder class, which was initially part of the AbstractAdmin class. I extracted it so now it is a component of the admin class, but ideally, in the future, "admin controllers" would access it directly without any facade. No need for a gigantic proxy that acts like a subset of the DIC.

[greg0ire]

Next component to emerge: the template registry. There should be one service, created from the list of default templates. When a template is needed for a given admin code (no AbstractAdmin involved here), then it should find the corresponding service (<admin_code>.template_registry), and grab templates from there. For each existing admin_code, such a service should be created (but in 90% of the case, that will just be an alias to the default service, which should be very low cost).

So if we want to get rid of the AbstractAdmin god object, one way would be to focus on admin codes instead. So what's an admin code? Right now, it's a simple string, know for being the first argument to AbstractAdmin. It can contain pipes in case of an admin hierarchy. That means a list of admin codes can be obtained from the list of AbstractAdmin services, and a list of template registry aliases can be created from there. We would have comment.template_registry => default_template_registry. However, the user should be able to override any of those aliases to provide their own template registry service (might be another instance of TemplateRegistry , might be another instance of another class). Similar things could be done for the breadcrumbs builder component.

That means the config should look like this:

sonata_admin:
    admins:
        some_admin_code:
            template_overrides:
                    edit: my_custom_template.html.twig
        some_other_admin_code:
            services:
                template_registry: '@Some\Custom\TemplateRegistry'
                breadcrumbs_builder: '@Some\Custom\BreadcrumbsBuilder'

To achieve BC, we could inject the template registry in the admin and proxy all template-related calls to it (and trigger a deprecation error if there are any).

What do you think @TimoBakx @sonata-project/contributors ?

[TimoBakx]

Would it perhaps be easier to have a single TemplateRegistry service that has all the default (general) templates in it and a list of all the templates that need to be overridden per admin? This would have methods like getAdminTemplate($adminCode, $name): string which will return the overridden template for that admin, or the default/general one of none is set. And a getGeneralTemplate($name): string function which will return the default/general template, to replace the Pool::getTemplate() function. In that case, only the general list and a (often empty or very small) list of overriding templates are stored in memory, instead of a full copy of all templates for each admin that has a single change.

We can statically (using the services XML) fill the TemplateRegistry with the default values from the parameters. And inject the TemplateRegistry in the Twig extension.

For BC, we can inject the registry itself into the AbstractAdmin and Pool to be called inside the getTemplate(), setTemplate() and setTemplates() methods.

[greg0ire]

I'd be against an all-knowing template registry. I think that the same issue will arise for all other admin components we will extract from the admin, and that the knowledge about the admin code should be outside of them, because it keeps them more simple. I don't think we should take any memory micro-optimization into consideration.

To me you should have several admin codes, and for each of these code, you should get many services, and in 90% of the cases, those services will just be aliases. And there shouldn't be any abstract admin god object, and neither should one such service know about admin code. We should really try to run away from that "global service manager that knows everyone" mindset. That TemplateRegistry could be used to extract the template responsability from the Pool too, it looks really similar indeed.

To sum up, all services will probably need that kind of "override" vs "default" thing. The override should be managed in the same way everytime, without the service knowing if it's used as default or as an override. That's a separate responsability, and it should be easy (and efficient) to manage with the DIC (aliases cost nothing I think).

[TimoBakx]

If we indeed dynamically create a separate service per admin, how should I get that service in, for example, the Twig extension added earlier? Should I inject the service locator/container itself in order to get the right service to pull the template name from?

[greg0ire]

I think we should leverage the service locator feature (and bump to 3.4).

[greg0ire]

That means we should tag all template registry services / service aliases with the same tag (can and should be done automatically).

[TimoBakx]

I'll look into it.