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

Projects
None yet
6 participants
@przemyslaw-bogusz
Copy link
Contributor

commented Jan 21, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

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
@GuilhemN
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

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 przemyslaw-bogusz force-pushed the przemyslaw-bogusz:ticket_28326 branch from 85a28f1 to 3bbf849 Jan 27, 2019

@przemyslaw-bogusz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

@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.

@nicolas-grekas
Copy link
Member

left a comment

cannot test right now,here is a very light review

@nicolas-grekas nicolas-grekas force-pushed the przemyslaw-bogusz:ticket_28326 branch from 3bbf849 to 5cf6a3f Apr 12, 2019

@nicolas-grekas nicolas-grekas force-pushed the przemyslaw-bogusz:ticket_28326 branch from 5cf6a3f to 1ad8e81 Apr 12, 2019

@nicolas-grekas nicolas-grekas force-pushed the przemyslaw-bogusz:ticket_28326 branch from 1ad8e81 to 7e805ea Apr 12, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Thank you @przemyslaw-bogusz.

@nicolas-grekas nicolas-grekas merged commit 7e805ea into symfony:3.4 Apr 12, 2019

0 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

nicolas-grekas added a commit that referenced this pull request Apr 12, 2019

bug #29944 [DI] Overriding services autowired by name under _defaults…
… 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 przemyslaw-bogusz:ticket_28326 branch Apr 13, 2019

@fabpot fabpot referenced this pull request Apr 16, 2019

Merged

Release v3.4.25 #31123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.