core/core.services.yml | 4 +- .../ContentControllerSubscriber.php | 11 ++- .../CustomPageExceptionHtmlSubscriber.php | 29 +++---- .../DefaultExceptionHtmlSubscriber.php | 94 +++++++++++----------- .../system/src/Tests/Routing/DestinationTest.php | 2 +- .../src/Tests/Routing/ExceptionHandlingTest.php | 16 ++++ .../router_test_directory/router_test.routing.yml | 7 ++ .../router_test_directory/src/TestControllers.php | 9 +++ .../CustomPageExceptionHtmlSubscriberTest.php | 38 ++++----- 9 files changed, 118 insertions(+), 92 deletions(-) diff --git a/core/core.services.yml b/core/core.services.yml index a83af34..7e4f476 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -1144,7 +1144,7 @@ services: class: Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber tags: - { name: event_subscriber } - arguments: ['@http_kernel', '@logger.channel.php', '@redirect.destination'] + arguments: ['@http_kernel', '@logger.channel.php', '@redirect.destination', '@router.no_access_checks'] exception.default: class: Drupal\Core\EventSubscriber\DefaultExceptionSubscriber tags: @@ -1164,7 +1164,7 @@ services: class: Drupal\Core\EventSubscriber\CustomPageExceptionHtmlSubscriber tags: - { name: event_subscriber } - arguments: ['@config.factory', '@path.alias_manager', '@http_kernel', '@logger.channel.php', '@redirect.destination'] + arguments: ['@config.factory', '@http_kernel', '@logger.channel.php', '@redirect.destination', '@router.no_access_checks'] exception.fast_404_html: class: Drupal\Core\EventSubscriber\Fast404ExceptionHtmlSubscriber tags: diff --git a/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php index f6f30fe..aecf408 100644 --- a/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php @@ -28,7 +28,7 @@ class ContentControllerSubscriber implements EventSubscriberInterface { public function onRequestDeriveFormWrapper(GetResponseEvent $event) { $request = $event->getRequest(); - if ($request->attributes->has('_form')) { + if ($request->attributes->has('_form') && !$request->attributes->has('_controller')) { $request->attributes->set('_controller', 'controller.form:getContentResult'); } } @@ -40,8 +40,17 @@ public function onRequestDeriveFormWrapper(GetResponseEvent $event) { * An array of event listener definitions. */ static function getSubscribedEvents() { + // Run this at priority 25, after routing (i.e. after RouterListener): when + // a route points to a _form, we need to convert it to a _controller. $events[KernelEvents::REQUEST][] = array('onRequestDeriveFormWrapper', 25); + // Also do this at priority 33, before routing (i.e. before RouterListener), + // in case the request has already been explicitly configured to point to + // a _form. This allows us to map that _form to a _controller before routing + // and thus avoid performing routing logic at all. This is necessary when an + // exception (4xx response) is configured to use a form. + $events[KernelEvents::REQUEST][] = array('onRequestDeriveFormWrapper', 33); + return $events; } diff --git a/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php index 8422db2..0aef10c 100644 --- a/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php @@ -14,6 +14,7 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; +use Symfony\Component\Routing\Matcher\UrlMatcherInterface; /** * Exception subscriber for handling core custom HTML error pages. @@ -28,30 +29,22 @@ class CustomPageExceptionHtmlSubscriber extends DefaultExceptionHtmlSubscriber { protected $configFactory; /** - * The page alias manager. - * - * @var \Drupal\Core\Path\AliasManagerInterface - */ - protected $aliasManager; - - /** * Constructs a new CustomPageExceptionHtmlSubscriber. * * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory * The configuration factory. - * @param \Drupal\Core\Path\AliasManagerInterface $alias_manager - * The alias manager service. * @param \Symfony\Component\HttpKernel\HttpKernelInterface $http_kernel * The HTTP Kernel service. * @param \Psr\Log\LoggerInterface $logger * The logger service. * @param \Drupal\Core\Routing\RedirectDestinationInterface $redirect_destination * The redirect destination service. + * @param \Symfony\Component\Routing\Matcher\UrlMatcherInterface $access_unaware_router + * A router implementation which does not check access. */ - public function __construct(ConfigFactoryInterface $config_factory, AliasManagerInterface $alias_manager, HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination) { - parent::__construct($http_kernel, $logger, $redirect_destination); + public function __construct(ConfigFactoryInterface $config_factory, HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination, UrlMatcherInterface $access_unaware_router) { + parent::__construct($http_kernel, $logger, $redirect_destination, $access_unaware_router); $this->configFactory = $config_factory; - $this->aliasManager = $alias_manager; } /** @@ -65,16 +58,20 @@ protected static function getPriority() { * {@inheritdoc} */ public function on403(GetResponseForExceptionEvent $event) { - $path = $this->aliasManager->getPathByAlias($this->configFactory->get('system.site')->get('page.403')); - $this->makeSubrequest($event, trim($path, '/'), Response::HTTP_FORBIDDEN); + $custom_403_path = $this->configFactory->get('system.site')->get('page.403'); + if (!empty($custom_403_path)) { + $this->makeSubrequest($event, $custom_403_path, Response::HTTP_FORBIDDEN); + } } /** * {@inheritdoc} */ public function on404(GetResponseForExceptionEvent $event) { - $path = $this->aliasManager->getPathByAlias($this->configFactory->get('system.site')->get('page.404')); - $this->makeSubrequest($event, trim($path, '/'), Response::HTTP_NOT_FOUND); + $custom_404_path = $this->configFactory->get('system.site')->get('page.404'); + if (!empty($custom_404_path)) { + $this->makeSubrequest($event, $custom_404_path, Response::HTTP_NOT_FOUND); + } } } diff --git a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php index 770b96f..4ace34c 100644 --- a/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php @@ -7,16 +7,14 @@ namespace Drupal\Core\EventSubscriber; -use Drupal\Core\Routing\AccessAwareRouterInterface; use Drupal\Core\Routing\RedirectDestinationInterface; -use Drupal\Core\Url; use Drupal\Core\Utility\Error; use Psr\Log\LoggerInterface; -use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface; +use Symfony\Component\Routing\Matcher\UrlMatcherInterface; /** * Exception subscriber for handling core default HTML error pages. @@ -45,6 +43,13 @@ class DefaultExceptionHtmlSubscriber extends HttpExceptionSubscriberBase { protected $redirectDestination; /** + * A router implementation which does not check access. + * + * @var \Symfony\Component\Routing\Matcher\UrlMatcherInterface + */ + protected $accessUnawareRouter; + + /** * Constructs a new DefaultExceptionHtmlSubscriber. * * @param \Symfony\Component\HttpKernel\HttpKernelInterface $http_kernel @@ -53,11 +58,14 @@ class DefaultExceptionHtmlSubscriber extends HttpExceptionSubscriberBase { * The logger service. * @param \Drupal\Core\Routing\RedirectDestinationInterface $redirect_destination * The redirect destination service. + * @param \Symfony\Component\Routing\Matcher\UrlMatcherInterface $access_unaware_router + * A router implementation which does not check access. */ - public function __construct(HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination) { + public function __construct(HttpKernelInterface $http_kernel, LoggerInterface $logger, RedirectDestinationInterface $redirect_destination, UrlMatcherInterface $access_unaware_router) { $this->httpKernel = $http_kernel; $this->logger = $logger; $this->redirectDestination = $redirect_destination; + $this->accessUnawareRouter = $access_unaware_router; } /** @@ -83,7 +91,7 @@ protected function getHandledFormats() { * The event to process. */ public function on401(GetResponseForExceptionEvent $event) { - $this->makeSubrequest($event, Url::fromRoute('system.401')->toString(), Response::HTTP_UNAUTHORIZED); + $this->makeSubrequest($event, '/system/401', Response::HTTP_UNAUTHORIZED); } /** @@ -93,7 +101,7 @@ public function on401(GetResponseForExceptionEvent $event) { * The event to process. */ public function on403(GetResponseForExceptionEvent $event) { - $this->makeSubrequest($event, Url::fromRoute('system.403')->toString(), Response::HTTP_FORBIDDEN); + $this->makeSubrequest($event, '/system/403', Response::HTTP_FORBIDDEN); } /** @@ -103,7 +111,7 @@ public function on403(GetResponseForExceptionEvent $event) { * The event to process. */ public function on404(GetResponseForExceptionEvent $event) { - $this->makeSubrequest($event, Url::fromRoute('system.404')->toString(), Response::HTTP_NOT_FOUND); + $this->makeSubrequest($event, '/system/404', Response::HTTP_NOT_FOUND); } /** @@ -120,49 +128,39 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $url, $st $request = $event->getRequest(); $exception = $event->getException(); - if (!($url && $url[0] == '/')) { - $url = $request->getBasePath() . '/' . $url; - } - - $current_url = $request->getBasePath() . $request->getPathInfo(); - - if ($url != $request->getBasePath() . '/' && $url != $current_url) { - if ($request->getMethod() === 'POST') { - $sub_request = Request::create($url, 'POST', $this->redirectDestination->getAsArray() + ['_exception_statuscode' => $status_code] + $request->request->all(), $request->cookies->all(), [], $request->server->all()); - } - else { - $sub_request = Request::create($url, 'GET', $request->query->all() + $this->redirectDestination->getAsArray() + ['_exception_statuscode' => $status_code], $request->cookies->all(), [], $request->server->all()); + try { + // Reuse the exact same request (so keep the same URL, keep the access + // result, the exception, et cetera) but override the routing information. + // This means that aside from routing, this is identical to the master + // request. This allows us to generate a response that is executed on + // behalf of the master request, i.e. for the original URL. This is what + // allows us to e.g. generate a 404 response for the original URL; if we + // would execute a subrequest with the 404 route's URL, then it'd be + // generated for *that* URL, not the *original* URL. + $sub_request = clone $request; + $sub_request->attributes->add($this->accessUnawareRouter->match($url)); + + // Add to the query string: + // - a 'destination' query argument + // - an '_exception_statuscode' + $sub_request->query->add($this->redirectDestination->getAsArray() + ['_exception_statuscode' => $status_code]); + + $response = $this->httpKernel->handle($sub_request, HttpKernelInterface::SUB_REQUEST); + $response->setStatusCode($status_code); + + // Persist any special HTTP headers that were set on the exception. + if ($exception instanceof HttpExceptionInterface) { + $response->headers->add($exception->getHeaders()); } - try { - // Persist the 'exception' attribute to the subrequest. - $sub_request->attributes->set('exception', $request->attributes->get('exception')); - // Persist the access result attribute to the subrequest, so that the - // error page inherits the access result of the master request. - $sub_request->attributes->set(AccessAwareRouterInterface::ACCESS_RESULT, $request->attributes->get(AccessAwareRouterInterface::ACCESS_RESULT)); - - // Carry over the session to the subrequest. - if ($session = $request->getSession()) { - $sub_request->setSession($session); - } - - $response = $this->httpKernel->handle($sub_request, HttpKernelInterface::SUB_REQUEST); - $response->setStatusCode($status_code); - - // Persist any special HTTP headers that were set on the exception. - if ($exception instanceof HttpExceptionInterface) { - $response->headers->add($exception->getHeaders()); - } - - $event->setResponse($response); - } - catch (\Exception $e) { - // If an error happened in the subrequest we can't do much else. Instead, - // just log it. The DefaultExceptionSubscriber will catch the original - // exception and handle it normally. - $error = Error::decodeException($e); - $this->logger->log($error['severity_level'], '%type: @message in %function (line %line of %file).', $error); - } + $event->setResponse($response); + } + catch (\Exception $e) { + // If an error happened in the subrequest we can't do much else. Instead, + // just log it. The DefaultExceptionSubscriber will catch the original + // exception and handle it normally. + $error = Error::decodeException($e); + $this->logger->log($error['severity_level'], '%type: @message in %function (line %line of %file).', $error); } } diff --git a/core/modules/system/src/Tests/Routing/DestinationTest.php b/core/modules/system/src/Tests/Routing/DestinationTest.php index a590d62..ce72f7b 100644 --- a/core/modules/system/src/Tests/Routing/DestinationTest.php +++ b/core/modules/system/src/Tests/Routing/DestinationTest.php @@ -74,7 +74,7 @@ public function testDestination() { // Make sure that 404 pages do not populate $_GET['destination'] with // external URLs. - \Drupal::configFactory()->getEditable('system.site')->set('page.404', 'system-test/get-destination')->save(); + \Drupal::configFactory()->getEditable('system.site')->set('page.404', '/system-test/get-destination')->save(); $this->drupalGet('http://example.com', ['external' => FALSE]); $this->assertResponse(404); $this->assertIdentical(Url::fromRoute('')->toString(), $this->getRawContent(), 'External URL is not allowed on 404 pages.'); diff --git a/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php b/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php index ee3b6eb..c70bfb6 100644 --- a/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php +++ b/core/modules/system/src/Tests/Routing/ExceptionHandlingTest.php @@ -99,6 +99,22 @@ public function testHtml404() { } /** + * Tests that the exception response is executed in the original context. + */ + public function testExceptionResponseGeneratedForOriginalRequest() { + $this->config('system.site')->set('page.404', '/router_test/test25')->save(); + + $request = Request::create('/not-found'); + $request->setFormat('html', ['text/html']); + + /** @var \Symfony\Component\HttpKernel\HttpKernelInterface $kernel */ + $kernel = \Drupal::getContainer()->get('http_kernel'); + $response = $kernel->handle($request)->prepare($request); + + $this->assertTrue(strpos($response->getContent(), '/not-found') !== FALSE); + } + + /** * Tests if exception backtraces are properly escaped when output to HTML. */ public function testBacktraceEscaping() { diff --git a/core/modules/system/tests/modules/router_test_directory/router_test.routing.yml b/core/modules/system/tests/modules/router_test_directory/router_test.routing.yml index e0c91dd..6a329ce 100644 --- a/core/modules/system/tests/modules/router_test_directory/router_test.routing.yml +++ b/core/modules/system/tests/modules/router_test_directory/router_test.routing.yml @@ -155,6 +155,13 @@ router_test.24: requirements: _access: 'TRUE' +router_test.25: + path: '/router_test/test25' + defaults: + _controller: '\Drupal\router_test\TestControllers::test25' + requirements: + _access: 'TRUE' + router_test.hierarchy_parent: path: '/menu-test/parent' defaults: diff --git a/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php b/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php index 40fad0b..d603de8 100644 --- a/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php +++ b/core/modules/system/tests/modules/router_test_directory/src/TestControllers.php @@ -106,6 +106,15 @@ public function test24() { throw new \Exception('Escaped content:


'); } + public function test25() { + return [ + '#cache' => [ + 'url', + ], + '#markup' => \Drupal::requestStack()->getCurrentRequest()->getUri(), + ]; + } + /** * Throws an exception. * diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php index a7efb7e..cdcf5b4 100644 --- a/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/CustomPageExceptionHtmlSubscriberTest.php @@ -36,13 +36,6 @@ class CustomPageExceptionHtmlSubscriberTest extends UnitTestCase { protected $configFactory; /** - * The mocked alias manager. - * - * @var \Drupal\Core\Path\AliasManagerInterface|\PHPUnit_Framework_MockObject_MockObject - */ - protected $aliasManager; - - /** * The mocked logger. * * @var \Psr\Log\LoggerInterface @@ -71,21 +64,31 @@ class CustomPageExceptionHtmlSubscriberTest extends UnitTestCase { protected $redirectDestination; /** + * The mocked access unaware router. + * @var \Symfony\Component\Routing\Matcher\UrlMatcherInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $accessUnawareRouter; + + /** * {@inheritdoc} */ protected function setUp() { - $this->configFactory = $this->getConfigFactoryStub(['system.site' => ['page.403' => 'access-denied-page', 'page.404' => 'not-found-page']]); + $this->configFactory = $this->getConfigFactoryStub(['system.site' => ['page.403' => '/access-denied-page', 'page.404' => '/not-found-page']]); - $this->aliasManager = $this->getMock('Drupal\Core\Path\AliasManagerInterface'); $this->kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); $this->logger = $this->getMock('Psr\Log\LoggerInterface'); $this->redirectDestination = $this->getMock('\Drupal\Core\Routing\RedirectDestinationInterface'); - $this->redirectDestination->expects($this->any()) ->method('getAsArray') ->willReturn(['destination' => 'test']); + $this->accessUnawareRouter = $this->getMock('Symfony\Component\Routing\Matcher\UrlMatcherInterface'); + $this->accessUnawareRouter->expects($this->any()) + ->method('match') + ->willReturn([ + '_controller' => 'mocked', + ]); - $this->customPageSubscriber = new CustomPageExceptionHtmlSubscriber($this->configFactory, $this->aliasManager, $this->kernel, $this->logger, $this->redirectDestination); + $this->customPageSubscriber = new CustomPageExceptionHtmlSubscriber($this->configFactory, $this->kernel, $this->logger, $this->redirectDestination, $this->accessUnawareRouter); // You can't create an exception in PHP without throwing it. Store the // current error_log, and disable it temporarily. @@ -100,20 +103,9 @@ protected function tearDown() { } /** - * Sets up an alias manager that does nothing. - */ - protected function setupStubAliasManager() { - $this->aliasManager->expects($this->any()) - ->method('getPathByAlias') - ->willReturnArgument(0); - } - - /** * Tests onHandleException with a POST request. */ public function testHandleWithPostRequest() { - $this->setupStubAliasManager(); - $request = Request::create('/test', 'POST', array('name' => 'druplicon', 'pass' => '12345')); $this->kernel->expects($this->once())->method('handle')->will($this->returnCallback(function (Request $request) { @@ -133,8 +125,6 @@ public function testHandleWithPostRequest() { * Tests onHandleException with a GET request. */ public function testHandleWithGetRequest() { - $this->setupStubAliasManager(); - $request = Request::create('/test', 'GET', array('name' => 'druplicon', 'pass' => '12345')); $this->kernel->expects($this->once())->method('handle')->will($this->returnCallback(function (Request $request) {