Make access checkers (much) easier to find

For everything else we use the plugin system. We could cover both cases in a more performant, more DX friendly way: simply the access checker plugin IDs in an access section of the route and be done. For the dynamic case, we can keep an applies() method which in a base class is implemented simply as return TRUE.

We currently have no plugins inside the routing layer. Which parts are involved in the routing layer which could be plugins as well:

• Breadcrumbs
• Access checker
• Route enhancer
• Path processing
• Route subscriber (potentially)

I agree that some of these examples are not implemented that often though also breadcrumbs are potentially valuable as plugins.
If we move to plugins on access checkers we are less consistent inside the routing system but maybe more consistent throughout core.

• implement the StaticAccessCheckInterface. You can add the string returned by the appliesTo to your route definition requirements key.
• implement AccessCheckInterface for dynamic checking. These all need to be fired runtime.

So when the routes are compiled (added to the router table) each route store which access checkers apply to them. This adds
potentially N (count of times) times M (count of access checkers) calls to the applies() method. Static access checkers don't have an apply method so they don't have to be run.
AccessCheckInterface access checkers applies() method are also just executed on the COMPILE TIME.

Disagree. Plugins are a good fit when you have:

1) Multiple instances of an object
2) Configured by a user
3) Through the UI

Access checkers, route enhancers, path processors, breadcrumbs... *none* of those criteria apply to any of those systems. One could have, say, a breadcrumb builder that is backed by user configuration, and that particular builder could leverage plugins internally if it makes sense to. But none of these systems "fit" the plugin use case.

"For everything else we use plugins" is simply not true.

Frequency of implementation has nothing to do with it whatsoever.

I'm not comfortable this being listed as a critical DX task until / unless the issue summary is updated to explain the before/after and how this would be beneficial to people learning Drupal.

At a glance, though, it sounds like it might be more confusing, because it's broadening the definition of what plugins means to encompass things that don't fit its current definition, if #2 is valid.

Splitting the difference at normal, and re-adding the tag.

Updated the issue summary.

If you update the issue summary please take the last statement of #1 into account.

Updated issue summary.

Thank you very much!

Updated issue summary.

Plugins are a good fit when you have:
1) Multiple instances of an object
2) Configured by a user
3) Through the UI

I am assuming that is an OR list, but even then we have a couple of plugin types in core that are a bad fit, in case that statement is true:
* Local tasks
* Local actions
* Contextual links
There are a couple others that are less obvious violations of the above, so I'm not bringing them up here to avoid derailing the discussion.

There is a third option for how access checkers can be defined: by just giving it a class::method pair.

Removed incorrect statement. (The _custom_access approach doesn't require a service to be registered.)

With _custom_access, is this even necessary anymore? And what would be the performance impact of going through plugins for every request? Plus on link generation, which is already too slow...

tstoeckler: No, it was an AND list, or maybe at best a "2 of 3". Things like tabs as plugins are a bit odd, but there was no better alternative. We have a viable alternative for access control now, which keeps the system loosely coupled.

Given how many hundreds of hours we've poured into decoupling systems in Drupal 8, and hiding details behind sane and purpose-built interfaces, I'm very reluctant to introduce more coupling unless there's a very very strong need. I don't see that strong need here.

Working on this.

Symfony has native support for attributes on tags, I chose to use that instead of adding a top-level key to the definition.
It still supports everything we need.

I like this. It does look cleaner, and doesn't increase coupling with any other systems. (Well, maybe with the DIC but that's incidental and you can still use the code without it.) +1.

Damien did most of the work on StaticAccessCheckInterface so I really want his RTBC on this.

My implementation pretty much looks the same I coded offline.

Here are some small adapations.

* We don't need to support multiple ones ...
@tim Deleting a file is often just wrong, but I understand why you did that ... we have an issue to fix that already: https://drupal.org/node/1912602

Maybe this works now ...

1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -117,12 +117,13 @@ public function setRequest(Request $request) {
   -    foreach ($applies_checks as $applies_check) {
   +
   +    if ($applies_check) {
   
   +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterAccessChecksPass.php
   @@ -26,13 +26,14 @@ public function process(ContainerBuilder $container) {
   +          $applies = $attribute['applies_to'];
   

   This is how they work. They are arrays, even when there is only one.
   EDIT: I mean, this has to be reverted. It's just how Symfony does attributes in tagged services

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterAccessChecksPass.php
   @@ -26,13 +26,14 @@ public function process(ContainerBuilder $container) {
        foreach ($container->findTaggedServiceIds('access_check') as $id => $attributes) {
   ...
   +        if ($attribute['name'] == 'access_check' &&  isset($attribute['applies_to'])) {
   

   When will the name not be 'access_check'? That's what this does, AFAIK.

This uses the applies_to and the correct changes to views that @dawehner had, but otherwise is my original patch.
It also makes the second param to addCheckService() optional.

Yeah I had a little bit of a rage mode there.

1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -328,12 +332,11 @@ protected function loadCheck($service_id) {
      public function loadAccessRequirementMap() {
   

   This method might as well be renamed I guess.

2. +++ b/core/lib/Drupal/Core/Access/CustomAccessCheck.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Routing\Access\AccessInterface as RoutingAccessInterface;
   
   @@ -22,7 +23,7 @@
   +class CustomAccessCheck implements RoutingAccessInterface {
   

   Do we need to alias these? There is currently a mixture of both.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterAccessChecksPass.php
   @@ -26,7 +26,13 @@ public function process(ContainerBuilder $container) {
   +      $access_manager->addMethodCall('addCheckService', array($id, array_unique($applies)));
   

   Is this a bit too paranoid? Someone would have to register the same line twice in a YAML file? We don't do this checking in other places.

The only other thing that I would say is a 'concern' is having to do this:

    tags:
      - { name: access_check, applies: _access_my_checker }
      - { name: access_check, applies: _access_my_checker_alternate }

It just looks weird having to specify the name each time to be honest.

1) To what?
2) Yes, we do when the access checker is in Drupal\Core\Access
3) I'm not usually one for paranoia. You mean the array_unique? Doesn't bother me either way.

And yes, that is weird, but it is a) how it works in Symfony b) not used really in core.

1. Something with the word dynamic in? Not too bothered though really.
2. Ha, yes of course! Then another question, is it weird to have checkers in Core\Access using an interface in Core\Routing\Access?
3. Yep, the array_unique. Seems unnecessary.
4. I guess it's not a common case, so meh. Not the end of the world by any means.

I don't really see how this patch makes it that much easier to find access checkers but i can see the logic of looking in yaml. Plus does have some nice cleanups in.

Borrowing a oneline fix from #1912602: Changing view access from "Permission" to "Role" causes AJAX error message re getRoles()

1) Renamed to match the protected property
2) Yes it is. Blame Xano (#2095125: Use access constants in every access control context)
3) If this causes a bug later on, it's on you :)

Yes, it is easier. Only if you know where to look though :) That is why I said it is not that much easier. I do agree it *is* easier though.

1. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -118,9 +117,14 @@ public function setRequest(Request $request) {
   +   *   (optional) An array of static checks for a route to apply to.
   

   This doc doesn't sound right. Doesn't it want to be something like "An array of route requirement keys the check service applies to", or something...

2. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -341,14 +344,8 @@ public function loadAccessRequirementMap() {
   -      if (is_subclass_of($this->checks[$service_id], 'Drupal\Core\Access\StaticAccessCheckInterface')) {
   ...
   +      if ($this->checks[$service_id] instanceof AccessCheckInterface) {
   

   We check the AccessCheckInterface for dynamic checkers but nothing for static checkers anymore. Do you think we should do this too?

3) If this causes a bug later on, it's on you :)

Ha, fair enough :) However, if we are wanting to make sure this isn't a problem, the RegisterAccessChecksPass is not the place to do this, seems like it should be in the access manager itself. If anywhere.

We check the AccessCheckInterface for dynamic checkers but nothing for static checkers anymore. Do you think we should do this too?

I don't know what to check actually.

Rerolled. Plus, can't we just add it here to loadCheck() as all checkers will have to implement this interface in some form? That's what I was getting at in my comment about the checking.

+1 for that change.

44: 2109035-44.patch queued for re-testing.

Reroll

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -331,19 +337,24 @@ protected function loadCheck($service_id) {
+      throw new AccessException('All access checks must implement AccessInterface.');

The docblock for AccessException says:

"An exception thrown for invalid access callback return values."

But that's not how it's being used here. Either we need to use a new exception class or update the docblock of AccessException. I don't have a strong preference at the moment.

Other than that nit, I'm good here. Go ahead and mark RTBC when fixing the above.

Rerolled, and changed that docbock for AccessException.

Works for me. Thanks.

Wow, this is a great change!

- 63 files changed, 231 insertions, 435 deletions FTW
- I also have no idea what a StaticAccessCheckerInterface is, so happy to see that removed in favour of something more general
- I also like the idea of encoding route relationships into the route definitions vs. having to open a separate file to get those

Coupled with the improved "greppability" defined in the issue summary, this patch looks like a big DX win to me!

Committed and pushed to 8.x. Thanks!

We need to update the existing change notices and documentation under https://drupal.org/node/2122071 now.

Updated the docs in https://drupal.org/node/2122195

Made a slight tweak to the change notice. Looks good now.

Updating tags and title.

+++ b/core/modules/aggregator/aggregator.services.yml
@@ -8,3 +8,11 @@ services:
+  access_check.aggregator.categories:
+    class: Drupal\aggregator\Access\CategoriesAccessCheck
+    arguments: ['@database']
+    tags:
+      - { name: access_check, applies_to: _access_aggregator_categories }
+  aggregator.category.storage:
+    class: Drupal\aggregator\CategoryStorageController
+    arguments: ['@database']

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Access/CategoriesAccessCheck.php
@@ -0,0 +1,45 @@
+<?php
...
+}
...
+/**
...
+
...
+ * Contains \Drupal\aggregator\Access\CategoriesAccess.
...
+ * @file
...
+ */
+
+namespace Drupal\aggregator\Access;
...
+use Drupal\Core\Database\Connection;
...
+
...
+use Drupal\Core\Routing\Access\AccessInterface;
+use Drupal\Core\Session\AccountInterface;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\Route;
+
+/**
+ * Provides an access check for aggregator categories routes.
+ */
+class CategoriesAccessCheck implements AccessInterface {
...
+  /**
...
+
...
+   * The database connection.
...
+   * @var \Drupal\Core\Database\Connection
...
+   *
...
+   */
+  protected $database;
+
+  /**
...
+   *
...
+   *   The database connection.
...
+   * Constructs a CategoriesAccessCheck object.
...
+   * @param \Drupal\Core\Database\Connection
...
+   */
...
+    $this->database = $database;
...
+  public function __construct(Connection $database) {
...
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function access(Route $route, Request $request, AccountInterface $account) {
+    return $account->hasPermission('access news feeds') && (bool) $this->database->queryRange('SELECT 1 FROM {aggregator_category}', 0, 1)->fetchField() ? static::ALLOW : static::DENY;
+  }
+

junk
Opened #2187313: Remove reintroduced category code in aggregator