Sonataadminbundle: Set admin class into admin extension

[pzhuchkov]

To determine whether a child admin class.

It is necessary to determine whether to add an additional field.

For example in \Sonata\AdminBundle\Admin\AdminExtension:

/**
 * @var Admin
 */
protected $admin;

/**
 * set admin class
 *
 * @param \Sonata\AdminBundle\Admin\Admin $admin admin class
 *
 * @return void
 *
 */
public function setAdminClass(Admin $admin)
{
    $this->admin = $admin;
}

And in your extension configureFormFields() call:

if ($this->admin->hasParentFieldDescription()) {
       $form->add('example_field');
}

If need pull request i'm ready.

[pzhuchkov]

And in \Sonata\AdminBundle\Admin\Admin

public function addExtension(AdminExtensionInterface $extension)
{
    $extension->setAdminClass($this);
    $this->extensions[] = $extension;
}

[vyshkant]

I'm about to write a PR, @greg0ire @core23 @OskarStark WDYT?

[greg0ire]

1. I'm not sure I understood the issue
2. IIRC most (all?) methods of the admin interface have an argument to pass the admin
3. If you make a PR, don't change the current interface, which is to big. Create a new one instead (AdminAwareInterface?)

[vyshkant]

1. The issue is:
2. Some of methods does not have admin argument:

    /**
     * {@inheritdoc}
     */
    public function configureFormFields(FormMapper $formMapper)
    {
    }

    /**
     * {@inheritdoc}
     */
    public function configureListFields(ListMapper $listMapper)
    {
    }

    /**
     * {@inheritdoc}
     */
    public function configureDatagridFilters(DatagridMapper $datagridMapper)
    {
    }

    /**
     * {@inheritdoc}
     */
    public function configureShowFields(ShowMapper $showMapper)
    {
    }

So we don't know anything about extended admin when overriding these methods.

1. OK. But there are also some "next major todos" in AdminExtensionInterface, and I can just add one more commented line like //public function setAdmin(AdminInterface $admin);. But if I do that, there is a question: will we need to pass instance of AdminInterface to each method?

So I think we have two options:

• :+1: either to pass AdminInterface instance to all methods (including shown above),
• :-1: or to create setAdmin method and stop passing it.

[greg0ire]

👎 , in a separate interface. People who need the admin will get it w/o a BC-break, on 3.x, it does not violate the ISP. Only benefits!

[vyshkant]

 :-1: , in a separate interface. People who need the admin will get it w/o a BC-break, on 3.x, it does not violate the ISP. Only benefits!

I'm not sure I understand your idea. We will continue passing the AdminInterface instance to "90%" of methods (not to all of them), and create an interface which implies to set it once and stop passing it in every method.

I think there is a better way to avoid BC-break:

• don't touch AdminExtensionInterface (only add "next major" comment);
• add setAdmin method and protected $admin property to AbstractAdminExtension;
• check method_exists($extension, 'setAdmin') in AbstractAdmin
• use protected $admin in the children of AbstractAdminExtension.

BTW, these methods are not present in the AdminExtensionInterface and are waiting for next major:

    /**
     * {@inheritdoc}
     */
    public function getAccessMapping(AdminInterface $admin)
    {
        return [];
    }

    /**
     * {@inheritdoc}
     */
    public function configureBatchActions(AdminInterface $admin, array $actions)
    {
        return $actions;
    }

    /**
     * {@inheritdoc}
     */
    public function configureExportFields(AdminInterface $admin, array $fields)
    {
        return $fields;
    }

    /**
     * {@inheritdoc}
     */
    public function configureActionButtons(AdminInterface $admin, $list, $action, $object)
    {
        return $list;
    }

    /**
     * Returns a list of default filters.
     *
     * @param AdminInterface $admin
     * @param array          $filterValues
     */
    public function configureDefaultFilterValues(AdminInterface $admin, array &$filterValues)
    {
    }

What do you think?

[greg0ire]

My thoughts:

1. those commented methods are a clear ISP violation
2. the AbstractAdminExtension is a hacky patch for that
3. we should try to make things better, not worse, so maybe we should split that interface into several small interfaces? And guess what, I started doing just that, but for the AdminInterface. And both interfaces look a lot like each other, IIRC the main difference is that $admin argument.
4. extensions are good idea though, because the encourage composition over inheritance.

That means that an extension could probably look like this:

<?php
class MyExtension implements 
    AdminAwareInterface /** only if really needs access to admin */, 
    ButtonConfiguratorInterface,
    ShowFieldsConfigurator
{
        public function configureActionButtons($list, $action, $object)
        {
        }
// …
}

here, ShowFieldsConfigurator could also be implemented by AbstractAdmin... actually it's protected, so no, but I hope you get my point, if we see long term, that means => splitting of the admin, splitting of the admin and admin extension interfaces, splitting, splitting, splitting. And the end goal: files that are less than 3000 lines

[greg0ire]

And by the way, you probably do not need all the methods of the admin, but just some of them. So it should probably be named after the interface you need in your showFields method. What do you need to do?

[vyshkant]

splitting, splitting, splitting

I fully support this. I just wasn't mentally ready for this feat :) Now I am!

And by the way, you probably do not need all the methods of the admin, but just some of them. So it should probably be named after the interface you need in your showFields method. What do you need to do?

Oh, I don't need any method at all :) I needed the $admin instance to run get_class on it and then parse... But I'm ashamed to talk about this when we have such SOLID-talks here :satisfied:

So finally there's no sense in creating AdminAwareInterface because one big AdminInterface is in itself a bad idea. Isn't it?

[greg0ire]

Do you really need the class of the admin or the class of the model?

[vyshkant]

Do you really need the class of the admin or the class of the model?

I finally need the class of the model, and I was trying to get it parsing the class of the admin :no_mouth:

[pzhuchkov]

I already forgot about this issue +)

I wanted to get the admin class in the extension.
But for example, in the configureFormFields method, you can run $ formMapper-> getAdmin () and therefore no more or nothing is needed.

In general, it's best to pass AdminInterface to a method, not the class itself

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.