[HttpKernel] Ignore autowiring errors on controllers unless argument's type is an interface #46275
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
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... |
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. 🤞 |
|
Replaced by #46279 |
nicolas-grekas
added a commit
that referenced
this pull request
May 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 #46275 While 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 a https://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 request
May 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 | - Replaces symfony/symfony#46275 While 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 a https://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 request
Jun 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 | - Replaces symfony/symfony#46275 While 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 a https://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 request
Jun 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 | - Replaces symfony/symfony#46275 While 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 a https://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 request
Jul 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 | - Replaces symfony/symfony#46275 While 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 a https://blackfire.io comparison:  Commits ------- 739789f384 [DependencyInjection] Optimize autowiring logic by telling it about excluded symbols
Sign up for free
to 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 with a 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 references an 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.)