sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Cannot override final method AbstractAdmin->getDataSourceIterator() #7805

Closed lbcd closed 2 years ago

lbcd commented 2 years ago

Before we could override this function getDataSourceIterator() in Admin classes. Now after an update of sonata admoin I can't do it anymore. It can't be overrided as it is marked as "final" in parent.

So how can I update my source to bypass this BC break ?

VincentLanglet commented 2 years ago

In 4.x getDataSourceIterator is final.

You can implement your own https://github.com/sonata-project/SonataAdminBundle/blob/4.x/src/Exporter/DataSourceInterface.php and configure your admin to use your custom service instead of the default one.

lbcd commented 2 years ago

Thank you @VincentLanglet.

How can I configure my admin to use my custom service instead of the default one?

Should I use $this->setDataSource(); in the constructor or is there a better way?

VincentLanglet commented 2 years ago

Take a look at https://docs.sonata-project.org/projects/SonataAdminBundle/en/4.x/reference/advanced_configuration/#service-configuration

I won't say more to see if the doc is good enough or if a PR is needed to improve it.

lbcd commented 2 years ago

I understand a good way to have a good doc.

Was not easly to understand the doc. And when I think I have understand it doesn't work.

I create a service:

    admin.user_purchase.datasource:
        class: Admin\Services\UserPuchaseDatasource 

Here is my Datasource file :

final class UserPuchaseDatasource implements DataSourceInterface
{
    public function createIterator(ProxyQueryInterface $query, array $fields): SourceIteratorInterface
    {
        die('Work ?');
    }
}

Then I add it to my Admin service :

    admin.user_purchase:
        class: Admin\UserPurchaseAdmin
        arguments:
           ...
        tags:
            -
                name: sonata.admin
                manager_type: orm
                group: admin
                label: "Admin.UserPurchase"
                data_source: "admin.user_purchase.datasource"

But the die never append. And I don't have any error.

VincentLanglet commented 2 years ago

Was not easly to understand the doc.

Any suggestion is welcomed :)

    admin.user_purchase:
        class: Admin\UserPurchaseAdmin
        arguments:
           ...
        tags:
            -
                name: sonata.admin
                manager_type: orm
                group: admin
                label: "Admin.UserPurchase"
                data_source: "admin.user_purchase.datasource"

This should work IMHO.

The die should only happen when the createIterator is called, so when you're trying to export something. If not, you could try to debug call getSourceIterator() on the admin to know which class is loaded.

lbcd commented 2 years ago

Thank you @VincentLanglet my mistake was to thing that this createIterator was call on the list page. So it works.

My suggestion for the doc is to add all tags in the example.

So many thanks for your answers. Hope that this conversation can help others.

VincentLanglet commented 2 years ago

My suggestion for the doc is to add all tags in the example.

Can you provide a PR ?

lbcd commented 2 years ago

I will try do it.

mpoiriert commented 2 years ago

@VincentLanglet I think there is an issue with the abstraction here.

The getDataSourceIterator will return an \Iterator. But we have to rely on the DataSourceInterface that may not be appropriate.

The getDataSourceIterator is really generic which is good but the DataSourceInterface is opiniated which remove all flexibility of the getDataSourceIterator (since it's final).

VincentLanglet commented 2 years ago

I don't see any issue here.

It's the same way that we rely on a ModelManager, a TemplateRegistry or a FieldDescriptionFactory.

If you need to provide another Iterator than the one by default by Sonata, you just have to implement the DataSourceInterface which is not hard to implements.

mpoiriert commented 2 years ago

I understand but the interface signature:

public function createIterator(ProxyQueryInterface $query, array $fields): \Iterator;

Don't provide enough to be very flexible.

In some of my use case I need to join on other table base on the datagrid filter configuration.

With the current signature I don't really have access to those. I would need to inject the admin somewhere which will not really be possible because of a circular depency, so I will need to inject the admin pool, a attribute to have the name than make fetch the datagrid filter base on the request, make sure the ProxyQueryInterface is properly configure etc.

I might end up overriding the export action completely when in fact allowing to just override the getDataSourceIterator() would be enough.

VincentLanglet commented 2 years ago

I understand but the interface signature:

public function createIterator(ProxyQueryInterface $query, array $fields): \Iterator;

Don't provide enough to be very flexible.

Same could be said for all the other methods.

ModelManager::find()

is just taking a class and an id. But someone could come up with the id about doing different query base on something configured on the admin.

The current implementations are way to complicated, and we've to focus simplicity.

In some of my use case I need to join on other table base on the datagrid filter configuration.

Filters can be found in the Request if needed.

With the current signature I don't really have access to those. I would need to inject the admin somewhere which will not really be possible because of a circular depency, so I will need to inject the admin pool, a attribute to have the name than make fetch the datagrid filter base on the request, make sure the ProxyQueryInterface is properly configure etc.

I often get the admin with

$admin = $this->adminFetcher->get($this->getRequestStack()->getCurrentRequest());

this might be enough.

And I find weird the fact the dataSource is adding some joins on the query. This one should be touched much. The current implementation of getDataSourceIterator() is

$datagrid = $this->getDatagrid();
        $datagrid->buildPager();

        $fields = [];

        foreach ($this->getExportFields() as $key => $field) {
            if (!\is_string($key)) {
                $label = $this->getTranslationLabel($field, 'export', 'label');
                $key = $this->getTranslator()->trans($label, [], $this->getTranslationDomain());
            }

            $fields[$key] = $field;
        }

        $query = $datagrid->getQuery();

        return $this->getDataSource()->createIterator($query, $fields);

Did you considered implementing your own buildPager/getQuery method instead ? This could also improve performances on the list.

mpoiriert commented 2 years ago

$admin = $this->adminFetcher->get($this->getRequestStack()->getCurrentRequest());

Relying on the request to in a data source iterator make it tightly coupled with too many thing.

If the DataSourceIterator received the admin directly (Since it's tightly couple in the admin namespace).

The admin may have an interface like FieldExportableInterface that would have getDataGrid() and getExportFields().

The logic of the getDataSourceIterator would be in the DataSource directly which would be the default behaviour.

Allowing to override getDataSourceIterator or having more flexibility in a DataSourceIterator since it will receive the Admin itself.

Obviously all of this would need to be thing for backward compatibility etc.

In my use cases I don't export the exact data we see in the admin.

I just think that something that was simple to do became complicated.

VincentLanglet commented 2 years ago

You can still provide a PR to call

$this->getDataSource()->createIterator($query, $fields, $this);

and the interface might be updated in the next major.

It will be better to discuss about this on a opened PR rather than a closed issue