Issue #2874938 by Wim Leers, tim.plunkett, borisson_, effulgentsia:...

Issue #2874938 by Wim Leers, tim.plunkett, borisson_, effulgentsia: AdminRouteSubscriber must only mark HTML routes as administrative

core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php

@@ -383,18 +383,8 @@ public function testGet() {
     // 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 @@ public function testGet() {
     // 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));

core/modules/system/src/EventSubscriber/AdminRouteSubscriber.php

@@ -4,10 +4,11 @@
 
 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 @@ public static function getSubscribedEvents() {
     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);
+  }
+
 }

core/modules/system/tests/src/Unit/Routing/AdminRouteSubscriberTest.php

+<?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;
+  }
+
+}