Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] Ignore autowiring errors on controllers unless argument's type is an interface#46275
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
Closed
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
…s type is an interface
Member
chalasr commentedMay 5, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Could we improve the "default" error message to make it hint about more potential causes?
Is it really worth the DX loss then? There is still quite a lot of non-abstracted services in userland... |
MemberAuthor
nicolas-grekas commentedMay 6, 2022
I now measure a 2% perf boost after fixing#46276
An error about a missing argument when calling a controller will still tell about its type. If it's a userland class, then I suppose ppl will be able to figure out their own stuff without much trouble.
I'm not sure: the error message will tell about argument resolving that failed, and this part knows nothing about services/autowiring. But let me try another approach before going this way. There might be a lower-level one to short-circuit this heavy logic in AutowirePass. 🤞 |
MemberAuthor
nicolas-grekas commentedMay 6, 2022
Replaced by#46279 |
nicolas-grekas added a commit that referenced this pull requestMay 27, 2022
…ling it about excluded symbols (nicolas-grekas)This PR was merged into the 6.2 branch.Discussion----------[DependencyInjection] Optimize autowiring logic by telling it about excluded symbols| Q | A| ------------- | ---| Branch? | 6.1| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Replaces#46275While inspecting the dumped container of the demo app, I noticed that it contains so-called "errored" services. They come e.g from entities that are used as type-hints on controllers. To compute those services, the autowiring logic currently loads all services looking for possible type-compatible candidates.But here, we should know that those won't be found: they're excluded from being loaded.Instead of completely ignoring excluded classes, this PR still registers them, but as abstract and with a special `container.excluded` tag.This allows the rest of the compilation logic to skip+remove them, while allowing `AutowiringPass` to compute a better error message and way faster.Here is the relevant part of ahttps://blackfire.io comparison:Commits-------739789f [DependencyInjection] Optimize autowiring logic by telling it about excluded symbols
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestMay 31, 2022
…ling it about excluded symbols (nicolas-grekas)This PR was merged into the 6.2 branch.Discussion----------[DependencyInjection] Optimize autowiring logic by telling it about excluded symbols| Q | A| ------------- | ---| Branch? | 6.1| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Replacessymfony/symfony#46275While inspecting the dumped container of the demo app, I noticed that it contains so-called "errored" services. They come e.g from entities that are used as type-hints on controllers. To compute those services, the autowiring logic currently loads all services looking for possible type-compatible candidates.But here, we should know that those won't be found: they're excluded from being loaded.Instead of completely ignoring excluded classes, this PR still registers them, but as abstract and with a special `container.excluded` tag.This allows the rest of the compilation logic to skip+remove them, while allowing `AutowiringPass` to compute a better error message and way faster.Here is the relevant part of ahttps://blackfire.io comparison:Commits-------739789f384 [DependencyInjection] Optimize autowiring logic by telling it about excluded symbols
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull requestJun 6, 2022
…ling it about excluded symbols (nicolas-grekas)This PR was merged into the 6.2 branch.Discussion----------[DependencyInjection] Optimize autowiring logic by telling it about excluded symbols| Q | A| ------------- | ---| Branch? | 6.1| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Replacessymfony/symfony#46275While inspecting the dumped container of the demo app, I noticed that it contains so-called "errored" services. They come e.g from entities that are used as type-hints on controllers. To compute those services, the autowiring logic currently loads all services looking for possible type-compatible candidates.But here, we should know that those won't be found: they're excluded from being loaded.Instead of completely ignoring excluded classes, this PR still registers them, but as abstract and with a special `container.excluded` tag.This allows the rest of the compilation logic to skip+remove them, while allowing `AutowiringPass` to compute a better error message and way faster.Here is the relevant part of ahttps://blackfire.io comparison:Commits-------739789f384 [DependencyInjection] Optimize autowiring logic by telling it about excluded symbols
symfony-splitter pushed a commit to symfony/dependency-injection that referenced this pull requestJun 7, 2022
…ling it about excluded symbols (nicolas-grekas)This PR was merged into the 6.2 branch.Discussion----------[DependencyInjection] Optimize autowiring logic by telling it about excluded symbols| Q | A| ------------- | ---| Branch? | 6.1| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Replacessymfony/symfony#46275While inspecting the dumped container of the demo app, I noticed that it contains so-called "errored" services. They come e.g from entities that are used as type-hints on controllers. To compute those services, the autowiring logic currently loads all services looking for possible type-compatible candidates.But here, we should know that those won't be found: they're excluded from being loaded.Instead of completely ignoring excluded classes, this PR still registers them, but as abstract and with a special `container.excluded` tag.This allows the rest of the compilation logic to skip+remove them, while allowing `AutowiringPass` to compute a better error message and way faster.Here is the relevant part of ahttps://blackfire.io comparison:Commits-------739789f384 [DependencyInjection] Optimize autowiring logic by telling it about excluded symbols
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestJul 28, 2023
…ling it about excluded symbols (nicolas-grekas)This PR was merged into the 6.2 branch.Discussion----------[DependencyInjection] Optimize autowiring logic by telling it about excluded symbols| Q | A| ------------- | ---| Branch? | 6.1| Bug fix? | no| New feature? | yes| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Replacessymfony/symfony#46275While inspecting the dumped container of the demo app, I noticed that it contains so-called "errored" services. They come e.g from entities that are used as type-hints on controllers. To compute those services, the autowiring logic currently loads all services looking for possible type-compatible candidates.But here, we should know that those won't be found: they're excluded from being loaded.Instead of completely ignoring excluded classes, this PR still registers them, but as abstract and with a special `container.excluded` tag.This allows the rest of the compilation logic to skip+remove them, while allowing `AutowiringPass` to compute a better error message and way faster.Here is the relevant part of ahttps://blackfire.io comparison:Commits-------739789f384 [DependencyInjection] Optimize autowiring logic by telling it about excluded symbols
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While inspecting the dumped container of the demo app, I noticed that it contains so-called "errored" services.
Those come from controllers' arguments: when computing the service locator for the argument resolver, entities found as type hints are obviously not found by the autowiring logic since they're not services.
When this happens, we register an errored service in case this entity-class is fetched from the service argument resolver. The issue is that in order to compute the related error messages (to be maybe thrown at runtime), the autowiring logic needs to inspect all services for alternatives. This is a heavy process that is useless in this situation.
This PR proposes to simply skip computing those error messages when an argument is type-hinted witha class. In practice, this means that some error messages might become less informative. To alleviate this drawback, we keep computing those error messages in case an argument referencesan interface.
On the demo app, this removes any errored definitions from the dumped container. This should improve the dumping compilation time a bit (not measurable on the demo app though.)