first, check if the definition exists and return early otherwise.

missing empty line at the end of this file

Doesn't github show this if the case? Anyhow, line 66 is an empty line here for me, adding one more would result in 2 empty lines in the end

@HeahDude we should not have an empty line at the end. We should have a final LF char. PhpStorm displays an empty line after this LF, but there is no actual line 66. Github handles this properly (as git does) and displays an extra info at the end of line when it does not end with an EOL character

@stof Noted. Thanks for that precision :)

I thought there was a kind of convention to write services used as arguments before being used as arguments. I was wrong ?

In this case I just put it under the controller_resolver because that's where it originates from code wise. I can always move it if desired but in the end it makes no difference

Agreed. It was more a question about "what does the symfony style advise in that case ?"

What about merging the last two as being the "fallback" behavior based on reflection? That could even be "hardcoded" (not sure).

You mean directly in the ArgumentResolver or in 1 specific resolver?

missing doc block ?

Was removed on purpose, I can always add it instead of the @var. As long as the array'd type can be deferred I'm happy.

You should use {@inheritdoc} to link the ArgumentResolverInterface with auto generated doc api.

same here, needed.

Edited

foreach ($this->argumentValueResolvers as $resolver) {
    if (!$resolver->supports($request, $metadata)) {
        continue;
    }

    $resolved = $resolver->getValue($request, $metadata);
    if ($resolved) {
        // variadic is a special case, always being the last and being an array
        if ($metadata->isVariadic() && is_array($resolved)) {
            return array_merge($arguments, $resolved);
        }

        return $resolved;
    }
}

$repr = $controller;

if (is_array($controller)) {
    $repr = sprintf('%s::%s()', get_class($controller[0]), $controller[1]);
} elseif (is_object($controller)) {
    $repr = get_class($controller);
}

throw new \RuntimeException(sprintf('Controller "%s" requires that you provide a value for the "$%s" argument (because there is no default value or because there is a non optional argument after this one).', $repr, $metadata->getArgumentName()));

That would cause an issue with (Request $request, $foo) where foo wouldn't exist. Because at that point it would return true while the argument couldn't be found.

I've also thought about the guard clause you've posted but this case will actually cause 2 arguments to be resolved if they overlap. That means if (Request $request) is given and 2 resolvers support this:

• Request as class
• 'request' in the attribute bag for example

this would mean you'd call the method with 2 parameters instead of 1.

Technically, we should be more flexible here, by allowing any Traversable (as we only need to loop over it using foreach. There is no reason to force consumers to use a generator when implementing the iterator (even though the core does it this way for simplicity)

This makes it more complex. If it returns an array, I should iterate over it. If it's not an array, I should make it an array and iterate over it. This last case was discussed to rather throw an exception. If you check the comments above, it was decide to use yield as an elegant alternative. This way someone can yield an array and it won't give a buggy result. If I were to return an array while that single argument would have to be an array, it would start adding each array item as one, effectively breaking fooAction(array $foo)

This is one of the cases where I think it's easier to support only 1 method which is compatible with both arrays, non-arrays and variadic arguments.

@iltar Why ? Document that the return value is a Traversable being a list of argument, and always iterate over it. The code is the same than today, except for the instanceof safeguard here. People may want to implement iterators in other ways than through a generator.
When you only need iterations, the fact that a Generator is used should be an implementation detail. You are overly restrictive here IMO.

The only case where a consumer should care that it deals with a Generator is when using Generator-only APIs (i.e. Generator::send).

I'm not asking you to be able to return a single value from the resolver here (which is what you describe in your comment)

As I've discussed this before; @fabpot do you also agree that returning Traversable|array should be possible?

In my opinion it's a bit dangerous as this should only be possible for variadics.

@iltar returning an array in this case would then work the same than when returning an iterator. Iterating over it. I don't see much difference between the case of the array and the Generator here, for the consumer side (and the resolver can be implement in the way the implementer prefers)

What we should have though is a validation in the ArgumentResolver that any non-variadic argument leads to an iterator containing exactly 1 value.

@stof as the current solution is more strict, we can always decide to change it in a later version as it will remain backwards compatible. I don't think the current situation will cause any issues and if they do, we can patch it

@iltar If we want to make that change, it must be done before the 3.1 release. Otherwise, code relying on Generator instances being returned might break when a Traversable is returned instead.

@xabbuh but we're talking about the return value that is only internally used. No other code should ever rely on what the argument value resolver returns

@wouterj If we don't expect other code to make use of the return values, we should remove the ArgumentResolverInterface. Having such an interface calls for custom implementations imo which will then probably rely on the return type.

@xabbuh in theory it shouldn't return anything else and a clear exception
is thrown when not the case. The decision for yield was that it's a simple
syntax and uniform for every case. Ideally I'd return the resolve value,
but that means that the variadic version breaks. If it's allowed to also
return iterators and such, it would make it too easy to break things. If
you would return 2 array items, it will become the wrong sequence of
arguments and break your method.

In my opinion having just yield is a good idea because it limits the usage
and reduces the chance of breaking. If you have an idea on how to approve
it, I'm all ears though

On Sun, Apr 10, 2016 at 1:41 PM Christian Flothmann <
notifications@github.com> wrote:

In src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
#18308 (comment):

•    $this->argumentValueResolvers = $argumentValueResolvers;
  

• }

• public function getArguments(Request $request, $controller)
• {

•    $arguments = array();
  

•    foreach ($this->argumentMetadataFactory->createArgumentMetadata($controller) as $metadata) {
  

•        foreach ($this->argumentValueResolvers as $resolver) {
  

•            if (!$resolver->supports($request, $metadata)) {
  

•                continue;
  

•            }
  

•            $resolved = $resolver->resolve($request, $metadata);
  

•            if (!$resolved instanceof \Generator) {
  

@wouterj https://github.com/WouterJ If we don't expect other code to
make use of the return values, we should remove the
ArgumentResolverInterface. Having such an interface calls for custom
implementations imo which will then probably rely on the return type.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/symfony/symfony/pull/18308/files/b1e12875553883b0c3a01a7581109e0bc946a68a#r59132159

So I guess doc blocks were removed here too ?

I had inherit doc tags before so it was rather useless. If I remember correctly, the "way to go" is to omit the docblock if it's the same as the parent/interface

@iltar AFAIK, we use inheritdoc tags in Symfony, to make it clear that the phpdoc is inherited and so does not need to be written by someone looking for missing phpdoc (unless I missed a policy change).

👍

@stof that's what I thought at first too, I can re-add them though. @fabpot can you enlighten this?

Rules are the following:

• We don't document arguments when it does not add any more information than what the signature already gives
• We are using @{inheritdoc} to specify that the method is documented in a parent class or interface (to make a difference with methods that are not documented).

They are needed here though.