summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorxjm2017-11-07 05:18:58 -0600
committerxjm2017-11-07 05:18:58 -0600
commit3af361c83c622ed618328ebde2c8a25f26ea419e (patch)
tree693c730ac04d4fc09b9bd95abf2cdbcb2ad18160
parenta8e7de393b2b628714929a075402d9290ff473e2 (diff)
Issue #2874938 by Wim Leers, tim.plunkett, borisson_, effulgentsia: AdminRouteSubscriber must only mark HTML routes as administrative
-rw-r--r--core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php88
-rw-r--r--core/modules/system/src/EventSubscriber/AdminRouteSubscriber.php20
-rw-r--r--core/modules/system/tests/src/Unit/Routing/AdminRouteSubscriberTest.php70
3 files changed, 122 insertions, 56 deletions
diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
index 41d9eec..6faf028 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -383,18 +383,8 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
// 200 for well-formed HEAD request.
$response = $this->request('HEAD', $url, $request_options);
$this->assertResourceResponse(200, '', $response);
- // @todo Entity resources with URLs that begin with '/admin/' are marked as
- // administrative (see https://www.drupal.org/node/2874938), which
- // excludes them from Dynamic Page Cache (see
- // https://www.drupal.org/node/2877528). When either of those issues is
- // fixed, remove the if-test and the 'else' block.
- if (strpos($this->entity->getEntityType()->getLinkTemplate('canonical'), '/admin/') !== 0) {
- $this->assertTrue($response->hasHeader('X-Drupal-Dynamic-Cache'));
- $this->assertSame(['MISS'], $response->getHeader('X-Drupal-Dynamic-Cache'));
- }
- else {
- $this->assertFalse($response->hasHeader('X-Drupal-Dynamic-Cache'));
- }
+ $this->assertTrue($response->hasHeader('X-Drupal-Dynamic-Cache'));
+ $this->assertSame(['MISS'], $response->getHeader('X-Drupal-Dynamic-Cache'));
if (!$this->account) {
$this->assertSame(['MISS'], $response->getHeader('X-Drupal-Cache'));
}
@@ -407,50 +397,40 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
// Same for Dynamic Page Cache hit.
$response = $this->request('GET', $url, $request_options);
$this->assertResourceResponse(200, FALSE, $response);
- // @todo Entity resources with URLs that begin with '/admin/' are marked as
- // administrative (see https://www.drupal.org/node/2874938), which
- // excludes them from Dynamic Page Cache (see
- // https://www.drupal.org/node/2877528). When either of those issues is
- // fixed, remove the if-test and the 'else' block.
- if (strpos($this->entity->getEntityType()->getLinkTemplate('canonical'), '/admin/') !== 0) {
- $this->assertTrue($response->hasHeader('X-Drupal-Dynamic-Cache'));
- if (!static::$auth) {
- $this->assertSame(['HIT'], $response->getHeader('X-Drupal-Cache'));
- $this->assertSame(['MISS'], $response->getHeader('X-Drupal-Dynamic-Cache'));
- }
- else {
- $this->assertFalse($response->hasHeader('X-Drupal-Cache'));
- $this->assertSame(['HIT'], $response->getHeader('X-Drupal-Dynamic-Cache'));
- // Assert that Dynamic Page Cache did not store a ResourceResponse object,
- // which needs serialization after every cache hit. Instead, it should
- // contain a flattened response. Otherwise performance suffers.
- // @see \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::flattenResponse()
- $cache_items = $this->container->get('database')
- ->query("SELECT cid, data FROM {cache_dynamic_page_cache} WHERE cid LIKE :pattern", [
- ':pattern' => '%[route]=rest.%',
- ])
- ->fetchAllAssoc('cid');
- $this->assertCount(2, $cache_items);
- $found_cache_redirect = FALSE;
- $found_cached_response = FALSE;
- foreach ($cache_items as $cid => $cache_item) {
- $cached_data = unserialize($cache_item->data);
- if (!isset($cached_data['#cache_redirect'])) {
- $found_cached_response = TRUE;
- $cached_response = $cached_data['#response'];
- $this->assertNotInstanceOf(ResourceResponseInterface::class, $cached_response);
- $this->assertInstanceOf(CacheableResponseInterface::class, $cached_response);
- }
- else {
- $found_cache_redirect = TRUE;
- }
- }
- $this->assertTrue($found_cache_redirect);
- $this->assertTrue($found_cached_response);
- }
+ $this->assertTrue($response->hasHeader('X-Drupal-Dynamic-Cache'));
+ if (!static::$auth) {
+ $this->assertSame(['HIT'], $response->getHeader('X-Drupal-Cache'));
+ $this->assertSame(['MISS'], $response->getHeader('X-Drupal-Dynamic-Cache'));
}
else {
- $this->assertFalse($response->hasHeader('X-Drupal-Dynamic-Cache'));
+ $this->assertFalse($response->hasHeader('X-Drupal-Cache'));
+ $this->assertSame(['HIT'], $response->getHeader('X-Drupal-Dynamic-Cache'));
+ // Assert that Dynamic Page Cache did not store a ResourceResponse object,
+ // which needs serialization after every cache hit. Instead, it should
+ // contain a flattened response. Otherwise performance suffers.
+ // @see \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::flattenResponse()
+ $cache_items = $this->container->get('database')
+ ->query("SELECT cid, data FROM {cache_dynamic_page_cache} WHERE cid LIKE :pattern", [
+ ':pattern' => '%[route]=rest.%',
+ ])
+ ->fetchAllAssoc('cid');
+ $this->assertCount(2, $cache_items);
+ $found_cache_redirect = FALSE;
+ $found_cached_response = FALSE;
+ foreach ($cache_items as $cid => $cache_item) {
+ $cached_data = unserialize($cache_item->data);
+ if (!isset($cached_data['#cache_redirect'])) {
+ $found_cached_response = TRUE;
+ $cached_response = $cached_data['#response'];
+ $this->assertNotInstanceOf(ResourceResponseInterface::class, $cached_response);
+ $this->assertInstanceOf(CacheableResponseInterface::class, $cached_response);
+ }
+ else {
+ $found_cache_redirect = TRUE;
+ }
+ }
+ $this->assertTrue($found_cache_redirect);
+ $this->assertTrue($found_cached_response);
}
$cache_tags_header_value = $response->getHeader('X-Drupal-Cache-Tags')[0];
$this->assertEquals($this->getExpectedCacheTags(), empty($cache_tags_header_value) ? [] : explode(' ', $cache_tags_header_value));
diff --git a/core/modules/system/src/EventSubscriber/AdminRouteSubscriber.php b/core/modules/system/src/EventSubscriber/AdminRouteSubscriber.php
index b83fe7e..50b3a57 100644
--- a/core/modules/system/src/EventSubscriber/AdminRouteSubscriber.php
+++ b/core/modules/system/src/EventSubscriber/AdminRouteSubscriber.php
@@ -4,10 +4,11 @@ namespace Drupal\system\EventSubscriber;
use Drupal\Core\Routing\RouteSubscriberBase;
use Drupal\Core\Routing\RoutingEvents;
+use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
/**
- * Adds the _admin_route option to each admin route.
+ * Adds the _admin_route option to each admin HTML route.
*/
class AdminRouteSubscriber extends RouteSubscriberBase {
@@ -16,7 +17,7 @@ class AdminRouteSubscriber extends RouteSubscriberBase {
*/
protected function alterRoutes(RouteCollection $collection) {
foreach ($collection->all() as $route) {
- if (strpos($route->getPath(), '/admin') === 0 && !$route->hasOption('_admin_route')) {
+ if (strpos($route->getPath(), '/admin') === 0 && !$route->hasOption('_admin_route') && static::isHtmlRoute($route)) {
$route->setOption('_admin_route', TRUE);
}
}
@@ -36,4 +37,19 @@ class AdminRouteSubscriber extends RouteSubscriberBase {
return $events;
}
+ /**
+ * Determines whether the given route is a HTML route.
+ *
+ * @param \Symfony\Component\Routing\Route $route
+ * The route to analyze.
+ *
+ * @return bool
+ * TRUE if HTML is a valid format for this route.
+ */
+ protected static function isHtmlRoute(Route $route) {
+ // If a route has no explicit format, then HTML is valid.
+ $format = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : ['html'];
+ return in_array('html', $format, TRUE);
+ }
+
}
diff --git a/core/modules/system/tests/src/Unit/Routing/AdminRouteSubscriberTest.php b/core/modules/system/tests/src/Unit/Routing/AdminRouteSubscriberTest.php
new file mode 100644
index 0000000..153391c
--- /dev/null
+++ b/core/modules/system/tests/src/Unit/Routing/AdminRouteSubscriberTest.php
@@ -0,0 +1,70 @@
+<?php
+
+namespace Drupal\Tests\system\Unit\Routing;
+
+use Drupal\Core\Routing\RouteBuildEvent;
+use Drupal\system\EventSubscriber\AdminRouteSubscriber;
+use Drupal\Tests\UnitTestCase;
+use Symfony\Component\Routing\Route;
+use Symfony\Component\Routing\RouteCollection;
+
+/**
+ * @coversDefaultClass \Drupal\system\EventSubscriber\AdminRouteSubscriber
+ * @group system
+ */
+class AdminRouteSubscriberTest extends UnitTestCase {
+
+ /**
+ * @covers ::alterRoutes
+ * @covers ::isHtmlRoute
+ *
+ * @dataProvider providerTestAlterRoutes
+ */
+ public function testAlterRoutes(Route $route, $is_admin) {
+ $collection = new RouteCollection();
+ $collection->add('the_route', $route);
+ (new AdminRouteSubscriber())->onAlterRoutes(new RouteBuildEvent($collection));
+
+ $this->assertSame($is_admin, $route->getOption('_admin_route'));
+ }
+
+ public function providerTestAlterRoutes() {
+ $data = [];
+ $data['non-admin'] = [
+ new Route('/foo'),
+ NULL,
+ ];
+ $data['admin prefix'] = [
+ new Route('/admin/foo'),
+ TRUE,
+ ];
+ $data['admin option'] = [
+ (new Route('/foo'))
+ ->setOption('_admin_route', TRUE),
+ TRUE,
+ ];
+ $data['admin prefix, non-HTML format'] = [
+ (new Route('/admin/foo'))
+ ->setRequirement('_format', 'json'),
+ NULL,
+ ];
+ $data['admin option, non-HTML format'] = [
+ (new Route('/foo'))
+ ->setRequirement('_format', 'json')
+ ->setOption('_admin_route', TRUE),
+ TRUE,
+ ];
+ $data['admin prefix, HTML format'] = [
+ (new Route('/admin/foo'))
+ ->setRequirement('_format', 'html'),
+ TRUE,
+ ];
+ $data['admin prefix, multi-format including HTML'] = [
+ (new Route('/admin/foo'))
+ ->setRequirement('_format', 'json|html'),
+ TRUE,
+ ];
+ return $data;
+ }
+
+}