summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorLee Rowlands2018-05-11 22:20:15 (GMT)
committerLee Rowlands2018-05-11 22:20:15 (GMT)
commit41dfad388cbdb43664003f420b516b07a70e713e (patch)
tree609badfdd59876e5760d1cee303c2e296a9a05d3
parentc2585a1888dba714a61b48dc619c88674084e118 (diff)
Issue #2854543 by Wim Leers, polynya, dawehner, mpdonadio, gabesullice, larowlan, klausi: NegotiationMiddleware calls $request->setRequestFormat('html') when there is no _format request parameter, but shouldn't
-rw-r--r--core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php37
-rw-r--r--core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php8
-rw-r--r--core/modules/rest/tests/modules/rest_test/rest_test.services.yml4
-rw-r--r--core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php68
-rw-r--r--core/modules/system/tests/modules/default_format_test/default_format_test.info.yml6
-rw-r--r--core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml28
-rw-r--r--core/modules/system/tests/modules/default_format_test/src/DefaultFormatTestController.php15
-rw-r--r--core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php32
-rw-r--r--core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php10
9 files changed, 170 insertions, 38 deletions
diff --git a/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
index dbfb801..7448e6f 100644
--- a/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
+++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
@@ -4,6 +4,7 @@ namespace Drupal\Core\Routing;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException;
+use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
/**
@@ -17,7 +18,14 @@ class RequestFormatRouteFilter implements FilterInterface {
public function filter(RouteCollection $collection, Request $request) {
// Determine the request format.
$default_format = static::getDefaultFormat($collection);
- $format = $request->getRequestFormat($default_format);
+ // If the request does not specify a format then use the default.
+ if (is_null($request->getRequestFormat(NULL))) {
+ $format = $default_format;
+ $request->setRequestFormat($default_format);
+ }
+ else {
+ $format = $request->getRequestFormat($default_format);
+ }
$routes_with_requirement = [];
$routes_without_requirement = [];
@@ -60,7 +68,9 @@ class RequestFormatRouteFilter implements FilterInterface {
*
* By default, use 'html' as the default format. But when there's only a
* single route match, and that route specifies a '_format' requirement
- * listing a single format, then use that as the default format.
+ * listing a single format, then use that as the default format. Also, if
+ * there are multiple routes which all require the same single format then
+ * use it.
*
* @param \Symfony\Component\Routing\RouteCollection $collection
* The route collection to filter.
@@ -69,15 +79,20 @@ class RequestFormatRouteFilter implements FilterInterface {
* The default format.
*/
protected static function getDefaultFormat(RouteCollection $collection) {
- $default_format = 'html';
- if ($collection->count() === 1) {
- $only_route = $collection->getIterator()->current();
- $required_format = $only_route->getRequirement('_format');
- if (strpos($required_format, '|') === FALSE) {
- $default_format = $required_format;
- }
- }
- return $default_format;
+ // Get the set of formats across all routes in the collection.
+ $all_formats = array_reduce($collection->all(), function (array $carry, Route $route) {
+ // Routes without a '_format' requirement are assumed to require HTML.
+ $route_formats = !$route->hasRequirement('_format')
+ ? ['html']
+ : explode('|', $route->getRequirement('_format'));
+ return array_merge($carry,$route_formats);
+ }, []);
+ $formats = array_unique(array_filter($all_formats));
+
+ // The default format is 'html' unless ALL routes require the same format.
+ return count($formats) === 1
+ ? reset($formats)
+ : 'html';
}
}
diff --git a/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
index e386461..f246cce 100644
--- a/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
+++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
@@ -46,7 +46,9 @@ class NegotiationMiddleware implements HttpKernelInterface {
}
// Determine the request format using the negotiator.
- $request->setRequestFormat($this->getContentType($request));
+ if ($requested_format = $this->getContentType($request)) {
+ $request->setRequestFormat($requested_format);
+ }
return $this->app->handle($request, $type, $catch);
}
@@ -88,8 +90,8 @@ class NegotiationMiddleware implements HttpKernelInterface {
return $request->query->get('_format');
}
- // Do HTML last so that it always wins.
- return 'html';
+ // No format was specified in the request.
+ return NULL;
}
}
diff --git a/core/modules/rest/tests/modules/rest_test/rest_test.services.yml b/core/modules/rest/tests/modules/rest_test/rest_test.services.yml
index d316cf6..85b418a 100644
--- a/core/modules/rest/tests/modules/rest_test/rest_test.services.yml
+++ b/core/modules/rest/tests/modules/rest_test/rest_test.services.yml
@@ -12,3 +12,7 @@ services:
public: false
tags:
- { name: page_cache_request_policy }
+ rest_test.encoder.foobar:
+ class: Drupal\serialization\Encoder\JsonEncoder
+ tags:
+ - { name: encoder, format: foobar }
diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
index 2ab4527..b5d7fbb 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -156,12 +156,22 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
/**
* Provides an entity resource.
+ *
+ * @param bool $single_format
+ * Provisions a single-format entity REST resource. Defaults to FALSE.
*/
- protected function provisionEntityResource() {
+ protected function provisionEntityResource($single_format = FALSE) {
+ if ($existing = $this->resourceConfigStorage->load(static::$resourceConfigId)) {
+ $existing->delete();
+ }
+
+ $format = $single_format
+ ? [static::$format]
+ : [static::$format, 'foobar'];
// It's possible to not have any authentication providers enabled, when
// testing public (anonymous) usage of a REST resource.
$auth = isset(static::$auth) ? [static::$auth] : [];
- $this->provisionResource([static::$format], $auth);
+ $this->provisionResource($format, $auth);
}
/**
@@ -434,20 +444,6 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
}
$this->provisionEntityResource();
- // Simulate the developer again forgetting the ?_format query string.
- $url->setOption('query', []);
-
- // DX: 406 when ?_format is missing, except when requesting a canonical HTML
- // route.
- $response = $this->request('GET', $url, $request_options);
- if ($has_canonical_url && (!static::$auth || static::$auth === 'cookie')) {
- $this->assertSame(403, $response->getStatusCode());
- }
- else {
- $this->assert406Response($response);
- }
-
- $url->setOption('query', ['_format' => static::$format]);
// DX: forgetting authentication: authentication provider-specific error
// response.
@@ -472,10 +468,44 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
unset($request_options[RequestOptions::HEADERS]['REST-test-auth-global']);
$request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions('GET'));
- // DX: 403 when unauthorized.
- $response = $this->request('GET', $url, $request_options);
+ // First: single format. Drupal will automatically pick the only format.
+ $this->provisionEntityResource(TRUE);
$expected_403_cacheability = $this->getExpectedUnauthorizedAccessCacheability();
- $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS');
+ // DX: 403 because unauthorized single-format route, ?_format is omittable.
+ $url->setOption('query', []);
+ $response = $this->request('GET', $url, $request_options);
+ if ($has_canonical_url) {
+ $this->assertSame(403, $response->getStatusCode());
+ $this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
+ }
+ else {
+ $this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS');
+ }
+ $this->assertSame(static::$auth ? [] : ['MISS'], $response->getHeader('X-Drupal-Cache'));
+ // DX: 403 because unauthorized.
+ $url->setOption('query', ['_format' => static::$format]);
+ $response = $this->request('GET', $url, $request_options);
+ $this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', $has_canonical_url ? 'MISS' : 'HIT');
+
+ // Then, what we'll use for the remainder of the test: multiple formats.
+ $this->provisionEntityResource();
+ // DX: 406 because despite unauthorized, ?_format is not omittable.
+ $url->setOption('query', []);
+ $response = $this->request('GET', $url, $request_options);
+ if ($has_canonical_url) {
+ $this->assertSame(403, $response->getStatusCode());
+ $this->assertSame(['HIT'], $response->getHeader('X-Drupal-Dynamic-Cache'));
+ }
+ else {
+ $this->assertSame(406, $response->getStatusCode());
+ $this->assertSame(['UNCACHEABLE'], $response->getHeader('X-Drupal-Dynamic-Cache'));
+ }
+ $this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
+ $this->assertSame(static::$auth ? [] : ['MISS'], $response->getHeader('X-Drupal-Cache'));
+ // DX: 403 because unauthorized.
+ $url->setOption('query', ['_format' => static::$format]);
+ $response = $this->request('GET', $url, $request_options);
+ $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'HIT');
$this->assertArrayNotHasKey('Link', $response->getHeaders());
$this->setUpAuthorization('GET');
diff --git a/core/modules/system/tests/modules/default_format_test/default_format_test.info.yml b/core/modules/system/tests/modules/default_format_test/default_format_test.info.yml
new file mode 100644
index 0000000..0a02a06
--- /dev/null
+++ b/core/modules/system/tests/modules/default_format_test/default_format_test.info.yml
@@ -0,0 +1,6 @@
+name: 'Default format test'
+type: module
+description: 'Support module for testing default route format.'
+package: Testing
+version: VERSION
+core: 8.x
diff --git a/core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml b/core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml
new file mode 100644
index 0000000..368dcb1
--- /dev/null
+++ b/core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml
@@ -0,0 +1,28 @@
+default_format_test.machine:
+ path: '/default_format_test/machine'
+ defaults:
+ # Same controller + method!
+ _controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
+ requirements:
+ _access: 'TRUE'
+ _format: 'json'
+
+default_format_test.human:
+ path: '/default_format_test/human'
+ defaults:
+ # Same controller + method!
+ _controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
+ requirements:
+ _access: 'TRUE'
+ _format: 'html'
+
+# Route definition identical to default_format_test.machine, only different name.
+# @see \Drupal\FunctionalTests\Routing\DefaultFormatTest::testMultiple
+default_format_test.machine.alias:
+ path: '/default_format_test/machine'
+ defaults:
+ # Same controller + method!
+ _controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
+ requirements:
+ _access: 'TRUE'
+ _format: 'json'
diff --git a/core/modules/system/tests/modules/default_format_test/src/DefaultFormatTestController.php b/core/modules/system/tests/modules/default_format_test/src/DefaultFormatTestController.php
new file mode 100644
index 0000000..6e52eea
--- /dev/null
+++ b/core/modules/system/tests/modules/default_format_test/src/DefaultFormatTestController.php
@@ -0,0 +1,15 @@
+<?php
+
+namespace Drupal\default_format_test;
+
+use Drupal\Core\Cache\CacheableResponse;
+use Symfony\Component\HttpFoundation\Request;
+
+class DefaultFormatTestController {
+
+ public function content(Request $request) {
+ $format = $request->getRequestFormat();
+ return new CacheableResponse('format:' . $format, 200, ['Content-Type' => $request->getMimeType($format)]);
+ }
+
+}
diff --git a/core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php b/core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php
new file mode 100644
index 0000000..bf695d6
--- /dev/null
+++ b/core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php
@@ -0,0 +1,32 @@
+<?php
+
+namespace Drupal\FunctionalTests\Routing;
+
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * @group routing
+ */
+class DefaultFormatTest extends BrowserTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ public static $modules = ['system', 'default_format_test'];
+
+ public function testFoo() {
+ $this->drupalGet('/default_format_test/human');
+ $this->assertSame('format:html', $this->getSession()->getPage()->getContent());
+ $this->assertSame('MISS', $this->drupalGetHeader('X-Drupal-Cache'));
+
+ $this->drupalGet('/default_format_test/machine');
+ $this->assertSame('format:json', $this->getSession()->getPage()->getContent());
+ $this->assertSame('MISS', $this->drupalGetHeader('X-Drupal-Cache'));
+ }
+
+ public function testMultipleRoutesWithSameSingleFormat() {
+ $this->drupalGet('/default_format_test/machine');
+ $this->assertSame('format:json', $this->getSession()->getPage()->getContent());
+ }
+
+}
diff --git a/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
index 10d2df8..7b5217d 100644
--- a/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
+++ b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
@@ -68,10 +68,10 @@ class NegotiationMiddlewareTest extends UnitTestCase {
*
* @covers ::getContentType
*/
- public function testUnknowContentTypeReturnsHtmlByDefault() {
+ public function testUnknowContentTypeReturnsNull() {
$request = new Request();
- $this->assertSame('html', $this->contentNegotiation->getContentType($request));
+ $this->assertNull($this->contentNegotiation->getContentType($request));
}
/**
@@ -83,7 +83,7 @@ class NegotiationMiddlewareTest extends UnitTestCase {
$request = new Request();
$request->headers->set('X-Requested-With', 'XMLHttpRequest');
- $this->assertSame('html', $this->contentNegotiation->getContentType($request));
+ $this->assertNull($this->contentNegotiation->getContentType($request));
}
/**
@@ -98,7 +98,7 @@ class NegotiationMiddlewareTest extends UnitTestCase {
$request->setFormat()->shouldNotBeCalled();
// Request format will be set with default format.
- $request->setRequestFormat('html')->shouldBeCalled();
+ $request->setRequestFormat()->shouldNotBeCalled();
// Some getContentType calls we don't really care about but have to mock.
$request_data = $this->prophesize(ParameterBag::class);
@@ -127,7 +127,7 @@ class NegotiationMiddlewareTest extends UnitTestCase {
$request->setFormat('david', 'geeky/david')->shouldBeCalled();
// Some calls we don't care about.
- $request->setRequestFormat('html')->shouldBeCalled();
+ $request->setRequestFormat()->shouldNotBeCalled();
$request_data = $this->prophesize(ParameterBag::class);
$request_data->get('ajax_iframe_upload', FALSE)->shouldBeCalled();
$request_mock = $request->reveal();