Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 2 commits intosymfony:3.4fromprzemyslaw-bogusz:ticket_28326
Apr 12, 2019

Conversation

@przemyslaw-bogusz
Copy link
Contributor

QA
Branch?3.4
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#28326
LicenseMIT

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

@GuilhemNGuilhemN 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 thePhpFileLoader which doesn't callFileDefinition::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-grekasnicolas-grekas added this to the3.4 milestoneJan 24, 2019
@nicolas-grekasnicolas-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 workingJan 25, 2019
@przemyslaw-boguszprzemyslaw-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 workingJan 27, 2019
@przemyslaw-bogusz
Copy link
ContributorAuthor

@GuilhemN Thanks for the suggestion. I added the funcionality forPhpFileLoader, and as a consequence I moved code fromFileLoader intoContainerBuilder 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-grekasnicolas-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-grekasnicolas-grekas merged commit7e805ea intosymfony:3.4Apr 12, 2019
nicolas-grekas added a commit that referenced this pull requestApr 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       | MITThis is an implementation of ideas and suggestions of@nicolas-grekas and@GuilhemN.Commits-------7e805ea more tests35a40ac [DI]Fixes:#28326 - Overriding services autowired by name under _defaults bind not working
@przemyslaw-boguszprzemyslaw-bogusz deleted the ticket_28326 branchApril 13, 2019 14:30
@fabpotfabpot mentioned this pull requestApr 16, 2019
@fabpotfabpot mentioned this pull requestApr 16, 2019
nicolas-grekas added a commit that referenced this pull requestApr 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     <!-- seehttps://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 freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@OskarStarkOskarStarkOskarStark left review comments

+2 more reviewers

@teohhanhuiteohhanhuiteohhanhui left review comments

@GuilhemNGuilhemNGuilhemN left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

6 participants

@przemyslaw-bogusz@OskarStark@nicolas-grekas@teohhanhui@GuilhemN@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp