Convert hook_admin_paths() into declarative properties on routes

Seems to make a lot of sense to me. Does anyone know if there is a technical reason the functions were spilt?

This make lot more sens than the hook_admin_paths. With hook_admin_paths I can find a way to create set admin theme to false only for a content type on edit...

That sounds good, but we currently do *not* save arbitrary metadata attached to menu routers. I would definitely support doing so.

What determines whether a path is an "admin path" though? I'd say that's up to the menu's user_access() callback to decide. We should get rid of the hook entirely and use the menu's user_access() context.

admin path mean you use the admin theme it have nothing to do with access. So what about using a theme key.

/**
* Implements hook_menu().
*/
function user_menu() {
  $items['user/%user/edit'] = array(
    'title' => 'Edit',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('user_profile_form', 1),
    'access callback' => 'user_edit_access',
    'access arguments' => array(1),
    'theme' => admin,        //   <- this line has been added
    'type' => MENU_LOCAL_TASK,
    'file' => 'user.pages.inc',
  );
}

So in the theme key you can chose a theme or use "admin" to chose the admin theme and "default" for the default one (or just not give the key)

Related issue: #1032700-27: Add caching to hook_admin_paths()

In D8, we should convert these declarations into properties on routes, and only keep the alter hook (or equivalent) for dynamic or page/route-specific adjustments.

For info path_is_admin() is now used by the back to site button which replaces the overlay. We rely on it to know if the current page is an admin page or not and display the button appropriately. As long as there is a way to know if current_path() is admin or not we're cool.

I am not really sure how to deal with user/%/translations taxonomy_term/%/translations ... it feels like we might should just set it on all content/config translation routes.

I cannot even remember on how many issues I added that bit.

The last patch seems like a good direction to go in. The issue summary is way out of date though.

I've updated the summary based on the latest patch. This is entirely sensible to me.

Rerolled.

I fixed up a couple things...

It'd be nice to have the internals of path_is_admin moved somewhere, like AdminContext::isAdminPath($path), but I didn't want to make AdminContext dependent on the router...

Also we should probably go ahead and make an interface for AdminContext...

The new d.o comment form is awesome!

Except the part where it misattributes stuff. #19 and #20 are tim.plunkett.

Okay, had to add code to handle the node setting.

Wrong interface!

25: admin-paths-1316692-25.patch queued for re-testing.

If LanguageRequestSubscriber runs before the router, it doesn't have the route object available, and then LanguageNegotiationUserAdmin will break when it calls isAdminRoute().

I'm not sure if there are other service priorities that need to be adjusted, or if this makes a loop between the router and language, but we'll find out soon.

+++ b/core/lib/Drupal/Core/Routing/AdminContext.php
@@ -0,0 +1,64 @@
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents() {
+    $events[RoutingEvents::ALTER] = array('onAlterRoutes', -10);
+    return $events;
+  }

Is that a leftover?

This is unfortunate, but it is only needed because the language *has* to come before the RouterListener. I think we're better off special casing the language here than making the AdminContext class dependent on the router.

33: admin-paths-1316692-33.patch queued for re-testing.

We seriously cannot run the router twice. Do we know whether we actually need to be able to set the language like that?

Trying this just for fun. I think I may have misunderstood @dawehner in IRC, but let's see what fails.

Just an idea: You could try to call LanguageNegotiatorInterface::reset() on the second running listener, this might fix some of the negotiation.

I want to take a look at this again soon.

Reroll of #33, just to get back to passing.

+++ b/core/modules/user/lib/Drupal/user/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
@@ -29,6 +34,46 @@ class LanguageNegotiationUserAdmin extends LanguageNegotiationMethodBase {
+  /**
+   * The router.
+   *
+   * This is only used when called from an event subscriber, before the request
+   * has been populated with the route info.
+   *
+   * @var \Symfony\Component\Routing\Matcher\UrlMatcherInterface
+   */
+  protected $router;
...
+   * @param \Symfony\Component\Routing\Matcher\UrlMatcherInterface $router
+   *   The router.
...
+    $this->router = $router;
...
+      $container->get('router')

@@ -51,17 +96,22 @@ public function getLangcode(Request $request = NULL) {
+      // If called from an event subscriber, the request may not the route info
+      // yet, so use the router to look up the path first.
+      if (!$route_object = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)) {
+        $attributes = $this->router->match('/' . urldecode(trim($request->getPathInfo(), '/')));
+        $route_object = $attributes[RouteObjectInterface::ROUTE_OBJECT];
+      }

These are the parts @dawehner objects to, and I have no idea how to work around.

1. +++ b/core/lib/Drupal/Core/Routing/AdminContext.php
   @@ -0,0 +1,56 @@
   +  public function isAdminRoute(Route $route = NULL) {
   +    if (!$route) {
   +      $route = $this->route;
   +      if (!$route) {
   +        return FALSE;
   +      }
   +    }
   +    return (bool) $route->getOption('_admin_route');
   +  }
   

   This is a service. Pass in the route. Don't provide a stateful, state-changing default. That way lies madness.

2. +++ b/core/lib/Drupal/Core/Routing/RouteSubscriberBase.php
   @@ -25,8 +25,7 @@
   -  protected function alterRoutes(RouteCollection $collection, $provider) {
   -  }
   +  abstract protected function alterRoutes(RouteCollection $collection, $provider);
   

   Why? Does this not force people to implement it?

3. +++ b/core/modules/node/lib/Drupal/node/EventSubscriber/NodeAdminRouteSubscriber.php
   @@ -0,0 +1,49 @@
   +  protected function alterRoutes(RouteCollection $collection, $provider) {
   +    if ($this->configFactory->get('node.settings')->get('use_admin_theme')) {
   +      foreach ($collection->all() as $route) {
   +        if ($route->hasOption('_node_admin_route')) {
   +          $route->setOption('_admin_route', TRUE);
   +        }
   +      }
   +    }
   +  }
   +
   

   Hrm. This makes me wonder if we really do need a route_type or something instead of multiple booleans. I won't block this issue on that question but I think we'll be there soon.

4. +++ b/core/modules/user/lib/Drupal/user/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
   @@ -51,17 +96,22 @@ public function getLangcode(Request $request = NULL) {
   +    if ($request && $this->adminContext) {
   +      // If called from an event subscriber, the request may not the route info
   +      // yet, so use the router to look up the path first.
   +      if (!$route_object = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)) {
   +        $attributes = $this->router->match('/' . urldecode(trim($request->getPathInfo(), '/')));
   +        $route_object = $attributes[RouteObjectInterface::ROUTE_OBJECT];
   +      }
   +      $result = $this->adminContext->isAdminRoute($route_object);
   

   I agree this feels very wrong. The problem is that this class is depending on where in the request it's called. That suggests the problem is elsewhere; eg, why is this class being called from unknown places? Should it be different classes then?

The original rationale for a hook was performance (https://drupal.org/node/517688?page=1#comment-2171240).

Does the patch here avoid that by removing the path_get_admin_paths() functionality altogether? ... So would something like Overlay be impossible in Drupal 8?

No, it uses routes instead of paths. For admin paths it sets a flag during compile-time of the routes, so there is no performance needed during runtime.

#44.1
Yes, it's a service now, but it probably doesn't need to be. I think it could actually build on top of #2103301: Add a helper class to introspect a given request nicely...

#44.2
RouteSubscriberBase has devolved into a base class that is only useful for those wanting to alter routes. But that said, I don't know why @dawehner made that change here.

#44.3
In all of core and contrib, this is the only other very common pattern (did the user choose to use the admin theme for content creation).
And classes like this will all boil it down to _admin_route anyway.

#44.4

eg, why is this class being called from unknown places

The places are not unknown. In fact, I state right here where the problem is: If called from an event subscriber
LanguageNegotiationUserAdmin determines what language to use, which is required by the URL generator any other early services (think /de/admin/content), and yet LanguageNegotiationUserAdmin also needs to know if this is an admin path.

So what do we do? How do we break that circle?

This is just a reroll of #42 to keep it fresh, no changes.

@David the reason there's no longer a performance penalty is because we already have that penalty - routes have to be loaded to generate internal URLs, which is exactly what the hook was avoiding in 7.x. #2058845: Pre-load non-admin routes and #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees should claw some of that back though.

@David the reason there's no longer a performance penalty is because we already have that penalty - routes have to be loaded to generate internal URLs, which is exactly what the hook was avoiding in 7.x

Ah, thanks, that's what I was missing. It looks like even calling url('some/path') now loads the route...

So assuming that's there to stay, I agree there's no performance hit on top of it here, nor any need for path_get_admin_paths() to stay (which would presumably be a big performance hit if it did because it would need to load, well, everything)... modules which want to identify every admin path link on the current page should have other ways to do that going forward if the route is already available when the link is generated.

Yeah, in case we get the other issue in we can easily reiterate.

Patch context conflict with #2219891: Duplicate declaration of hook_menu_link_defaults(), no changes.

back to RTBC

+++ b/core/modules/node/lib/Drupal/node/EventSubscriber/NodeAdminRouteSubscriber.php
@@ -0,0 +1,49 @@
+  protected function alterRoutes(RouteCollection $collection, $provider) {
+    if ($this->configFactory->get('node.settings')->get('use_admin_theme')) {
+      foreach ($collection->all() as $route) {
+        if ($route->hasOption('_node_admin_route')) {
+          $route->setOption('_admin_route', TRUE);
+        }
+      }
+    }
+  }
+
+}

It's a bit minor but could this use something like _node_operation_route instead of _node_admin_route? I didn't immediately figure out why we need both _admin_route and _node_admin_route. Cannot think of another way to do it that's better than this, but something without 'admin' would make it clearer that this might or might not be treated as administrative depending on the site.

Fine by me. And _node_operation_route is only converted to _admin_route when configured to, otherwise its non-admin. Makes sense.

+1 for the better name.

I committed/pushed this to 8.x, then ten seconds later remembered to look for a draft change notice, which this does not have. So doing old style I guess..

index 802d143..c881a36 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -276,6 +276,10 @@ services:
     class: Symfony\Component\Routing\RequestContext
     calls:
       - [fromRequest, ['@request']]
+  router.admin_context:
+    class: Drupal\Core\Routing\AdminContext
+    calls:
+      - [setRequest, ['@?request=']]
   router.route_provider:
     class: Drupal\Core\Routing\RouteProvider
     arguments: ['@database', '@router.builder']

New code should use the request_stack service instead of the request service. Can we have a (trivial) follow-up on that?

also field ui subscriber does not set admin option

Change record created [#2224207]

@znerol
You are totally right here. You can create one and tag it as "Quickfix".

@kim
It would be actually also be great if your example could include an addition to menu links instead of just the removal part of it.

@dawehner Done!

We forgot admin/content and all manage fields / form / display paths, they don't show up in the admin screen.

Filed follow-up for that #2224553: Add _admin_route for field manager and admin/content

New follow-up for translations paths #2225577: Add _admin_route for translations

Follow up for the request to request_stack conversion: #2225539: Use request stack in admin context service