-
Notifications
You must be signed in to change notification settings - Fork 8
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
Override callback resolver #10
Changes from 8 commits
bc76078
4dacf57
882b01d
20b417e
3396b2f
1c370f2
0c4b29d
c94ee6b
0b1745f
fd9430f
8b7b821
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
<?php | ||
|
||
namespace DI\Bridge\Silex; | ||
|
||
use Invoker\CallableResolver; | ||
use Invoker\Exception\NotCallableException; | ||
|
||
/** | ||
* This alternative resolver uses the generic Invoker to support PHP-DI's extended callable syntax | ||
*/ | ||
class CallbackResolver extends \Silex\CallbackResolver | ||
{ | ||
/** | ||
* @var CallableResolver | ||
*/ | ||
private $resolver; | ||
|
||
public function __construct(\Pimple $app, CallableResolver $resolver) | ||
{ | ||
$this->resolver = $resolver; | ||
parent::__construct($app); | ||
} | ||
|
||
/** | ||
* @param string $name | ||
* @return array|callable | ||
*/ | ||
public function convertCallback($name) | ||
{ | ||
// original pattern | ||
if ($this->isValid($name)) { | ||
return parent::convertCallback($name); | ||
} | ||
|
||
// try to resolve callback from container | ||
try { | ||
return $this->resolver->resolve($name); | ||
} catch (NotCallableException $e) { | ||
throw new \InvalidArgumentException(sprintf('Service "%s" does not exist.', $name)); | ||
} | ||
} | ||
|
||
/** | ||
* @param string|callable $name | ||
* @return callable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't know whats the convention for return type hints for callables but it should be consistent with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
*/ | ||
public function resolveCallback($name) | ||
{ | ||
// return immediately if $name is callable | ||
if (is_callable($name)) { | ||
return $name; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm afraid this might be an issue as method calls are considered as statically callable. For example Anyway all that to say that this shortcut will create issues with some types of callables. |
||
|
||
return $this->convertCallback($name); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,4 +53,14 @@ public function the_controller_resolver_should_be_registered_as_a_service() | |
$this->assertInstanceOf('Closure', $app->raw('resolver')); | ||
$this->assertInstanceOf('DI\Bridge\Silex\Controller\ControllerResolver', $app['resolver']); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function the_callback_resolver_should_be_registers_as_a_service() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/registers/registered |
||
{ | ||
$app = new Application(); | ||
|
||
$this->assertInstanceOf('DI\Bridge\Silex\CallbackResolver', $app['callback_resolver']); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
<?php | ||
|
||
namespace DI\Bridge\Silex\Test; | ||
|
||
use DI\Bridge\Silex\Application; | ||
use DI\Bridge\Silex\Test\Fixture\InvokableController; | ||
|
||
class CallbackResolverTest extends BaseTestCase | ||
{ | ||
/** | ||
* @test | ||
*/ | ||
public function resolver_must_able_to_resolve_original_callback_format() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it reads besser with |
||
{ | ||
$app = new Application(); | ||
$app['controller'] = function () { | ||
return new InvokableController(); | ||
}; | ||
|
||
$callable = $app['callback_resolver']->resolveCallback('controller:__invoke'); | ||
|
||
$this->assertEquals('Hello world', $callable()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function resolver_must_be_able_to_resolve_custom_callback_format() | ||
{ | ||
$app = new Application(); | ||
$app['controller'] = function () { | ||
return new InvokableController(); | ||
}; | ||
|
||
$callable = $app['callback_resolver']->resolveCallback('controller'); | ||
|
||
$this->assertEquals('Hello world', $callable()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function resolver_must_able_to_resolve_callable_callback() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above: I think it reads besser with |
||
{ | ||
$app = new Application(); | ||
|
||
$callable = $app['callback_resolver']->resolveCallback(function () { | ||
return 'Hello world'; | ||
}); | ||
|
||
$this->assertEquals('Hello world', $callable()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function resolver_must_throw_exeption_when_callback_not_found_in_container() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/exeption/exception |
||
{ | ||
$app = new Application(); | ||
|
||
$this->setExpectedException('\InvalidArgumentException'); | ||
$app['callback_resolver']->resolveCallback('some.service'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
<?php | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nits: remove 1 newline |
||
namespace DI\Bridge\Silex\Test\Fixture; | ||
|
||
class HelloController | ||
{ | ||
public function __invoke(\ArrayObject $user) | ||
{ | ||
return $user['name']; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<?php | ||
|
||
namespace DI\Bridge\Silex\Test\Fixture; | ||
|
||
class InvokableConverter | ||
{ | ||
public function __invoke($user) | ||
{ | ||
return new \ArrayObject(['name' => $user]); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<?php | ||
|
||
namespace DI\Bridge\Silex\Test\Fixture; | ||
|
||
use Symfony\Component\HttpFoundation\Response; | ||
|
||
class InvokableErrorListener | ||
{ | ||
public function __invoke(\Exception $e, $code) | ||
{ | ||
return new Response("Sad panda :("); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nits: use single quotes for consistency |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
<?php | ||
|
||
namespace DI\Bridge\Silex\Test\Fixture; | ||
|
||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpFoundation\Response; | ||
|
||
class InvokableMiddleware | ||
{ | ||
public function __invoke(Request $request) | ||
{ | ||
return new Response('Hello from middleware'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<?php | ||
|
||
namespace DI\Bridge\Silex\Test\Fixture; | ||
|
||
class InvokableViewListener | ||
{ | ||
public function __invoke($controllerResult) | ||
{ | ||
return $controllerResult . ' from mars'; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -188,4 +188,60 @@ public function should_pass_request_object_based_on_type_hint() | |
$response = $app->handle(Request::create('/?name=john')); | ||
$this->assertEquals('Hello john', $response->getContent()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function should_able_to_convert_request() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above: I think it reads better with |
||
{ | ||
$app = $this->createApplication(); | ||
|
||
$app->get('/{user}', 'DI\Bridge\Silex\Test\Fixture\HelloController') | ||
->convert('user', 'DI\Bridge\Silex\Test\Fixture\InvokableConverter'); | ||
|
||
$response = $app->handle(Request::create('/PHPDI')); | ||
$this->assertEquals('PHPDI', $response->getContent()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function should_able_to_use_invokable_middleware() | ||
{ | ||
$app = $this->createApplication(); | ||
|
||
$app->get('/', 'DI\Bridge\Silex\Test\Fixture\InvokableController') | ||
->before('DI\Bridge\Silex\Test\Fixture\InvokableMiddleware'); | ||
|
||
$response = $app->handle(Request::create('/')); | ||
$this->assertEquals('Hello from middleware', $response->getContent()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function should_able_to_use_invokable_error_listener() | ||
{ | ||
$app = $this->createApplication(); | ||
|
||
$app->error('DI\Bridge\Silex\Test\Fixture\InvokableErrorListener'); | ||
|
||
$response = $app->handle(Request::create('/')); | ||
$this->assertEquals('Sad panda :(', $response->getContent()); | ||
} | ||
|
||
/** | ||
* @test | ||
*/ | ||
public function should_able_to_use_view_listener() | ||
{ | ||
$app = $this->createApplication(); | ||
|
||
$app->get('/', 'DI\Bridge\Silex\Test\Fixture\InvokableController'); | ||
|
||
$app->view('DI\Bridge\Silex\Test\Fixture\InvokableViewListener'); | ||
|
||
$response = $app->handle(Request::create('/')); | ||
$this->assertEquals('Hello world from mars', $response->getContent()); | ||
} | ||
} |
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.
Could the callable resolver from line 54 be reused here? I don't think there's a need to create it twice.
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.
yep, sure should I register callable resolver in DI first?
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.
yep sounds good. Maybe name it something like
phpdi.callable_resolver
to avoid crowding the root "namespace" for users?