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 6919ef5 on homersimpsons:patch-1 into 747966c on liip:2.x.

[dbu]

thanks, good idea! can you please adjust Resources/doc/filters.rst accordingly to first explain things without specifying the loader attribute in the tag and using service name in the example, and then add a block explaining that you can change the loader name from service name to a specific name by using loader?

the data loaders use the same pattern. i think it would make sense to also adjust for the liip_imagine.binary.loader tag to have consistent behaviour, and adjust Resources/doc/data-loaders-custom.rst

can you please do that?

[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]

thanks a lot!

right, functionality wise we could do without custom names at all. but i think its nice to still allow them.

[dbu]

this is only available since 5.3 - can you check the symfony version before calling it?

the 5.4 build and lowest version builds fail - not sure if they install a too old symfony or if there is something else as well that we need to do for the test to work on legacy versions.

[dbu]

i'd also be ok to only do it with symfony 6 or 7 and otherwise skip.

the doc should mention from what symfony version on this works.

[homersimpsons]

I did update the code to check if the function exists and updated the doc too.

[homersimpsons]

Well actually it looks like this was introduced a while ago looking at this commit symfony/symfony@18627bf

But here it is returning null which is rather strange as the first implementation always return a Symfony\Component\DependencyInjection\ChildDefinition

[homersimpsons]

Oh okay, this is because we are using a MockObject in place of a real container in the test.

[homersimpsons]

I did revert my changes and add the method to the mock which should fix the tests.

Note that the setMethods mock method is removed from PHPUnit sebastianbergmann/phpunit#3687 (comment). Also I'm wondering why we have to create such mocks at first I think we can rely on the ContainerBuilder directly (like in https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php)

[dbu]

in #1558 i kill the createObjectMock method. this might get "interesting" when merging 2.x to 3.x.

we could also make it a normal mock and expect the registerForAutoConfiguration method to be called and return either a mock definition or an empty real definition (in the test that actually uses this). then we can even expect $this->exactly(3) to be sure the passes are registered.

[homersimpsons]

I'm not used to mock things in test. Can I have some guidance ? I'm not exactly sure how to fix the issue.

I could re-target 3.x but I think those changes are worth in 2.x.

[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 9813c8f

i will close this branch and merge the other.