Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
bug #23013 Parse the _controller format in sub-requests (weaverryan)
This PR was merged into the 3.3 branch. Discussion ---------- Parse the _controller format in sub-requests | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | possibly (edge case) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22966 | License | MIT | Doc PR | n/a As mentioned on the issue (#22966 (comment)), the new "controller service args" functionality relies on the `_controller` attribute to be in either the service format `App\Controller\Foo:bar` or at least the final parsed format `App\Controller\Foo::bar`. But when you make a sub-request with the `App:Foo:bar` format, the `ControllerResolver` correctly parses this, but the `_controller` request attribute will always contain the original `App:Foo:bar` format. That causes the `ServiceValueResolver` to fail. The only way I can think to fix this - reliably - is to parse the `_controller` attribute in a listener. And this, works great! Notes: A) There is a small chance for a BC break: if you were relying on the `_controller` old format in a `kernel.request` format in the framework, in a listener between the priority of 25 and 31 for sub-requests (because normal requests have `_controller` normalized during routing)... then you will see a behavior change. B) We could load the `ControllerNameParser` lazily via a service locator. C) We could deprecate calling the parser in the FW's `ControllerResolver`. Along with (B), I think it would (in 4.0) mean that the `ControllerNameParser` is not instantiated at runtime (except for sub-requests). If someone can think of a different/better solution, please let me know! Cheers! Commits ------- 9578fd3 Adding a new event subscriber that "parses" the _controller attribute in the FW
- Loading branch information
Showing
8 changed files
with
185 additions
and
0 deletions.
There are no files selected for viewing
48 changes: 48 additions & 0 deletions
48
src/Symfony/Bundle/FrameworkBundle/EventListener/ResolveControllerNameSubscriber.php
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
<?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\Bundle\FrameworkBundle\EventListener; | ||
|
||
use Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser; | ||
use Symfony\Component\EventDispatcher\EventSubscriberInterface; | ||
use Symfony\Component\HttpKernel\Event\GetResponseEvent; | ||
use Symfony\Component\HttpKernel\KernelEvents; | ||
|
||
/** | ||
* Guarantees that the _controller key is parsed into its final format. | ||
* | ||
* @author Ryan Weaver <ryan@knpuniversity.com> | ||
*/ | ||
class ResolveControllerNameSubscriber implements EventSubscriberInterface | ||
{ | ||
private $parser; | ||
|
||
public function __construct(ControllerNameParser $parser) | ||
{ | ||
$this->parser = $parser; | ||
} | ||
|
||
public function onKernelRequest(GetResponseEvent $event) | ||
{ | ||
$controller = $event->getRequest()->attributes->get('_controller'); | ||
if ($controller && false === strpos($controller, '::') && 2 === substr_count($controller, ':')) { | ||
// controller in the a:b:c notation then | ||
$event->getRequest()->attributes->set('_controller', $this->parser->parse($controller)); | ||
} | ||
} | ||
|
||
public static function getSubscribedEvents() | ||
{ | ||
return array( | ||
KernelEvents::REQUEST => array('onKernelRequest', 24), | ||
); | ||
} | ||
} |
This file contains 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
54 changes: 54 additions & 0 deletions
54
...ymfony/Bundle/FrameworkBundle/Tests/EventListener/ResolveControllerNameSubscriberTest.php
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
<?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\Bundle\FrameworkBundle\Tests\EventListener; | ||
|
||
use Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser; | ||
use Symfony\Bundle\FrameworkBundle\EventListener\ResolveControllerNameSubscriber; | ||
use Symfony\Bundle\FrameworkBundle\Tests\TestCase; | ||
use Symfony\Component\HttpFoundation\Request; | ||
use Symfony\Component\HttpKernel\Event\GetResponseEvent; | ||
use Symfony\Component\HttpKernel\HttpKernelInterface; | ||
|
||
class ResolveControllerNameSubscriberTest extends TestCase | ||
{ | ||
public function testReplacesControllerAttribute() | ||
{ | ||
$parser = $this->getMockBuilder(ControllerNameParser::class)->disableOriginalConstructor()->getMock(); | ||
$parser->expects($this->any()) | ||
->method('parse') | ||
->with('AppBundle:Starting:format') | ||
->willReturn('App\\Final\\Format::methodName'); | ||
$httpKernel = $this->getMockBuilder(HttpKernelInterface::class)->getMock(); | ||
|
||
$request = new Request(); | ||
$request->attributes->set('_controller', 'AppBundle:Starting:format'); | ||
|
||
$subscriber = new ResolveControllerNameSubscriber($parser); | ||
$subscriber->onKernelRequest(new GetResponseEvent($httpKernel, $request, HttpKernelInterface::MASTER_REQUEST)); | ||
$this->assertEquals('App\\Final\\Format::methodName', $request->attributes->get('_controller')); | ||
} | ||
|
||
public function testSkipsOtherControllerFormats() | ||
{ | ||
$parser = $this->getMockBuilder(ControllerNameParser::class)->disableOriginalConstructor()->getMock(); | ||
$parser->expects($this->never()) | ||
->method('parse'); | ||
$httpKernel = $this->getMockBuilder(HttpKernelInterface::class)->getMock(); | ||
|
||
$request = new Request(); | ||
$request->attributes->set('_controller', 'Other:format'); | ||
|
||
$subscriber = new ResolveControllerNameSubscriber($parser); | ||
$subscriber->onKernelRequest(new GetResponseEvent($httpKernel, $request, HttpKernelInterface::MASTER_REQUEST)); | ||
$this->assertEquals('Other:format', $request->attributes->get('_controller')); | ||
} | ||
} |
38 changes: 38 additions & 0 deletions
38
...e/Tests/Functional/Bundle/TestBundle/Controller/SubRequestServiceResolutionController.php
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
<?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\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller; | ||
|
||
use Psr\Log\LoggerInterface; | ||
use Symfony\Component\DependencyInjection\ContainerAwareInterface; | ||
use Symfony\Component\DependencyInjection\ContainerAwareTrait; | ||
use Symfony\Component\HttpFoundation\Response; | ||
use Symfony\Component\HttpKernel\HttpKernelInterface; | ||
|
||
class SubRequestServiceResolutionController implements ContainerAwareInterface | ||
{ | ||
use ContainerAwareTrait; | ||
|
||
public function indexAction() | ||
{ | ||
$request = $this->container->get('request_stack')->getCurrentRequest(); | ||
$path['_forwarded'] = $request->attributes; | ||
$path['_controller'] = 'TestBundle:SubRequestServiceResolution:fragment'; | ||
$subRequest = $request->duplicate(array(), null, $path); | ||
|
||
return $this->container->get('http_kernel')->handle($subRequest, HttpKernelInterface::SUB_REQUEST); | ||
} | ||
|
||
public function fragmentAction(LoggerInterface $logger) | ||
{ | ||
return new Response('---'); | ||
} | ||
} |
This file contains 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
18 changes: 18 additions & 0 deletions
18
...mfony/Bundle/FrameworkBundle/Tests/Functional/app/ControllerServiceResolution/bundles.php
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<?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. | ||
*/ | ||
|
||
use Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\TestBundle; | ||
use Symfony\Bundle\FrameworkBundle\FrameworkBundle; | ||
|
||
return array( | ||
new FrameworkBundle(), | ||
new TestBundle(), | ||
); |
10 changes: 10 additions & 0 deletions
10
...ymfony/Bundle/FrameworkBundle/Tests/Functional/app/ControllerServiceResolution/config.yml
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
imports: | ||
- { resource: ../config/default.yml } | ||
|
||
services: | ||
Symfony\Bundle\FrameworkBundle\Tests\Functional\Bundle\TestBundle\Controller\SubRequestServiceResolutionController: | ||
public: true | ||
tags: [controller.service_arguments] | ||
|
||
logger: { class: Psr\Log\NullLogger } | ||
Psr\Log\LoggerInterface: '@logger' |
4 changes: 4 additions & 0 deletions
4
...mfony/Bundle/FrameworkBundle/Tests/Functional/app/ControllerServiceResolution/routing.yml
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
sub_request_page: | ||
path: /subrequest | ||
defaults: | ||
_controller: 'TestBundle:SubRequestServiceResolution:index' |