Defaults to service id over names

[homersimpsons]

Q	A
Branch?	2.x
Bug fix?	no
New feature?	yes
BC breaks?	no (targeting 2.x)
Deprecations?	no
Fixed tickets	First step for #1485
License	MIT
Doc	done

[coveralls]

Coverage decreased (-1.9%) to 79.568% when pulling 6919ef5c9558af7b9fc62dc1ad1c97d1776a73a0 on homersimpsons:patch-1 into 747966c4a8b74d20115afeb1e88c4399fff2f185 on liip:2.x.

[dbu]

btw, for the cs failure you can rebase on 2.x to get them fixed.

[homersimpsons]

@dbu Yes I will update the doc, I was just waiting for feedback before doing so.

[dbu]

@homersimpsons do you have time to wrap this up?

[dbu]

we are heading towards the 3.x release. it could be a good moment to change this.

[homersimpsons]

Reading through Symfony's documentation, I understand that it is not recommended to use autowiring in the bundle (https://symfony.com/doc/current/bundles/best_practices.html#services).

For public services, aliases should be created from the interface/class to the service id. For example, in MonologBundle, an alias is created from Psr\Log\LoggerInterface to logger so that the LoggerInterface type-hint can be used for autowiring.

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

So I think we should define the various loader twice, once with the FQN and then alias it. But doing so means we will have 2 possible references for a given service, so we may need to call the addLoader method twice. What do you think ?

It looks like the TwigBundle still is registering some autowiring: https://github.com/symfony/twig-bundle/blob/42c4a60f1b83894cd85a6b00533f8216c413ac11/DependencyInjection/TwigExtension.php

(note that I did change the target to 3.x, but the current changes should be applicable to 2.x).

[dbu]

yes, we should not rely on autowiring in the services we define in the bundle, as that can be fragile.

we can and should create FQN alias for services that consumers are expected to use. afaik when they are alias, the loaders should work correctly and not see them twice (we don't define 2 services but alias one service with another name)

[homersimpsons]

yes, we should not rely on autowiring in the services we define in the bundle, as that can be fragile

So we cannot rely on a Service Locator ? If we can rely on a service locator (using https://symfony.com/doc/current/service_container/service_subscribers_locators.html#using-service-locators-in-compiler-passes) this should be rather straight forward (but may be backward incompatible)

[dbu]

i think the service locator is a special case and imho not covered by the no autowiring best practice.

if i understand the compiler passes correctly, we want to both load our own things and allow the user to provide their own services. when we do this against the 3.x branch we don't need to worry too much about strict backwards compatibility.

in #1554 we will add a few aliases for FQN service names. in general, i don't think we should change most services to FQN though - they are not meant to be used from outside the bundle, and many of them can have several services with the same class defined (different stacks etc) so autowiring would not be practical anyways.

i think your changes are fine - you only add autotagging, but when we define the services inside the bundle, we already set the explicit tag, so all is fine.

can you adjust the documentation for registering custom filter loaders/processors/binary loaders to reflect the autotagging? apart from that imho this pull request is ready to be merged.

[homersimpsons]

@dbu I just updated the documentation. As it is currently it should be usable in 2.x so maybe the best is to:

1. Merge this into 2.x, users will then be able to prepare their configuration for 3.x (it may even be possible to delare a deprecation when there is the 'loader' property on the tag)
2. Prepare a merge for 3.x where I fully remove the 'loader' property handling and use a ServiceLocator to wire them directly in the FilterManager / DataManager

[dbu]

i would not forbid to use services and custom names. that will allow users to register multiple loaders from the same class with different configuration, so there is still use for it.

we can still add it in 2.x as this is just added functionality. can you change the pull request to target the 2.x branch?

[homersimpsons]

i would not forbid to use services and custom names. that will allow users to register multiple loaders from the same class with different configuration, so there is still use for it.

To me, this can be handled with a custom service declaration like

app.my_filter_1:
  class: App\Filters\MyFilter
  arguments:
    - '1'
app.my_filter_2:
  class: App\Filters\MyFilter
  arguments:
    - '2'

we can still add it in 2.x as this is just added functionality. can you change the pull request to target the 2.x branch?

~I just did change the base branch but it looks like I had rebase it, I'm going to "rebase" on 2.x~ DONE

PR message is updated @dbu I guess everything is okay now.

[dbu]

i changed the mock building to not create a partial mock but a complete mock. with modern phpunit, it automatically returns a mock of the ChildDefinition as that is what the mocked class declares must be returned. for PHP 7, i had to add explicitly that the method should return a mock ChildDefinition.

the changes are in https://github.com/liip/LiipImagineBundle/pull/1561/commits/9813c8f5238f49a8849350de5ea0ae85d49c4001

i will close this branch and merge the other.