Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DependencyInjection] Added "ignore" option to "_instanceof" definitions #35658

Closed
wants to merge 11 commits into from

Conversation

@tsantos84
Copy link
Contributor

tsantos84 commented Feb 9, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR awaiting the approval of this PR

As discussed on #35581, this PR aims to add the capability of ignore some implementations when configuring definitions with _instanceof option.

Problem

The Dependency Injection component has a convenient way to create default configuration for services that implementing some interface or extending some classe. Although we can add those default configurations, we can't skip some implementations in this process.

Consider the following PHP classes and interface:

interface FooInterface {}
class Foo implements FooInterface {}
class Bar implements FooInterface {}
class Baz implements FooInterface {}

If you want, for example, to not have Baz service configured from _instanceof configuration, you can change your service files like this:

YAML file:

services:
    _instanceof:
        FooInterface:
            tags: [{name:"foo"}]
            ignore:
                - Baz
            # or simple string for only one ignored implementation
            ignore: 'Baz'
    Foo: ~
    Bar: ~
    Baz:
        tags: [{name:"app.baz"}]

XML file

<?xml version="1.0" encoding="utf-8"?>
<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd">
    <services>
        <instanceof id="FooInterface">
            <tag name="foo" />
            <ignore name="Baz" />
        </instanceof>
        <service id="Foo" />
        <service id="Bar" />
        <service id="Baz">
            <tag name="app.baz" />
        </service>
    </services>
</container>

Remarks

  • In the original issue, was proposed to add an exclude option but personally I think that ignore is more concise for this feature, although the component has already the option exclude to skip files and directories when configuring resources.
  • Ignoring a implementation is valid per file only.
@tsantos84 tsantos84 marked this pull request as ready for review Feb 9, 2020
@tsantos84 tsantos84 changed the title Added "ignore" option to "_instanceof" definitions [DependencyInjection] Added "ignore" option to "_instanceof" definitions Feb 9, 2020
@nicolas-grekas nicolas-grekas added this to the next milestone Feb 10, 2020
Copy link
Member

nicolas-grekas left a comment

thanks, here are some comments on the implementation
don't miss patching InstanceofConfigurator also

@tsantos84 tsantos84 force-pushed the tsantos84:di-instanceof-skip-class branch 2 times, most recently from e47fb8f to fbb5343 Feb 10, 2020
Copy link
Member

nicolas-grekas left a comment

The implementation looks good to me.
Feature-wise, this is an edge case, but I'm no objections against supporting it - this has already been reported before.

@nicolas-grekas nicolas-grekas dismissed their stale review Feb 10, 2020

I forgot InstanceofConfigurator needs a patch too.

Copy link
Member

nicolas-grekas left a comment

I just realized that this implementation ignores by "id" and not by "fqcn".
Maybe that's for the best, but this needs to be a conscious decision and the wording should be updated accordingly.
About "ignore" vs "exclude", I'm not sure about "ignore" anymore: we started the discussion with "exclude", which could mean that's the most expected word. For thoughts.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Feb 16, 2020

-1 as there is a clear solution with separate files (#35581 (comment)) which does not need documentation or explanation what ignore supports and what it doesn't.

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

tsantos84 commented Feb 17, 2020

@nicolas-grekas

About "ignore" vs "exclude", I'm not sure about "ignore" anymore: we started the discussion with "exclude", which could mean that's the most expected word. For thoughts.

Maybe "exclude" is a better term because devs already knows what "exclude" means in this context.

@Tobion

-1 as there is a clear solution with separate files (#35581 (comment)) which does not need documentation or explanation what ignore supports and what it doesn't.

I agree with you that it'll work. I've just create a demo application to see how easy it'd be to accomplish the same feature by creating two separated files. The order of loading the file matters. Depending on how I load the new file in Kernel (before or after "services.yaml") and configure those services, the "exclude" feature will not work. So, this is not so easy to understand, mainly for newcomers.

Example with two separated files:

#services.yaml
services:
    _instanceof:
        App\Foo\FooInterface:
            tags: [{name: "app.foo"}]

    App\Foo\FooInterface: '@App\Foo\FooComposite'

#services_foo.yaml
services:
    App\Foo\FooComposite:
        arguments: [!tagged_iterator "app.foo"]

With these files, I'm obligated to load services_foo.yaml after services.yaml. Another point is that creating two separated files to implement a simple design pattern is not straightforward, IHMO.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Feb 17, 2020

With these files, I'm obligated to load services_composite.yaml after services.yaml.

What's the reason?

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

tsantos84 commented Feb 17, 2020

What's the reason?

The following error is raised otherwise:

Cannot autowire service "App\Foo\FooComposite": argument "$foos" of method "__construct()" is type-hinted "iterable", you should configure its value explicitly.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Feb 17, 2020

Then you probably still load FooComposite inside services.yaml which overwrites services_composite.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 17, 2020

I think this discussion is a good justification to add the feature :)

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Feb 17, 2020

The service file is just not completely given. So it's impossible to tell what they are doing wrong.
If you start adding exlusion rules for certain rules, then you need to add exclusions for all of them. What about bind etc? They all have the same logic: It applies to services loaded/defined from this file.

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

tsantos84 commented Feb 17, 2020

The service file is just not completely given. So it's impossible to tell what they are doing wrong.

I've just pushed the demo application on https://github.com/tsantos84/di-composite-example. The important files are:

  1. config/services.yaml
  2. config/services_foo.yaml
  3. src/Foo/*
  4. src/Controller/

If you start adding exlusion rules for certain rules, then you need to add exclusions for all of them. What about bind etc? They all have the same logic: It applies to services loaded/defined from this file.

I believe when a tool allows me to configure something in batch mode (_instanceof in this case), then I expect that tool gives me the possibility to "exclude/ignore" part of the content being handled. _defaults, bind and etc are very straightforward because they can be easily redefined on a specific service definition, and this is not the case for _instanceof feature.

@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Feb 17, 2020

Thanks for the demo.

https://github.com/tsantos84/di-composite-example/blob/495a21a67ab8b925720dc3cbf9dc455a089d3401/config/services.yaml#L16-L18
includes the FooComposite. That means it tries to autowire it and it tries to apply _instanceof on it. You need to exclude it as I already assummed

Then you probably still load FooComposite inside services.yaml which overwrites services_composite.

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

tsantos84 commented Feb 17, 2020

includes the FooComposite. That means it tries to autowire it and it tries to apply _instanceof on it. You need to exclude it as I already assummed

Finally got the final version to get the solution done using two separated files:

#services.yaml
services:
  _instanceof:
    App\Foo\FooInterface:
      tags: [{name: "app.foo"}]

  App\:
    resource: '../src/*'
    exclude: '../src/{DependencyInjection,Entity,Migrations,Tests,Kernel.php,Foo/FooComposite.php}'

#services_foo.yaml
services:
  App\Foo\FooComposite:
    arguments: [!tagged_iterator "app.foo"]

  App\Foo\FooInterface: '@App\Foo\FooComposite'

As I told before, it will work splitting the definition in two separated files, but is not so simple to accomplish as we need to exclude totally FooComposite definition from services.yaml and define it on its own yaml file (and make reference to tags defined in services.yaml file). I keep my vote to have the "exclude" option. 😄

@tsantos84

This comment has been minimized.

Copy link
Contributor Author

tsantos84 commented Feb 18, 2020

After discussing this proposal with @Tobion here and @nicolas-grekas on Slack, I prefer to stop this contribution as there are few use cases that will be impacted with this solution even they exist. If some day more people ask for this, I think it will be reconsidered to make part of base code. Until that, solving the problem on dev side would be a better solution. Thanks @Tobion and @nicolas-grekas for the rich discussion on this topic.

@tsantos84 tsantos84 closed this Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.