Problem/Motivation

Over at #2429617-303: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), failures in CommentNonNodeTest were discovered.

(The SmartCache patch caches all pages, with some exceptions. One of those exceptions: any admin route will not be cached.)

They happened were because entity_test/structure is actually an admin route, but wasn't marked as such because it doesn't have the admin/ prefix. So, changing the system path for that route to have that prefix fixes the problems, because those routes are then correctly marked as admin routes.

This was introduced several years ago, in "the early new routing system days". I checked with Berdir, and he confirms/thinks that was simply an oversight.

Proposed resolution

Fix that oversight: make that route have the admin/ prefix in its system path.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only changes tests.
Disruption Contrib modules that use this same route in tests will need to be updated, but only if they weren't using the route name already.
CommentFileSizeAuthor
#18 2551429-18.patch2.53 KBWim Leers
#13 2551429-13.patch2.71 KBstefan.r
#11 interdiff.txt791 bytesWim Leers
#11 2551429-11.patch1.69 KBWim Leers
#10 2551429-10.patch968 bytesWim Leers
#6 interdiff.txt641 bytesWim Leers
#6 entity_test_structure_route_should_be_admin-2551429-6.patch14.18 KBWim Leers
#2 entity_test_structure_route_should_be_admin-2551429-2.patch13.87 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Active » Needs review
FileSize
entity_test_structure_route_should_be_admin-2551429-2.patch13.87 KB

Further indication this was definitely intended to be an admin route:

       $routes["entity.$entity_type_id.admin_form"] = new Route(
-        "$entity_type_id/structure/{bundle}",
+        "admin/structure/$entity_type_id/{bundle}",
         array('_controller' => '\Drupal\entity_test\Controller\EntityTestController::testAdmin'),
         array('_permission' => 'administer entity_test content')
       );

Note the name of the controller.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community

This will break a lot of tests for me in contrib. Anyways good to have this sanity.

 .../modules/comment/src/Tests/CommentNonNodeTest.php | 20 ++++++++++----------
 .../comment/src/Tests/CommentTranslationUITest.php   |  1 +
 .../src/Tests/ConfigTranslationUiTest.php            |  2 +-
 .../src/Tests/ContentTranslationSyncImageTest.php    |  2 +-
 .../modules/datetime/src/Tests/DateTimeFieldTest.php |  4 ++--
 .../src/Tests/EntityReferenceIntegrationTest.php     |  2 +-
 .../field/src/Tests/Boolean/BooleanFieldTest.php     |  6 +++---
 .../field/src/Tests/Number/NumberFieldTest.php       |  2 +-
 core/modules/field_ui/src/Tests/FieldUIRouteTest.php |  2 +-
 .../entity_test/src/Routing/EntityTestRoutes.php     |  2 +-

Beta Eval test only change.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes

Thanks, created an actual beta evaluation.

Also: to avoid your tests from breaking for this, don't use $this->drupalGet('some/path') but $this->drupalGet(Url::fromRoute('some_route').

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: entity_test_structure_route_should_be_admin-2551429-2.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
FileSize
entity_test_structure_route_should_be_admin-2551429-6.patch14.18 KB
interdiff.txt641 bytes

Woops, that included a small unintended hunk.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Can't you just flag the route as admin in the yml and leave the paths as is?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan as a volunteer commented

Eg block/add

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

Let's do less breaking and do what @larowlan suggests

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Rename "$entity_type_id/structure/{bundle}" to "admin/structure/$entity_type_id/{bundle}" » Mark "$entity_type_id/structure/{bundle}" as an admin route
Status: Needs work » Needs review
FileSize
2551429-10.patch968 bytes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
FileSize
2551429-11.patch1.69 KB
interdiff.txt791 bytes

Actually, #10 does NOT work.

Because \Drupal\system\EventSubscriber\AdminRouteSubscriber::alterRoutes():

  protected function alterRoutes(RouteCollection $collection) {
    foreach ($collection->all() as $route) {
      if (strpos($route->getPath(), '/admin') === 0 && !$route->hasOption('_admin_route')) {
        $route->setOption('_admin_route', TRUE);
      }
    }
  }

So this only sets the admin route option for any specific route that has the /admin prefix.

But… we want routes that use the route modified in #10 as a base route, to also be admin routes. Otherwise, we're still not fixing the problem #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) uncovered.

Which means we'd have to modify \Drupal\field_ui\Routing\RouteSubscriber::alterRoutes() to also inherit the options of the base route.

Conclusion

We have two options:

  1. Change the path name, like #6 did. That was considered too disruptive.
  2. Set the _admin_route option, and make Field UI's logic that generates field UI routes inherit the options from the base route it is given. That's what this patch does.
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
+++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
@@ -47,7 +47,7 @@ protected function alterRoutes(RouteCollection $collection) {
-        $options = array();
+        $options = $entity_route->getOptions();

I think if we are not doing this already then this is a bug.
Let's add tests for this.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented
FileSize
2551429-13.patch2.71 KB
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: Mark "$entity_type_id/structure/{bundle}" as an admin route » FieldUI should accommodate route options in RouteSubscriber
Component: entity system » field_ui.module
Category: Task » Bug report

Thanks @stefan.r. This seems ready just needs a test only patch and interdiff :D

+++ b/core/modules/field_ui/src/Routing/RouteSubscriber.php
@@ -47,7 +47,7 @@ protected function alterRoutes(RouteCollection $collection) {
-        $options = array();
+        $options = $entity_route->getOptions();

You can create test only patch by reverting this hunk.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented 
+++ b/core/modules/field_ui/src/Tests/FieldUIRouteTest.php
@@ -112,4 +114,15 @@ public function assertLocalTasks() {
+    $path = '/entity_test/structure/entity_test/fields';
+    $request = Request::create($path);
+    $route_match = \Drupal::service('router.no_access_checks')->matchRequest($request);
+    $route = $route_match[RouteObjectInterface::ROUTE_OBJECT];

Should we not simplify this code a bit and just load the route object itself from the router.route_provider service by its name? Afaik its enough test coverage if you do it like that

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

I probably won't be able to today, can one of you?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented
Status: Needs work » Needs review
FileSize
2551429-18.patch2.53 KB

Done.

Also:

+++ b/core/modules/field_ui/src/Tests/FieldUIRouteTest.php
@@ -112,4 +114,15 @@ public function assertLocalTasks() {
+  public function assertAdminRoute() {

s/assert/test/, otherwise it won't run as a test :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting commented
Status: Needs review » Reviewed & tested by the community

Ha, this is why jibran asked for a test only patch :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 13e93a9 and pushed to 8.0.x. Thanks!

  • alexpott committed 13e93a9 on 8.0.x 
    Issue #2551429 by Wim Leers, stefan.r, jibran, larowlan: FieldUI should...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.