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.

Sign up for GitHub

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

Jump to bottom

[DI] Fix bad error message for unused bind under _defaults #29935

Merged
merged 1 commit into from Apr 7, 2019
Merged

[DI] Fix bad error message for unused bind under _defaults #29935

merged 1 commit into from Apr 7, 2019

Conversation

przemyslaw-bogusz
Copy link
Contributor

@przemyslaw-bogusz przemyslaw-bogusz commented Jan 19, 2019 •
edited

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.

@przemyslaw-bogusz przemyslaw-bogusz mentioned this pull request Jan 19, 2019
Closed
@przemyslaw-bogusz przemyslaw-bogusz force-pushed the ticket_27828 branch 3 times, most recently from 328bbfb to ffbdd40 Compare January 20, 2019 14:41
@przemyslaw-bogusz przemyslaw-bogusz mentioned this pull request Jan 21, 2019
Merged
@nicolas-grekas nicolas-grekas changed the title [DI] Fixes: #27828 - Bad error message for unused bind under _defaults [DI] Fix bad error message for unused bind under _defaults Jan 24, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jan 24, 2019
nicolas-grekas
nicolas-grekas reviewed Jan 24, 2019
View reviewed changes
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.

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.

src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php Outdated Show resolved Hide resolved
@sstok
Copy link
Contributor

sstok commented Jan 25, 2019

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

@przemyslaw-bogusz przemyslaw-bogusz changed the base branch from 3.4 to master January 26, 2019 23:37
@przemyslaw-bogusz
Copy link
Contributor Author

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
Contributor Author

@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

fabpot commented Mar 4, 2019

@nicolas-grekas friendly ping

@weaverryan weaverryan modified the milestones: 3.4, next Apr 3, 2019
nicolas-grekas
nicolas-grekas reviewed Apr 3, 2019
View reviewed changes
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.

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

src/Symfony/Component/DependencyInjection/Argument/BoundArgument.php Outdated Show resolved Hide resolved
src/Symfony/Component/DependencyInjection/Argument/BoundArgument.php Outdated Show resolved Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php Outdated Show resolved Hide resolved
src/Symfony/Component/DependencyInjection/Loader/Configurator/ServicesConfigurator.php Outdated Show resolved Hide resolved
src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpKernel/DependencyInjection/RegisterControllerArgumentLocatorsPass.php Outdated Show resolved Hide resolved
@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
Contributor Author

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-bogusz przemyslaw-bogusz force-pushed the ticket_27828 branch 2 times, most recently from 627c437 to 531be07 Compare April 6, 2019 22:17
@przemyslaw-bogusz
Copy link
Contributor Author

@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-grekas nicolas-grekas merged commit 35bf420 into symfony:master Apr 7, 2019
nicolas-grekas added a commit that referenced this pull request Apr 7, 2019
@nicolas-grekas
feature #29935 [DI] Fix bad error message for unused bind under _defa…
b0989aa 
…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-bogusz przemyslaw-bogusz deleted the ticket_27828 branch April 8, 2019 04:47
@symfony symfony deleted a comment from nicolas-grekas Apr 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
Contributor Author

@scott-r-lindsey I am not sure, if I understand you correctly, but the whole purpose of ResolveBindingsPass 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-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fabpot fabpot mentioned this pull request May 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

Assignees
No one assigned
Labels
Bug DependencyInjection Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@przemyslaw-bogusz @sstok @fabpot @weaverryan @Simperfit @nicolas-grekas @scott-r-lindsey @carsonbot