-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Introduce pinnable value resolvers with #[ValueResolver]
and #[AsPinnedValueResolver]
#48992
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
#[ValueResolver]
for specifying a parameter argume…#[ValueResolver]
for specifying a parameter argument resolver
998e944
to
8b7f9b0
Compare
#[ValueResolver]
for specifying a parameter argument resolver#[ValueResolver]
for specifying an argument resolver
8b7f9b0
to
f8c3d5c
Compare
#[ValueResolver]
for specifying an argument resolver#[ValueResolver]
for specifying a controller argument resolver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature makes sense to me
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
Outdated
Show resolved
Hide resolved
539c344
to
3ddc732
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For param converters we have this behavior:
You can register a converter by priority, by name (attribute "converter"), or both. If you don't specify a priority or a name, the converter will be added to the converter stack with a priority of 0. To explicitly disable the registration by priority you have to set priority="false" in your tag definition.
Do we want a way to not register a value resolver in the iterator and make it accessible only via explicit attribute? That's require accepting both an iterator and a locator on the constructor.
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
Outdated
Show resolved
Hide resolved
Thanks for the review!
From what I understood, we only want a way to force a specific resolver to be used for an argument.
Maybe we could do the same? This would require to name existing resolvers. |
165a0e2
to
e1cebd9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my review as a diff proposal:
diff --git a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
index 3ce8519c26..ab318d1334 100644
--- a/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
+++ b/src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
@@ -11,7 +11,6 @@
namespace Symfony\Component\HttpKernel\Controller;
-use Symfony\Component\DependencyInjection\ServiceLocator;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Attribute\ValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\DefaultValueResolver;
@@ -31,34 +30,39 @@ use Symfony\Contracts\Service\ServiceProviderInterface;
*/
final class ArgumentResolver implements ArgumentResolverInterface
{
- private ArgumentMetadataFactoryInterface $argumentMetadataFactory;
- private ServiceProviderInterface $argumentValueResolvers;
-
/**
* @param iterable<mixed, ArgumentValueResolverInterface|ValueResolverInterface>|ServiceProviderInterface<ValueResolverInterface> $argumentValueResolvers
*/
- public function __construct(ArgumentMetadataFactoryInterface $argumentMetadataFactory = null, iterable|ServiceProviderInterface $argumentValueResolvers = [])
- {
- $this->argumentMetadataFactory = $argumentMetadataFactory ?? new ArgumentMetadataFactory();
- $this->argumentValueResolvers = $argumentValueResolvers instanceof ServiceProviderInterface
- ? $argumentValueResolvers
- : self::getServiceProvider($argumentValueResolvers ?: self::getDefaultArgumentValueResolvers())
- ;
+ public function __construct(
+ private ArgumentMetadataFactoryInterface|null $argumentMetadataFactory = new ArgumentMetadataFactory(),
+ private iterable|ServiceProviderInterface $argumentValueResolvers = [],
+ ) {
+ if (null === $argumentMetadataFactory) {
+ $this->argumentMetadataFactory = new ArgumentMetadataFactory();
+ trigger_deprecation('symfony/http-kernel', '6.3', 'Passing null as $argumentMetadataFactory to "%s" is deprecated. Pass a "%s" instead.', static::class, ArgumentMetadataFactory::class);
+ }
}
public function getArguments(Request $request, callable $controller, \ReflectionFunctionAbstract $reflector = null): array
{
$arguments = [];
- $resolversName = array_keys($this->argumentValueResolvers->getProvidedServices());
+ $resolversMap = null;
+ $allResolvers = $this->argumentValueResolvers ?: self::getDefaultArgumentValueResolvers();
+ $isProvider = $allResolvers instanceof ServiceProviderInterface;
foreach ($this->argumentMetadataFactory->createArgumentMetadata($controller, $reflector) as $metadata) {
- $argumentResolversName = $resolversName;
+
if ($attributes = $metadata->getAttributesOfType(ValueResolver::class)) {
- $argumentResolversName = [$attributes[0]->name];
+ $name = $attributes[0]->name;
+ $resolvers = [$name => $isProvider ? null : ($resolversMap ??= self::getResolversMap($allResolvers))[$name]];
+ } else {
+ $resolvers = $isProvider ? $allResolvers->getProvidedServices() : $allResolvers;
}
- foreach ($argumentResolversName as $resolverName) {
- $resolver = $this->argumentValueResolvers->get($resolverName);
+ foreach ($resolvers as $name => $resolver) {
+ if ($isProvider) {
+ $resolver = $allResolvers->get($name);
+ }
if ((!$resolver instanceof ValueResolverInterface || $resolver instanceof TraceableValueResolver) && !$resolver->supports($request, $metadata)) {
continue;
@@ -115,16 +119,16 @@ final class ArgumentResolver implements ArgumentResolverInterface
/**
* @param iterable<ArgumentValueResolverInterface|ValueResolverInterface> $resolvers
*
- * @return ServiceProviderInterface<ArgumentValueResolverInterface|ValueResolverInterface>
+ * @return array<string, ArgumentValueResolverInterface|ValueResolverInterface>
*/
- private static function getServiceProvider(iterable $resolvers): ServiceProviderInterface
+ private static function getResolversMap(iterable $resolvers): array
{
- $factories = [];
+ $map = [];
$i = 0;
foreach ($resolvers as $name => $resolver) {
- $factories[$name === $i++ ? $resolver::class : $name] = static fn () => $resolver;
+ $map[$name === $i++ ? $resolver::class : $name] = $resolver;
}
- return new ServiceLocator($factories);
+ return $map;
}
}
From what I understood, we only want a way to force a specific resolver to be used for an argument.
Sure, but this is unrelated to my question: do we want a way to make a value resolver accessible only via the attribute? The extra bundle allows this. Do we also want that capability? I don't have an answer yet but I'm still wondering.
Hmm the Is there a way to circumvent this behavior?
I personally never had any use-case for this so I cannot say. |
a ServiceLocator does not guarantee iteration order. To me, this means you should inject 2 different arguments:
|
An alternative way is to make this ValueResolver be processed by a ValueResolver registered first (with a very high priority) and receiving this ServiceLocator instead of making it a special feature of the ArgumentResolver. |
Thanks @stof for the leads 👍 Is there any incentive for implementing one rather than the other? |
I like @stof's idea! Less low level complexity, each class their use case. |
Okay, going with a dedicated value resolver. |
d74a72c
to
3701de7
Compare
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/AttributeValueResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/AttributeValueResolver.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpKernel/Controller/ArgumentResolver/AttributeValueResolver.php
Outdated
Show resolved
Hide resolved
…]` and `#[AsPinnedValueResolver]`
d2376aa
to
245485c
Compare
/cc @symfony/mergers I'd like to merge this to unlock some other PRs. Could you please have a look? 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼 for the feature
Thank you @MatTheCat. |
…solvers (nicolas-grekas) This PR was merged into the 6.3 branch. Discussion ---------- [HttpKernel] Renamed "pinned" to "targeted" for value resolvers | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - "pinned" was introduced in #48992 but a quick chat within the core-team lead to this proposal, which might be preferred. WDYT? Commits ------- ad58cc6 [HttpKernel] Renamed "pinned" to "targeted" for value resolvers
…rs (HypeMC) This PR was merged into the 6.3 branch. Discussion ---------- [HttpKernel] Fix default value ignored with pinned resolvers | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Since #48992 the default value is ignored when, for example, `#[MapEntity]` is used: ```php #[Route('/')] #[Route('/{someId}')] public function index(#[MapEntity(id: 'someId')] ?Post $post): Response { // ... } ``` Before, `$post` would be `null` when making a request to `/`, now an exception is thrown: ``` Controller "App\Controller\TestController::index" requires that you provide a value for the "$post" argument. Either the argument is nullable and no null value has been provided, no default value has been provided or there is a non-optional argument after this one. ``` Since I can't think of a valid case when one would want to ignore the default value, I'd suggest always adding the `DefaultValueResolver` to the list when a pinned resolver is used. Commits ------- fabe7bc [HttpKernel] Fix default value ignored with pinned resolvers
…edValueResolver]` (Mathieu, MatTheCat) This PR was merged into the 6.3 branch. Discussion ---------- [HttpKernel] Document `#[ValueResolver]` and `#[AsTargetedValueResolver]` cf. symfony/symfony#48992 Commits ------- b57df54 Reword disabled resolvers paragraph 432c2eb Remove “pin” usage remnants ac13dfc Update “Managing Value Resolvers” section 31edf49 Fix code samples 1009ca2 Do not hardcode priorities 50450a4 Address #17763 (comment) 7fc8065 Document symfony/symfony#48992
Introducing a new
ValueResolver
attribute, which allows toEvery existing resolver-related attribute (
MapEntity
,CurrentUser
…) now extendsValueResolver
.Each
controller.argument_value_resolver
tag is added aname
attribute, which is the resolver’s FQCN. This is the first argumentValueResolver
expects.A new
AsPinnedValueResolver
attribute is added for autoconfiguration, adding thecontroller.pinned_value_resolver
tag. Such resolvers can only be “pinned”, meaning they won’t ever be called for an argument missing theValueResolver
attribute.