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] Fix bad error message for unused bind under _defaults#29935

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

Conversation

@przemyslaw-bogusz
Copy link
Contributor

@przemyslaw-boguszprzemyslaw-bogusz commentedJan 19, 2019•
edited
Loading

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

Sidenote: I originally included the fix in#29897, but I decided to close the previous PR and divide it into two separate PRs for clarity.

Description:
With this fix, the message regarding an unused bind will have a clear information about the type of the bind (defined under _defaults, _instanceof orper service), as well as the name of the file, in which it was configurated. It's for, both, YAML and XML configurations.

For the core team, please note, that the fix assumes a possibility of definings binds under _instanceof, which was introduced in#27806. But since fixes are merged into other branches, I thought that it might be necessary to inlude this possibility. If this case requires making separate fixes for different branches, I will gladly do it.

sstok and vudaltsov reacted with hooray emoji
@przemyslaw-boguszprzemyslaw-boguszforce-pushed theticket_27828 branch 3 times, most recently from328bbfb toffbdd40CompareJanuary 20, 2019 14:41
@nicolas-grekasnicolas-grekas changed the title[DI] Fixes: #27828 - Bad error message for unused bind under _defaults[DI] Fix bad error message for unused bind under _defaultsJan 24, 2019
@nicolas-grekasnicolas-grekas added this to the3.4 milestoneJan 24, 2019
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.

Looks nice!
That should target master to me, it's more an improvement than a bug fix.
Tests fail because composer.json files need to bump the minimum supported version of di, for http-kernel.

@sstok
Copy link
Contributor

Can this also be added for the PHP DSL loader? Really nice addition!

@przemyslaw-bogusz
Copy link
ContributorAuthor

Thanks for the positive feedback!

@sstok
I added the same functionality to PHP DSL loader.

@nicolas-grekas

  1. Changed target to master.
  2. Modified the error message so that it refers to argument's name, type or both - depending on the configuration.

@przemyslaw-bogusz
Copy link
ContributorAuthor

@nicolas-grekas

Tests fail because composer.json files need to bump the minimum supported version of di, for http-kernel.

Does it mean I have to make changes in composer.json in my repository? It seems to be identical with the one from Symfony's repository?

@fabpot
Copy link
Member

@nicolas-grekas friendly ping

@weaverryanweaverryan modified the milestones:3.4,nextApr 3, 2019
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.

Looks nice, sorry for the late review, here are some comments.

@weaverryan
Copy link
Member

Ping@przemyslaw-bogusz! Will you have time to check out the tweaks? We can hopefully get this into 4.3! :)

@przemyslaw-bogusz
Copy link
ContributorAuthor

I will do it tomorrow, so I believe we can close it by the end of the weekend. Hope that's good enough.

@weaverryan
Copy link
Member

It's great :). Thank you for your work!

@Simperfit
Copy link
Contributor

@przemyslaw-bogusz do you have time to finish this or do you want me to take the rest of it ?

@przemyslaw-boguszprzemyslaw-boguszforce-pushed theticket_27828 branch 2 times, most recently from627c437 to531be07CompareApril 6, 2019 22:17
@przemyslaw-bogusz
Copy link
ContributorAuthor

@nicolas-grekas,@weaverryan Thank you for your patience. If you have any additional comments, please let me know. I hope we will also be able to close#29944.

@nicolas-grekas
Copy link
Member

Thank you@przemyslaw-bogusz.

@nicolas-grekasnicolas-grekas merged commit35bf420 intosymfony:masterApr 7, 2019
nicolas-grekas added a commit that referenced this pull requestApr 7, 2019
…ults (przemyslaw-bogusz)This PR was merged into the 4.3-dev branch.Discussion----------[DI] Fix bad error message for unused bind under _defaults| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#27828| License       | MIT**Sidenote**: I originally included the fix in#29897, but I decided to close the previous PR and divide it into two separate PRs for clarity.**Description:**With this fix, the message regarding an unused bind will have a clear information about the type of the bind (defined under __defaults_, __instanceof_ or _per service_), as well as the name of the file, in which it was configurated. It's for, both, YAML and XML configurations.For the core team, please note, that the fix assumes a possibility of definings binds under __instanceof_, which was introduced in#27806. But since fixes are merged into other branches, I thought that it might be necessary to inlude this possibility. If this case requires making separate fixes for different branches, I will gladly do it.Commits-------35bf420 [DI] Fix bad error message for unused bind under _defaults
@przemyslaw-boguszprzemyslaw-bogusz deleted the ticket_27828 branchApril 8, 2019 04:47
@symfonysymfony deleted a comment fromnicolas-grekasApr 8, 2019
@weaverryan
Copy link
Member

Congrats! Thank you SO much@przemyslaw-bogusz!!

@scott-r-lindsey
Copy link

Workaround until 4.2.6: make sure that at least one service consumes each dependency mentioned in _defaults / bind

@weaverryan
Copy link
Member

That will still be the case after this PR. This is just to correct the error message, which was misleading. I think#29944 might be what you’re looking for.

@przemyslaw-bogusz
Copy link
ContributorAuthor

@scott-r-lindsey I am not sure, if I understand you correctly, but the whole purpose ofResolveBindingsPass is to inform you of any unused bindings - the ones that you define, but do not use/consume in any of your services. You simply cannot have a surplus of bindings.

@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
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

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

8 participants

@przemyslaw-bogusz@sstok@fabpot@weaverryan@Simperfit@nicolas-grekas@scott-r-lindsey@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp