Make defining bundle-specific routes easier

Here's a proof of concept.

New access checker - looks for an '_entity_bundle' route requirement.

The route requirement looks like:

  requirements:
    _entity_bundle: 'entity_type:bundle'

Updated for #8.

+++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
@@ -0,0 +1,52 @@
+        $entity = $parameter->get($entity_type);

typo: $parameters

This is definitely missing test coverage ;)

Also, I was wondering whether we will ever need something like this to act on a route having the bundle name as parameter rather than an entity.

I just realised the patch doesn't seem to allow for multiple bundles.

I initially tried an array, but route requirements can only be a string. We could do some custom serialization, but that gets ugly quickly, so seems better to keep this simple since a single bundle route is the most likely use-case here. Most modules either deal with one bundle, or rely on per-bundle configuration (forum, book) which can't be expressed in routing YAML and would require a custom access check anyway.

I was wondering whether we will ever need something like this to act on a route having the bundle name as parameter rather than an entity.

When the route has a bundle name, this could be handled with a route filter instead of an access check. The reason I had to do the access check here is because we only know the bundle once we get the entity, but if the bundle is in the request, it can be done in a filter during route selection. So this would probably need to be a different requirement with a different syntax.

Will add some tests.

Now with test coverage.

This looks like a nice little feature request for custom code!

1. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,52 @@
   +class EntityBundleAccessCheck implements AccessInterface {
   

   I'm impressed how bad the documentation for \Drupal\Core\Routing\Access\AccessInterface actually is :)

2. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,52 @@
   +   *   path: foo/{entity_type}/{example}
   +   *   requirements:
   +   *     _entity_bundle: 'example_entity_type:example_bundle'
   

   Reading this example, I'd suggest to do something like:

   path: foo/{example_entity_type}/{other_parameter}
   to make it more clear how this works.

3. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,52 @@
   +        if ($entity instanceof EntityInterface) {
   +          if ($entity->bundle() !== $bundle) {
   +            return AccessResult::forbidden();
   +          }
   +        }
   

   Nitpick: It feels like the code would be a bit easier to read if this two nested ifs would be combined like if ($entity instanceof EntityInterface && $entity->bundle !== $bundle)

#14 makes sense, updated for that.

Looks good to me, I have just a couple of minor remarks:

1. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,50 @@
   +   * example.route:
   +   *   path: foo/{entity_type}/{other_parameter}
   +   *   requirements:
   +   *     _entity_bundle: 'example_entity_type:example_bundle'
   

   As @dawehner mentioned, it would be good for the example path to be foo/{example_entity_type}/{other_parameter}, to make it clear that the requirement value must match it.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityBundleAccessCheckTest.php
   @@ -0,0 +1,84 @@
   +  public function testAccessMatchingRoute() {
   ...
   +  public function testAccessNonMatchingRoute() {
   

   We could use a data provider to avoid duplicating the test code.

Addressing #16.

Arggh - with proper indentation.

Looks good!

1. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,50 @@
   +      if ($parameters->has($entity_type)) {
   

   If this if is FALSE is returning a neutral result correct? If the route has the _entity_bundle requirement and there is no entity parameter then I think this should be forbidden. Not sure though.

2. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,50 @@
   +        if ($entity instanceof EntityInterface && $entity->bundle() !== $bundle) {
   

   I think there is some could somewhere that changes an entity's bundle - at least in contrib - do we need to care about cacheability here?

3. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,50 @@
   +          return AccessResult::forbidden();
   

   We can provide a reason here.

#1. I'm also not sure. The most likely case that would happen would be a mis-use of this feature somehow, say a c&p error. I thought about throwing an exception there too. Don't have a strong opinion.

#2. It does exist, doesn't hurt to add it. https://www.drupal.org/project/convert_bundles

#3. Yes we should add this.

So... could do with a third opinion on #1.

20.1 Route access checks are AND-combined, so there is very little difference between neutral and forbidden. All access checks must return allowed or access is denied.

@catch:

+++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
@@ -0,0 +1,50 @@
+  public function access(Route $route, RouteMatchInterface $route_match, AccountInterface $account) {
+    if ($route->hasRequirement('_entity_bundle')) {
+      list($entity_type, $bundle) = explode(':', $route->getRequirement('_entity_bundle'));
+      $parameters = $route_match->getParameters();
+      if ($parameters->has($entity_type)) {
+        $entity = $parameters->get($entity_type);
+        if ($entity instanceof EntityInterface && $entity->bundle() !== $bundle) {
+          return AccessResult::forbidden();
+        }
+      }
+    }
+    return AccessResult::neutral();

Given the comments above, maybe it would make more sense to reverse the logic and always return AccessResult::forbidden() unless $entity instanceof EntityInterface && $entity->bundle() === $bundle?

I got back to #11 with @catch since in our project we will likely need also multiple bundles. I suggested the following custom serialization which @catch didn't dislike: _entity_bundle: node:article|page|news. This seems a good combination of readability and ease of implementation to me.

One reason why it would make sense to support this custom format is that, if support for multiple requirements is ever introduced, it would still require to prefix each requirement with the same entity type:

- node:article
- node:page
- node:news

Additionally, if we adopt this custom format, we would be able to do this:

- node:article|page|news
- comment:article|page|news

if the route supports multiple entity types at once.

Yes I really like the format in #25, it's fairly self-explanatory and doesn't require duplicating the entity type which was my main objection to trying to support multiples.

Here's an updated patch addressing #20, #24, and #25. Excuse the double interdiff I missed a bit..

1. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,52 @@
   +   *     _entity_bundles:
   +   *     'example_entity_type:example_bundle|other_example_bundle'
   

   Can we keep these on the same line, otherwise it might be confusing.

2. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,52 @@
   +          return AccessResult::neutral()->addCacheableDependency($entity);
   
   +++ b/core/tests/Drupal/Tests/Core/Entity/EntityBundleAccessCheckTest.php
   @@ -0,0 +1,89 @@
   +        AccessResult::neutral(),
   ...
   +        AccessResult::neutral(),
   

   Per #23 shouldn't these be ::allowed()?

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityBundleAccessCheckTest.php
   @@ -0,0 +1,89 @@
   +      [
   +        'page',
   +        'node:article|page',
   ...
   +      ],
   

   What about adding another equivalent test case having article as bundle?

Addressing #27.

Looks good, thanks, back to RTBC!

It would be good to provide a CR and an IS update.

Failed to update the actual service definition.

Also there might be a core use-case for this.

If you add a view as a tab on an entity type, then even if you add validation by the entity bundle and restrict it, the tab shows up for all bundles (leading to a 404). This is because argument validation isn't access.

So we could add a views access handler that specifies the bundles of the entity type it appears on maybe.

I think #34 is follow-up material, back to RTBC.

Added the follow-up.

1. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,51 @@
   + * Provides an access checker for the _entity_bundles route requirement.
   ...
   +   * Checks access based on the _entity_bundles route requirement.
   

   This does not really explain the intent of this route requirement. Given it's name, you could most likely guess it, but we could probably be more explicit.

2. +++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
   @@ -0,0 +1,51 @@
   +   *   The parametrized route
   

   Nit: Missing a "."

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityBundleAccessCheckTest.php
   @@ -0,0 +1,94 @@
   +  function getBundleAndAccessResult() {
   

   Nice, to make it complete we could add one more, testing multiple bundles that should be forbidden as well.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityBundleAccessCheckTest.php
@@ -0,0 +1,94 @@
+  function getBundleAndAccessResult() {

Missing visibility on the method.

+++ b/core/lib/Drupal/Core/Entity/EntityBundleAccessCheck.php
@@ -0,0 +1,51 @@
+          return AccessResult::allowed()->addCacheableDependency($entity);
...
+    return AccessResult::forbidden('The entity bundle does not match the route _entity_bundles requirement.');

Per #23 shouldn't this be allowed and neutral instead of allowed and forbidden?

I've addressed comments 37,39 and 40. On top of that I removed two unused uses that were added by this patch.

Committed and pushed 5def5fc1d2 to 9.0.x and 4b5f1ac075 to 8.9.x. Thanks!

Thanks for this!

I was a bit disappointed to discover that this wouldn't let me add node bundle requirements to a Views-created menu tab, e.g.,

protected function alterRoutes(RouteCollection $collection) {
  // Add a requirement for tab to be only on nodes of bundle_a and bundle_b.
  if ($route = $collection->get('view.view_machine_name.display_machine_name')) {
    $route->setRequirement('_entity_bundles', 'node:bundle_a|bundle_b');
  }
}

It did, however, let me make a custom route with the correct bundle requirements. I set the view (a page display) to "No menu" and then created my own route for it.

my_module.routing.yml:

my_module.node.subscribers:
  path: /node/{node}/subscribers
  requirements:
     _entity_bundles: 'node:bundle_a|bundle_b'
  options:
    parameters:
      node:
        type: entity:node

my_module.links.task.yml:

my_module.node.subscribers:
  title: "Subscribers"
  route_name: my_module.node.subscribers
  base_route: entity.node.canonical
  weight: 15

Now I have a "Subscribers" tab on nodes of two different bundle types but no others.

In order to then get the node tabs to show up on my view page:

use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Cache\RefinableCacheableDependencyInterface;

  /**
   * Implements hook_menu_local_tasks_alter().
   */

  // Show the same local tasks on the subscribers page as on node canonical pages.
  function my_module_menu_local_tasks_alter(&$local_tasks, $route_name, RefinableCacheableDependencyInterface &$cacheability) {
    if ($route_name === 'view.view_machine_name.display_machine_name') {
      $node_canonical_local_tasks = \Drupal::service('plugin.manager.menu.local_task')
        ->getLocalTasks('entity.node.canonical');
      $local_tasks['tabs'][0] = $node_canonical_local_tasks['tabs'];
      // Mark the subscribers tab as active when viewing it.
      foreach ($local_tasks['tabs'][0] as $route_name => &$tab) {
        $tab['#active'] = $route_name === 'my_module.node.subscribers';
      }
      $cacheability = CacheableMetadata::createFromObject($cacheability)
        ->merge($node_canonical_local_tasks['cacheability']);
    }
  }

We are deprecating this route requirement in #3155568: Filter by bundle in EntityConverter route param converter (see comment #11)