Make sonata.media.admin.media.manager an alias of sonata.admin.manager.orm / sonata.admin.manager.doctrine_mongodb

[jorrit]

Subject

Since sonata-project/doctrine-orm-admin-bundle 3.22 the constructor of Sonata\DoctrineORMAdminBundle\Model\ModelManager throws a deprecation warning when the second argument is not provided. The file doctrine_orm_admin.xml of the media bundle registers this class as the service sonata.media.admin.media.manager without this argument. I think this service can be made an alias of sonata.admin.manager.orm. This makes sure the parameters are correct regardless of the orm bundle version.

Maybe I've overlooked something that would make this solution invalid, but it seems this solution also makes sure there is just one instance of the ModelManager, which seems a plus.

I am targeting this branch, because it is a backwards compatible change.

Changelog

### Changed
- When using Doctrine ORM or MongoDB the service `sonata.media.admin.media.manager` is now an alias of `sonata.admin.manager.orm` or `sonata.admin.manager.doctrine_mongodb` instead of a separate service implemented by the same class.

### Fixed
- Deprecation notice in `Sonata\DoctrineORMAdminBundle\Model\ModelManager` and `Sonata\DoctrineMongoDBAdminBundle\Model\ModelManager`.

[franmomu]

Seems like a fine solution 👍

[phansys]

I think this is a good proposal, I'd just like to have a clear idea about how both services are currently registered, in order to avoid any potential edge case where the aliased service could not be found.
Could you please provide a little explanation @jorrit?

[jorrit]

The service sonata.admin.manager.orm is created unconditionally by the SonataDoctrineORMAdmin bundle (see https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Resources/config/doctrine_orm.xml).
It is set to the same class as sonata.media.admin.media.manager but depending on the version of the ORM bundle it will have the right amount of arguments.

[phansys]

This service was unconditionally available before this change. Now, it aliases a service which might not exist if doctrine/orm is not installed.
IMO, if we decide to make this change, we should take the same action for all the available managers.

[jorrit]

The class these two services are currently referring to is Sonata\DoctrineORMAdminBundle\Model\ModelManager so right now these can only exist when SonataDoctrineORMAdminBundle is installed. Nothing will change there.

Furthermore, sonata_orm_admin.xml will only be loaded when the sonata_media.db_driver setting is set to doctrine_orm.

I would prefer to remove the sonata.media.admin.media.manager service altogether and change the setModelManager call of sonata.media.admin.media to sonata.admin.manager.orm directly, but the service is public so that would be a BC break.

[phansys]

I'm referring to these sibling services:

The phpcr implementation seems to be following the same alias approach:

SonataMediaBundle/src/Resources/config/doctrine_phpcr_admin.xml

Line 21 in f18c04b

<service id="sonata.media.admin.media.manager" alias="sonata.admin.manager.doctrine_phpcr" public="true"/>

But not mongodb:

SonataMediaBundle/src/Resources/config/doctrine_mongodb_admin.xml

Line 43 in f18c04b

<service id="sonata.media.admin.media.manager" class="Sonata\DoctrineMongoDBAdminBundle\Model\ModelManager" public="true">

[jorrit]

I hadn't looked at the other persistence bundles. It's interesting that PHPCR already uses an alias. This confirms that it works. I changed mongodb too, which seems to have the same deprecation situation (see sonata-project/SonataDoctrineMongoDBAdminBundle@b382c81)

[phansys]

Excellent!
I think we could remove the "sonata.media.admin.media.manager" definition if its aliased service doesn't exist (which is the case for custom managers).

[jorrit]

@phansys What situation would that be? Wouldn't sonata.admin.manager.orm point to the custom manager also?

[phansys]

@phansys What situation would that be? Wouldn't sonata.admin.manager.orm point to the custom manager also?

We are currently supporting 3 built-in managers: orm, doctrine_mongodb and doctrine_phpcr. I was thinking that also custom managers were allowed and inferred from the manager_type option, but this is not the case:

SonataMediaBundle/src/DependencyInjection/Configuration.php

Lines 45 to 52 in f18c04b

	->scalarNode('db_driver')
	->defaultValue('no_driver')
	->info('Choose persistence mechanism driver from the following list: "doctrine_orm", "doctrine_mongodb", "doctrine_phpcr"')
	->validate()
	->ifNotInArray(self::DB_DRIVERS)
	->thenInvalid('SonataMediaBundle - Invalid db driver %s.')
	->end()
	->end()

So, forget about my last comment.

[phansys]

Thank you @jorrit!