Skip to content

[DependencyInjection] Fix bindings and tagged_locator #32722

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

Merged
merged 1 commit into from
Jul 30, 2019

Conversation

deguif
Copy link
Contributor

@deguif deguif commented Jul 24, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Working tagged_locator

App\XXX:
    arguments:
        $taggedLocator: !tagged_locator { tag: 'my_tag', index_by: 'my_key' }

Not working tagged_locator

App\XXX:
    bind:
        $taggedLocator: !tagged_locator { tag: 'my_tag', index_by: 'my_key' }

Currently ResolveBindingsPass is executed after ServiceLocatorTagPass, which produces empty service locators when using bind.
I'm proposing to change the optimization passes order, so that this issue is solved.
I'm not confident with the impact, so let's discuss about it.

@deguif deguif changed the title Fix bindings and tagged_locator [Dependency Injection] Fix bindings and tagged_locator Jul 24, 2019
@deguif deguif changed the title [Dependency Injection] Fix bindings and tagged_locator [DependencyInjection] Fix bindings and tagged_locator Jul 24, 2019
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Jul 24, 2019
@deguif deguif force-pushed the fix-tagged-locator-bindings branch 2 times, most recently from d609dce to 43af679 Compare July 24, 2019 16:43
@deguif
Copy link
Contributor Author

deguif commented Jul 24, 2019

Ok, it's ready.
Finally I needed to change the CheckDefinitionValidityPass order too, as ServiceLocatorTagPass add some definition classes to service locator which haven't them yet before this pass and CheckDefinitionValidityPass checks that each definition has a class set.

@deguif deguif force-pushed the fix-tagged-locator-bindings branch from 0223e71 to bf4c713 Compare July 25, 2019 06:48
@nicolas-grekas
Copy link
Member

Thank you @deguif.

@nicolas-grekas nicolas-grekas merged commit bf4c713 into symfony:4.3 Jul 30, 2019
nicolas-grekas added a commit that referenced this pull request Jul 30, 2019
…uif)

This PR was merged into the 4.3 branch.

Discussion
----------

[DependencyInjection] Fix bindings and tagged_locator

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        |

Working `tagged_locator`

```yaml
App\XXX:
    arguments:
        $taggedLocator: !tagged_locator { tag: 'my_tag', index_by: 'my_key' }
```

Not working `tagged_locator`
```yaml
App\XXX:
    bind:
        $taggedLocator: !tagged_locator { tag: 'my_tag', index_by: 'my_key' }
```

Currently `ResolveBindingsPass` is executed after `ServiceLocatorTagPass`, which produces empty service locators when using `bind`.
I'm proposing to change the optimization passes order, so that this issue is solved.
I'm not confident with the impact, so let's discuss about it.

Commits
-------

bf4c713 Fix bindings and tagged_locator
@deguif deguif deleted the fix-tagged-locator-bindings branch July 31, 2019 12:48
@fabpot fabpot mentioned this pull request Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants