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

[DI] Overriding services autowired by name under _defaults bind not working #29944

Merged
merged 2 commits into from Apr 12, 2019

Conversation

przemyslaw-bogusz
Copy link
Contributor

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #28326
License MIT

This is an implementation of ideas and suggestions of @nicolas-grekas and @GuilhemN.

@OskarStark
Copy link
Contributor

Typo in PR headline:

- [DI] Fixes: #28326 - Overriding services autowired by name under _deaults bind not working
+ [DI] Fixes: #28326 - Overriding services autowired by name under _defaults bind not working

Copy link
Contributor

@GuilhemN GuilhemN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great 👍 I'm only concerned about the PhpFileLoader which doesn't call FileDefinition::setDefinition() (https://github.com/przemyslaw-bogusz/symfony/blob/85a28f1e4e5a6197bbc4ed0550b9ce68936c83bf/src/Symfony/Component/DependencyInjection/Loader/Configurator/ServiceConfigurator.php#L65).
Shouldn't the logic be added there too?

@nicolas-grekas
Copy link
Member

See comment in #29949: there may be an issue that makes the container load all classes at compile time. We should avoid it if confirmed.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jan 24, 2019
@nicolas-grekas nicolas-grekas changed the title [DI] Fixes: #28326 - Overriding services autowired by name under _deaults bind not working [DI] Overriding services autowired by name under _deaults bind not working Jan 25, 2019
@przemyslaw-bogusz przemyslaw-bogusz changed the title [DI] Overriding services autowired by name under _deaults bind not working [DI] Overriding services autowired by name under _defaults bind not working Jan 27, 2019
@przemyslaw-bogusz
Copy link
Contributor Author

@GuilhemN Thanks for the suggestion. I added the funcionality for PhpFileLoader, and as a consequence I moved code from FileLoader into ContainerBuilder to avoid duplication.

@nicolas-grekas This PR is rather simple so I don't think I've made a change that would cause

the container load all classes at compile time

At least, I'm not aware of that.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot test right now,here is a very light review

@nicolas-grekas
Copy link
Member

Thank you @przemyslaw-bogusz.

@nicolas-grekas nicolas-grekas merged commit 7e805ea into symfony:3.4 Apr 12, 2019
nicolas-grekas added a commit that referenced this pull request Apr 12, 2019
… bind not working (przemyslaw-bogusz, renanbr)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Overriding services autowired by name under _defaults bind not working

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28326
| License       | MIT

This is an implementation of ideas and suggestions of @nicolas-grekas and @GuilhemN.

Commits
-------

7e805ea more tests
35a40ac [DI] Fixes: #28326 - Overriding services autowired by name under _defaults bind not working
@przemyslaw-bogusz przemyslaw-bogusz deleted the ticket_28326 branch April 13, 2019 14:30
@fabpot fabpot mentioned this pull request Apr 16, 2019
@fabpot fabpot mentioned this pull request Apr 16, 2019
nicolas-grekas added a commit that referenced this pull request Apr 27, 2019
…(teohhanhui)

This PR was merged into the 3.4 branch.

Discussion
----------

Fix name and phpdoc of ContainerBuilder::removeBindings

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29944 (comment)
| License       | MIT
| Doc PR        | N/A

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

c93194d Fix name and phpdoc of ContainerBuilder::removeBindings
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.

None yet

6 participants