only load service.xml if SonataDoctrineORMAdminBundle is registered

[OskarStark]

I am targetting this branch, because this is BC.

Changelog

### Changed
- renamed `service.xml` to `service_orm.xml`
- only load `service_orm.xml` if `SonataDoctrineORMAdminBundle` is registered

[greg0ire]

What if you don't use the ORM?

[core23]

ATM the file https://github.com/sonata-project/SonataTranslationBundle/blob/2.x/Resources/config/service.xml is always loaded. Maybe we should load it with a condition like the other services instead.

[dbu]

afaik this bundle can also be used with the phpcr-odm admin (or at least it used to be possible at some time) so we should avoid a hard dependency.

[OskarStark]

then we should suggest it?

[core23]

Yes, and we should add the missing condition in the config

[OskarStark]

Yes, and we should add the missing condition in the config

what do you mean?

[core23]

Add a check if the doctrine orm bundle is defined:
https://github.com/sonata-project/SonataTranslationBundle/blob/2.x/DependencyInjection/SonataTranslationExtension.php#L39

[dbu]

Add a check if the doctrine orm bundle is defined: https://github.com/sonata-project/SonataTranslationBundle/blob/2.x/DependencyInjection/SonataTranslationExtension.php#L39

please do not check for class_exists, but do ```php $bundles = $container->getParameter('kernel.bundles'); if (array_key_exists('SonataDoctrineOrmAdminBundle', $bundles)) { $loader->load('service_orm.xml'); } ``` and move the orm dependent services to a separate configuration file to not skip loading everything.

[OskarStark]

I removed the dependency and added the check

[OskarStark]

I updated the PR Header

[greg0ire]

What about composer.json? No changes?

[OskarStark]

shall we rename service.xml to service_orm.xml analogously to service_phpcr.xml ?

[dbu]

i am not sure we need to suggest orm in composer.json. suggestions are used to tell there is additional features if the optional dependency is available. but this bundle adds additional features to the admins, so people already use orm and want translations, not the other way around.

[dbu]

looking at the other places, its checking for phpcr.enabled. should we do the same here? or is there no explicit configuration for that?

[OskarStark]

Not needed, imo, because we don't have a config node which can be enabled...

        $rootNode
            ->children()
                ->arrayNode('locales')
                    ->info('The list of your frontend locales in which your models will be translatable.')
                    ->requiresAtLeastOneElement()
                    ->prototype('scalar')->end()
                ->end()
                ->scalarNode('default_locale')
                    ->defaultValue('en')
                    ->info('The frontend locale that is used by default.')
                ->end()
                ->arrayNode('gedmo')
                    ->canBeEnabled()
                    ->children()
                        ->arrayNode('implements')
                            ->requiresAtLeastOneElement()
                            ->prototype('scalar')->end()
                        ->end()
                        ->arrayNode('instanceof')
                            ->requiresAtLeastOneElement()
                            ->prototype('scalar')->end()
                        ->end()
                    ->end()
                ->end()
                ->arrayNode('knplabs')
                    ->canBeEnabled()
                    ->children()
                        ->arrayNode('implements')
                            ->requiresAtLeastOneElement()
                            ->prototype('scalar')->end()
                        ->end()
                        ->arrayNode('instanceof')
                            ->requiresAtLeastOneElement()
                            ->prototype('scalar')->end()
                        ->end()
                    ->end()
                ->end()
                ->arrayNode('phpcr')
                    ->canBeEnabled()
                    ->children()
                        ->arrayNode('implements')
                            ->requiresAtLeastOneElement()
                            ->prototype('scalar')->end()
                        ->end()
                        ->arrayNode('instanceof')
                            ->requiresAtLeastOneElement()
                            ->prototype('scalar')->end()
                        ->end()
                    ->end()
                ->end()
            ->end();
``

[OskarStark]

shall we rename service.xml to service_orm.xml analogously to service_phpcr.xml ?

but how could we achieve this without a BC break, or shall I do this in master afterwards?

i am not sure we need to suggest orm in composer.json. suggestions are used to tell there is additional features if the optional dependency is available. but this bundle adds additional features to the admins, so people already use orm and want translations, not the other way around.

You are right @dbu

[dbu]

i don't see renaming a service file as a BC break. people are not supposed to use those directly. and not loading dead services (that would trigger php fatal errors when trying to use) when not using sonata orm seems also not like a BC break to me.

[dbu]

if somebody wrote a compiler pass to remove them instead of doing this bugfix PR you are now doing, they deserve at least some surprise.

[OskarStark]

done

[greg0ire]

Thanks @OskarStark !

[OskarStark]

Thanks for your feedback guys!