8000 Added an ArgumentResolver with clean extension point by linaori · Pull Request #18308 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Added an ArgumentResolver with clean extension point #18308

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 3, 2016
Merged

Added an ArgumentResolver with clean extension point #18308

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
360fc5f
Extracting arg resolving from ControllerResolver
Mar 16, 2016
cfcf764
Added an ArgumentResolver with clean extension point
Mar 25, 2016
cee5106
cs fixes
HeahDude Apr 1, 2016
f29bf4c
Refactor ArgumentResolverTest
HeahDude Apr 1, 2016
1bf80c9
Improved DX for the ArgumentResolver
Apr 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Improved DX for the ArgumentResolver
  • Loading branch information
@linaori
Iltar van der Berg authored and linaori committed Apr 2, 2016
commit 1bf80c92ee3daf4c526189f82a3dfa7a0482bc44
8 changes: 4 additions & 4 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,19 @@
<argument type="collection" />
</service>

<service id="argument_value_resolver.argument_from_attribute" class="Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\ArgumentFromAttributeResolver" public="false">
<service id="argument_resolver.request_attribute" class="Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestAttributeValueResolver" public="false">
<tag name="controller_argument.value_resolver" priority="100" />
</service>

<service id="argument_value_resolver.argument_is_request" class="Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\RequestResolver" public="false">
<service id="argument_resolver.request" class="Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestValueResolver" public="false">
<tag name="controller_argument.value_resolver" priority="50" />
</service>

<service id="argument_value_resolver.default_argument_value" class="Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\DefaultArgumentValueResolver" public="false">
<service id="argument_resolver.default" class="Symfony\Component\HttpKernel\Controller\ArgumentResolver\DefaultValueResolver" public="false">
<tag name="controller_argument.value_resolver" priority="-100" />
</service>

<service id="argument_value_resolver.variadic_argument_from_attribute" class="Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\VariadicArgumentValueResolver" public="false">
<service id="argument_resolver.variadic" class="Symfony\Component\HttpKernel\Controller\ArgumentResolver\VariadicValueResolver" public="false">
<tag name="controller_argument.value_resolver" priority="-150" />
</service>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Expand Down
14 changes: 12 additions & 2 deletions src/Symfony/Component/HttpKernel/Controller/ArgumentResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@
namespace Symfony\Component\HttpKernel\Controller;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\DefaultValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestAttributeValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\VariadicValueResolver;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactory;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactoryInterface;

/**
Expand All @@ -30,8 +35,13 @@ final class ArgumentResolver implements ArgumentResolverInterface

public function __construct(ArgumentMetadataFactoryInterface $argumentMetadataFactory = null, array $argumentValueResolvers = array())
{
$this->argumentMetadataFactory = $argumentMetadataFactory;
$this->argumentValueResolvers = $argumentValueResolvers;
$this->argumentMetadataFactory = $argumentMetadataFactory ?: new ArgumentMetadataFactory();
$this->argumentValueResolvers = $argumentValueResolvers ?: array(
new RequestAttributeValueResolver(),
new RequestValueResolver(),
new DefaultValueResolver(),
new VariadicValueResolver(),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Elsewhere, we would also have an add() method to be able to add more value resolver. But this logic here forbids to have one. What about (and I know it's going to be controversial) removing the array typehint, make the default value to null and only automatically register the default revolsers when the value is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current method of adding them is via a compiler pass, you tag it, gets collected and then injected into the constructor. I'm personally in favour of this method to avoid 4+ method calls each webrequest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm not in the middle of nowhere with an actual keyboard...

This compiler pass does all the logic: https://github.com/symfony/symfony/pull/18308/files#diff-6df4a64da7596af5827901ecbc5d2e78R18

  • If not given, same behavior as the LegacyArgumentResolver
  • Enhanced experience without the FrameworkBundle wiring everything via services
  • Easily extendable because you only need to tag your service with controller_argument.value_resolver

In your service.yml you would only have to do this:

services:
    app.argument_resolver.user:
        class: App\ArgumentResolver\UserValueResolver
        arguments:
            - "@security.token_storage"
        tags:
            - { name: controller_argument.value_resolver, priority: 150 }

The compiler pass will simply generate an array and replace the second argument to avoid the aforementioned method calls.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally understand how that works for Symfony full-stack, I was more thinking about the integration with Silex or Drupal where the FrameworkBundle is not available. Anyway, I'm going to merge like this and we still have 2 months to see how to deal with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What could be done, is move the compiler pass to the component, but I'm not sure if silex uses this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK Silex does not use Config and DependencyInjection components by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this is merged, let's see if we can find a nice solution for those cases because ideally I'd like to support that before 3.1 is released.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we were to allow to add value resolvers after the argument resolver has been created, we should imo move the priority handling to this class instead of doing that in the compiler pass (i.e. adding a method like addArgumentValueResolver(ArgumentValueResolver $resolver, $priority)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have the advantage of little overhead because everything is compiled, I'm personally for this approach because it feels safer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should allow to add value resolver after the argument resolver has been created. 👍 for the current way

}

/**
Expand Down
4 changes: 2 additions & 2 deletions ...Resolver/DefaultArgumentValueResolver.php → ...ArgumentResolver/DefaultValueResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Controller\ArgumentValueResolver;
namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface;
Expand All @@ -20,7 +20,7 @@
*
* @author Iltar van der Berg <kjarli@gmail.com>
*/
final class DefaultArgumentValueResolver implements ArgumentValueResolverInterface
final class DefaultValueResolver implements ArgumentValueResolverInterface
{
/**
* {@inheritdoc}
Expand Down
4 changes: 2 additions & 2 deletions ...esolver/ArgumentFromAttributeResolver.php → ...esolver/RequestAttributeValueResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Controller\ArgumentValueResolver;
namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface;
Expand All @@ -20,7 +20,7 @@
*
* @author Iltar van der Berg <kjarli@gmail.com>
*/
final class ArgumentFromAttributeResolver implements ArgumentValueResolverInterface
final class RequestAttributeValueResolver implements ArgumentValueResolverInterface
{
/**
* {@inheritdoc}
Expand Down
6 changes: 3 additions & 3 deletions ...ArgumentValueResolver/RequestResolver.php → ...ArgumentResolver/RequestValueResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Controller\ArgumentValueResolver;
namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface;
Expand All @@ -20,14 +20,14 @@
*
* @author Iltar van der Berg <kjarli@gmail.com>
*/
final class RequestResolver implements ArgumentValueResolverInterface
final class RequestValueResolver implements ArgumentValueResolverInterface
{
/**
* {@inheritdoc}
*/
public function supports(Request $request, ArgumentMetadata $argument)
{
return $argument->getType() === Request::class || is_subclass_of(Request::class, $argument->getType());
return $argument->getType() === Request::class || is_subclass_of($argument->getType(), Request::class);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions ...esolver/VariadicArgumentValueResolver.php → ...rgumentResolver/VariadicValueResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Controller\ArgumentValueResolver;
namespace Symfony\Component\HttpKernel\Controller\ArgumentResolver;

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface;
Expand All @@ -20,7 +20,7 @@
*
* @author Iltar van der Berg <kjarli@gmail.com>
*/
final class VariadicArgumentValueResolver implements ArgumentValueResolverInterface
final class VariadicValueResolver implements ArgumentValueResolverInterface
{
/**
* {@inheritdoc}
Expand Down
6 changes: 5 additions & 1 deletion src/Symfony/Component/HttpKernel/ControllerMetadata/ArgumentMetadata.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,16 @@ public function hasDefaultValue()
/**
* Returns the default value of the argument.
*
* Make sure to call {@see self::hasDefaultValue()} first to see if a default value is possible.
* @throws \LogicException if no default value is present; {@see self::hasDefaultValue()}
*
* @return mixed
*/
public function getDefaultValue()
{
if (!$this->hasDefaultValue) {
throw new \LogicException(sprintf('Argument $%s does not have a default value. Use %s::hasDefaultValue() to avoid this exception.', $this->name, __CLASS__));
}

return $this->defaultValue;
}
}
3 changes: 2 additions & 1 deletion src/Symfony/Component/HttpKernel/HttpKernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\HttpKernel;

use Symfony\Component\HttpKernel\Controller\ArgumentResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolverInterface;
use Symfony\Component\HttpKernel\Controller\ControllerResolverInterface;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
Expand Down Expand Up @@ -47,7 +48,7 @@ public function __construct(EventDispatcherInterface $dispatcher, ControllerReso
$this->argumentResolver = $argumentResolver;

if (null === $this->argumentResolver) {
@trigger_error(sprintf('As of 3.1 an %s is used to resolve arguments. In 4.0 the $argumentResolver becomes mandatory and the %s can no longer be used to resolve arguments.', ArgumentResolverInterface::class, ControllerResolverInterface::class), E_USER_DEPRECATED);
@trigger_error(sprintf('As of 3.1 an %s is used to resolve arguments. In 4.0 the $argumentResolver becomes the %s if no other is provided instead of using the $resolver argument.', ArgumentResolverInterface::class, ArgumentResolver::class), E_USER_DEPRECATED);
// fallback in case of deprecations
$this->argumentResolver = $resolver;
}
Expand Down
35 changes: 27 additions & 8 deletions src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@
namespace Symfony\Component\HttpKernel\Tests\Controller;

use Symfony\Component\HttpKernel\Controller\ArgumentResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\ArgumentFromAttributeResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\DefaultArgumentValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\RequestResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\VariadicArgumentValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\DefaultValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestAttributeValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\RequestValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver\VariadicValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolverInterface;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactory;
use Symfony\Component\HttpKernel\Tests\Fixtures\Controller\ExtendingRequest;
use Symfony\Component\HttpKernel\Tests\Fixtures\Controller\VariadicController;
use Symfony\Component\HttpFoundation\Request;

Expand All @@ -30,15 +31,21 @@ public static function setUpBeforeClass()
{
$factory = new ArgumentMetadataFactory();
$argumentValueResolvers = array(
new ArgumentFromAttributeResolver(),
new VariadicArgumentValueResolver(),
new RequestResolver(),
new DefaultArgumentValueResolver(),
new RequestAttributeValueResolver(),
new RequestValueResolver(),
new DefaultValueResolver(),
new VariadicValueResolver(),
);

self::$resolver = new ArgumentResolver($factory, $argumentValueResolvers);
}

public function testDefaultState()
{
$this->assertEquals(self::$resolver, new ArgumentResolver());
$this->assertNotEquals(self::$resolver, new ArgumentResolver(null, array(new RequestAttributeValueResolver())));
}

public function testGetArguments()
{
$request = Request::create('/');
Expand Down Expand Up @@ -140,6 +147,14 @@ public function testGetArgumentsInjectsRequest()
$this->assertEquals(array($request), self::$resolver->getArguments($request, $controller), '->getArguments() injects the request');
}

public function testGetArgumentsInjectsExtendingRequest()
{
$request = ExtendingRequest::create('/');
$controller = array(new self(), 'controllerWithExtendingRequest');

$this->assertEquals(array($request), self::$resolver->getArguments($request, $controller), '->getArguments() injects the request when extended');
}

/**
* @requires PHP 5.6
*/
Expand Down Expand Up @@ -210,6 +225,10 @@ protected function controllerWithFooBarFoobar($foo, $bar, $foobar)
protected function controllerWithRequest(Request $request)
{
}
F438
protected function controllerWithExtendingRequest(ExtendingRequest $request)
{
}
}

function controller_function($foo, $foobar)
Expand Down
36 changes: 36 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/ControllerMetadata/ArgumentMetadataTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpKernel\Tests\ControllerMetadata;

use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata;

class ArgumentMetadataTest extends \PHPUnit_Framework_TestCase
{
public function testDefaultValueAvailable()
{
$argument = new ArgumentMetadata('foo', 'string', false, true, 'default value');

$this->assertTrue($argument->hasDefaultValue());
$this->assertSame('default value', $argument->getDefaultValue());
}

/**
* @expectedException \LogicException
*/
public function testDefaultValueUnavailable()
{
$argument = new ArgumentMetadata('foo', 'string', false, false, null);

$this->assertFalse($argument->hasDefaultValue());
$argument->getDefaultValue();
}
}
9 changes: 9 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/Fixtures/Controller/ExtendingRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

namespace Symfony\Component\HttpKernel\Tests\Fixtures\Controller;

use Symfony\Component\HttpFoundation\Request;

class ExtendingRequest extends Request
{
}
15 changes: 2 additions & 13 deletions src/Symfony/Component/HttpKernel/Tests/Fragment/InlineFragmentRendererTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,8 @@

use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpKernel\Controller\ArgumentResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\ArgumentFromAttributeResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\DefaultArgumentValueResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\RequestResolver;
use Symfony\Component\HttpKernel\Controller\ArgumentValueResolver\VariadicArgumentValueResolver;
use Symfony\Component\HttpKernel\Controller\ControllerResolverInterface;
use Symfony\Component\HttpKernel\Controller\ControllerReference;
use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadataFactory;
use Symfony\Component\HttpKernel\HttpKernel;
use Symfony\Component\HttpKernel\Fragment\InlineFragmentRenderer;
use Symfony\Component\HttpKernel\KernelEvents;
Expand Down Expand Up @@ -91,14 +86,8 @@ public function testRenderWithObjectsAsAttributesPassedAsObjectsInTheController(
return new Response($object1->getBar());
}))
;
$argumentValueResolvers = array(
new ArgumentFromAttributeResolver(),
new VariadicArgumentValueResolver(),
new RequestResolver(),
new DefaultArgumentValueResolver(),
);

$kernel = new HttpKernel(new EventDispatcher(), $resolver, new RequestStack(), new ArgumentResolver(new ArgumentMetadataFactory(), $argumentValueResolvers));

$kernel = new HttpKernel(new EventDispatcher(), $resolver, new RequestStack(), new ArgumentResolver());
$renderer = new InlineFragmentRenderer($kernel);

$response = $renderer->render(new ControllerReference('main_controller', array('object' => new \stdClass(), 'object1' => new Bar()), array()), Request::create('/'));
Expand Down
0