[HttpKernel] Replace ArgumentValueResolverInterface by ValueResolverInterface #47363
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
ab4880f to
d1e236a
Compare
|
I really like this move 👍🏼 |
d1e236a to
2f90368
Compare
stof
reviewed
Aug 23, 2022
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
Outdated
Show resolved
Hide resolved
2f90368 to
e9663cc
Compare
stof
approved these changes
Aug 23, 2022
chalasr
approved these changes
Aug 24, 2022
|
Thank you @nicolas-grekas. |
Merged
ghost
reviewed
Nov 21, 2022
| @@ -18,6 +18,8 @@ | |||
| * Responsible for resolving the value of an argument based on its metadata. | |||
| * | |||
| * @author Iltar van der Berg <kjarli@gmail.com> | |||
There was a problem hiding this comment.
Maybe a bit late cr seeing the code is already merged but shouldn't we have some sort off:
trigger_deprecation('symfony/http-kernel', '6.2', 'The "%s" interface is deprecated.', ValueResolverInterface::class);
to inform the user on code execution @nicolas-grekas ?
There was a problem hiding this comment.
DebugClassLoader will trigger it when you implement it, which is much better than triggering it when it is autoloaded (core classes still need to implement it for BC)
There was a problem hiding this comment.
Cool is this the case for all interfaces that are deprecated when you are running debug mode?
javiereguiluz
added a commit
to EasyCorp/EasyAdminBundle
that referenced
this pull request
Nov 25, 2022
…luz) This PR was squashed before being merged into the 4.x branch. Discussion ---------- Add compatibility with ValueResolverInterface This avoids a deprecation when upgrading to Symfony 6.2. See symfony/symfony#47363 Commits ------- 497b39c Add compatibility with ValueResolverInterface
derrabus
added a commit
to symfony/psr-http-message-bridge
that referenced
this pull request
Jul 25, 2023
This PR was merged into the 2.2-dev branch. Discussion ---------- Implement ValueResolverInterface Follows symfony/symfony#47363 Commits ------- 0b54b85 Implement ValueResolverInterface
javiereguiluz
added a commit
to EasyCorp/EasyAdminBundle
that referenced
this pull request
Jan 18, 2025
This PR was merged into the 5.x branch. Discussion ---------- Simplified the BatchActionDto value resolver The `ValueResolverInterface` was introduced in Symfony 6.2 (see symfony/symfony#47363), so we don't need to take care of the previous interface. Commits ------- 1d69f8d Simplified the BatchActionDto value resolver
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.
Now that I looked at argument value resolvers more closely, I think that we can improve them a bit.
The issue I spotted is that by having two methods (
supports()andresolve()), we 1. require some correlation between both that isn't enforced by any contracts, 2. force some duplicate logic between the two and 3. as a consequence incur a needless perf overhead on the hotpath.I hereby propose to remove the
supports()method and allowresolve()to return the empty array instead, to signal that an argument is not supported by a specific value resolver.For perf also, I made all
resolve()methods return arrays instead of generators, which are overhead in our cases.The changes to
EntityValueResolverTestare a very good example at where the current design is fragile. Nothing prevents us from having de-correlatedsupports()andresolve()methods right now, and we felt into that trap.