search

php-translation / symfony-bundle

Symfony integration for Translations
MIT License
327 stars 94 forks source link

What about using a `_defaults` section in services files. #372

Closed odolbeau closed 4 years ago

odolbeau commented 4 years ago

What about moving all autowire & autoconfigure to a _defaults section?

_defaults:
    autowire: true
    autoconfigure: true

Originally posted by @rvanlaak in https://github.com/php-translation/symfony-bundle/pull/370

odolbeau commented 4 years ago

I don't know if Symfony suggest something for bundles?

Nyholm commented 4 years ago

That is a good question.

There is an issue with autoconfigure, what if I (as an application author) have my custom kernel and chose not to include all the "standard" compiler passes. Then autoconfigure will not work as we (as library authors) intended.

Sure, this is a rare scenario, but the fix is super simple from the bundle's side. I think we can write the proper DI tags ourself.

I dont see the same downside with autowire,

rvanlaak commented 4 years ago

@Nyholm interesting scenario.

Instead of compiler passes for registering tags I'd suggest using _instanceof: in combination with autoconfigure:true so userland can not disable them.

_instanceof:
    TranslatorInterface:
        tags: 'translation.translator'
XWB commented 4 years ago

In my opinion, bundles should not rely on the assumption that the user has autowire enabled.

rvanlaak commented 4 years ago

Did some extra investigation to provide a full picture on the issue:

It's the Bundle class that (via the compiler pass) imports the yaml service configuration. The autowiring defaults apply to the yaml file. So to my best knowledge, disabling autowire / autoconfigure in userland's service.yaml does not affect any other service import.

Did check the ContainerBuilder signatures, and did not find a way to get / override the compiler passes (there are no "remove" or "get" methods). Would there be any way to override a pass (except for not adding TranslationBundle in bundles.php at all)?

odolbeau commented 4 years ago

I took a look at the Symfony documentation and here is what I found: https://symfony.com/doc/current/bundles/best_practices.html#services

Services should not use autowiring or autoconfiguration. Instead, all services should be defined explicitly.

Furthermore, it looks like it wasn't a good idea to use FQCN... :/

If the bundle defines services, they must be prefixed with the bundle alias instead of using fully qualified class names [...]

rvanlaak commented 4 years ago

Great find, because that gives us our answer 👍 Feel free to close this issue, unless we want to document following these best practices somewhere.

Nyholm commented 4 years ago

Did check the ContainerBuilder signatures, and did not find a way to get / override the compiler passes (there are no "remove" or "get" methods). Would there be any way to override a pass (except for not adding TranslationBundle in bundles.php at all)?

You can just not add compiler passes.

So autowire/autoconfigred cannot be "disabled". But a user can configure how "autoconfigure" is working. And there is no way a bundle can detect it.

I agree with @XWB and the best practices. I'll close this issue.

Furthermore, it looks like it wasn't a good idea to use FQCN

For services that we intend the user to use, they should use FQCN. For internal services for our bundle only, they should not use FQCN.

But, I really dont mind if all services have FQCN. =)