Does not mark ModelManager as final anymore

[VincentLanglet]

Related to sonata-project/SonataDoctrineMongoDBAdminBundle#437 (review)

[core23]

But in my personal project I override the ModelManager,

* I override the `getDataSourceIterator` to use my own SourceIterator

And I think that someone could just want to override some config methods like

getDefaultSortValues
getDefaultPerPageOptions
...

How to do this if the ModelManager is final ?

It's not possible to use service decoration here?

[VincentLanglet]

It's not possible to use service decoration here?

Not sure to understand what you mean here.

I inject my own ModelManager instead of the doctrineORMModelManager everywhere I need it.

But I need to extends the doctrineORMModelManager to avoid copy-pasting all the other methods I don't need to override.

This is the implementation of getDataSourceIterator.

    public function getDataSourceIterator(DatagridInterface $datagrid, array $fields, $firstResult = null, $maxResult = null)
    {
        $datagrid->buildPager();
        $query = $datagrid->getQuery();

        $query->select('DISTINCT '.current($query->getRootAliases()));
        $query->setFirstResult($firstResult);
        $query->setMaxResults($maxResult);

        if ($query instanceof ProxyQueryInterface) {
            $sortBy = $query->getSortBy();

            if (!empty($sortBy)) {
                $query->addOrderBy($sortBy, $query->getSortOrder());
                $query = $query->getQuery();
                $query->setHint(Query::HINT_CUSTOM_TREE_WALKERS, [OrderByToSelectWalker::class]);
            } else {
                $query = $query->getQuery();
            }
        }

        return new DoctrineORMQuerySourceIterator($query, $fields);
    }

I can't use another SourceIterator without overriding this method.

Maybe changing the new DoctrineORMQuerySourceIterator for something like

return $this->sourceIteratorFactory->create($query, $fields);

could help. And then, people can override sourceIteratorFactory thanks to service decoration, if I understand https://symfony.com/doc/4.4/service_container/service_decoration.html.

But still, there are method like

getDefaultSortValues
getDefaultPerPageOptions
...

which are some methods that developper will often wants to override.
We can't ask people to write a full ModelManager for just overriding one method. So we need to provide an easy way.

[franmomu]

But still, there are method like

getDefaultSortValues
getDefaultPerPageOptions
...

which are some methods that developper will often wants to override.

IMHO those methods shouldn't belong to ModelManagerInterface.

[VincentLanglet]

But still, there are method like

getDefaultSortValues
getDefaultPerPageOptions
...

which are some methods that developper will often wants to override.

IMHO those methods shouldn't belong to ModelManagerInterface.

This is the implementation on master:

    public function getDefaultSortValues(string $class): array
    {
        return [
            '_page' => 1,
            '_per_page' => 25,
        ];
    }

    public function getDefaultPerPageOptions(string $class): array
    {
        return [10, 25, 50, 100, 250];
    }

We could deprecate these methods. And implement this directly in SonataAdmin WDYT ?

[VincentLanglet]

I made sonata-project/SonataAdminBundle#6458 as an example of the possible changes.

[core23]

Not sure to understand what you mean here.

I was talking about this: https://symfony.com/doc/current/service_container/service_decoration.html You create a proxy class that will delegate all calls to the original one and just reimplement the one you need.

[VincentLanglet]

Not sure to understand what you mean here.

I was talking about this: https://symfony.com/doc/current/service_container/service_decoration.html You create a proxy class that will delegate all calls to the original one and just reimplement the one you need.

I never used service decoration.

Does it work this way ?

class MyModelManager implement ModelManagerInterface {
    public function __construct(ModelManager $modelManager) {
        $this->modelManager = $modelManager;
    }

    public function supportsQuery(object $query) {
        return $this->modelManager->supportsQuery($query);
    }

    // Same for all the methods I don't want to override.

    public function getDataSourceIterator(DatagridInterface $datagrid, array $fields, $firstResult = null, $maxResult = null)
    {
        // Custom code
    }
}

I hope it's not the right way. So, if not, can you give a real example ?

[core23]

I hope it's not the right way. So, if not, can you give a real example ?

That's how you should do it. I know there is some copy & paste stuff, but that's an other problem (god object).

A few months / years ago we decided to advice service decoration so we can close our api. The ideas was based on this. This will allow us to do some changes that would otherwise result in a BC break.

[VincentLanglet]

I hope it's not the right way. So, if not, can you give a real example ?

That's how you should do it. I know there is some copy & paste stuff, but that's an other problem (god object).

IMHO, we should fix the god object problem BEFORE changing the class for final.

That's why I proposed:

• Deprecating the getDefault... methods cf: Deprecate DatagridManagerInterface SonataAdminBundle#6458 ; the configuration will be done in Admin.
• Rewriting getDataSourceIterator for something like

    public function getDataSourceIterator(DatagridInterface $datagrid, array $fields, $firstResult = null, $maxResult = null)
    {
        $datagrid->buildPager();
        $query = $datagrid->getQuery();

        $query->select('DISTINCT '.current($query->getRootAliases()));
        $query->setFirstResult($firstResult);
        $query->setMaxResults($maxResult);

        if ($query instanceof ProxyQueryInterface) {
            $sortBy = $query->getSortBy();

            if (!empty($sortBy)) {
                $query->addOrderBy($sortBy, $query->getSortOrder());
                $query = $query->getQuery();
                $query->setHint(Query::HINT_CUSTOM_TREE_WALKERS, [OrderByToSelectWalker::class]);
            } else {
                $query = $query->getQuery();
            }
        }

        return $this->sourceIteratorFactory->create($query, $fields);
    }

And that allow to decorate the sourceIteratorFactoryInterface instead of the whole ModelManager.

[franmomu]

• Rewriting getDataSourceIterator for something like

    public function getDataSourceIterator(DatagridInterface $datagrid, array $fields, $firstResult = null, $maxResult = null)
    {
        $datagrid->buildPager();
        $query = $datagrid->getQuery();

        $query->select('DISTINCT '.current($query->getRootAliases()));
        $query->setFirstResult($firstResult);
        $query->setMaxResults($maxResult);

        if ($query instanceof ProxyQueryInterface) {
            $sortBy = $query->getSortBy();

            if (!empty($sortBy)) {
                $query->addOrderBy($sortBy, $query->getSortOrder());
                $query = $query->getQuery();
                $query->setHint(Query::HINT_CUSTOM_TREE_WALKERS, [OrderByToSelectWalker::class]);
            } else {
                $query = $query->getQuery();
            }
        }

        return $this->sourceIteratorFactory->create($query, $fields);
    }

In fact, looks like it should belong somewhere else, apparently there are no references to the class itself (I haven't thought where though).

[VincentLanglet]

In fact, looks like it should belong somewhere else, apparently there are no references to the class itself (I haven't thought where though).

I'll be happy to move this somewhere else if you have a good idea.

The part

$query->select('DISTINCT '.current($query->getRootAliases()));
        $query->setFirstResult($firstResult);
        $query->setMaxResults($maxResult);

        if ($query instanceof ProxyQueryInterface) {
            $sortBy = $query->getSortBy();

            if (!empty($sortBy)) {
                $query->addOrderBy($sortBy, $query->getSortOrder());
                $query = $query->getQuery();
                $query->setHint(Query::HINT_CUSTOM_TREE_WALKERS, [OrderByToSelectWalker::class]);
            } else {
                $query = $query->getQuery();
            }
        }

is persistenceBundle-specific. I think that currently most of the persistenceBundle-specific code is injected with the ModelManager (especially when it's about query), that's why it's here.

[franmomu]

I'd create something under Exporter/ directory, I thought about Exporter/DataSourceInterface but maybe looks too generic and I don't know if we have more methods to add there apart from getIterator or createIterator.

[VincentLanglet]

I'd create something under Exporter/ directory, I thought about Exporter/DataSourceInterface but maybe looks too generic and I don't know if we have more methods to add there apart from getIterator or createIterator.

I made a PR to discuss about this sonata-project/SonataAdminBundle#6463

[SonataCI]

Could you please rebase your PR and fix merge conflicts?