I present as evidence http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul... but there are many others.

core/modules/toolbar/lib/Drupal/toolbar/Access/SubtreeAccess.php:

<?php
/**
* @file
* Contains \Drupal\toolbar\Access\SubtreeAccess.
*/
namespace Drupal\toolbar\Access;
use
Drupal\Core\Access\StaticAccessCheckInterface;
use
Symfony\Component\HttpFoundation\Request;
use
Symfony\Component\Routing\Route;
/**
* Defines a special access checker for the toolbar subtree route.
*/
class SubtreeAccess implements StaticAccessCheckInterface {
 
/**
   * {@inheritdoc}
   */
 
public function appliesTo() {
    return array(
'_access_toolbar_subtree');
  }
 
/**
   * {@inheritdoc}
   */
 
public function access(Route $route, Request $request) {
   
$hash = $request->get('hash');
    return (
user_access('access toolbar') && ($hash == _toolbar_get_subtrees_hash())) ? static::ALLOW : static::DENY;
  }
}
?>

core/modules/toolbar/toolbar.routing.yml:

toolbar.subtrees:
  path: '/toolbar/subtrees/{hash}'
  defaults:
    _controller: '\Drupal\toolbar\Routing\ToolbarController::subtreesJsonp'
  requirements:
    _access_toolbar_subtree: 'TRUE'

Counter-Suggestion

toolbar.subtrees:
  path: '/toolbar/subtrees/{hash}'
  defaults:
    _controller: '\Drupal\toolbar\Routing\ToolbarController::subtreesJsonp'
  requirements:
    _custom_access:  '\Drupal\toolbar\Routing\ToolbarController::accessSubtree'

...and then stick the accessSubtree method with the rest of the stuff that builds the output and have ControllerBase automatically bring in the right stuff (which afaik it does already). No need for an extra class+directory+3 "use" statements, in the vast majority of cases.

Files: 
CommentFileSizeAuthor
#34 access-check-2107137-34.patch9.61 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,452 pass(es).
[ View ]
#34 interdiff.txt682 bytesdawehner
#31 interdiff.txt457 bytesdawehner
#31 access-2107137-31.patch9.43 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,379 pass(es).
[ View ]
#28 interdiff.txt1.6 KBdawehner
#28 access-2107137-28.patch8.98 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#20 access-2107137-20.patch7.94 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,440 pass(es).
[ View ]
#20 interdiff.txt584 bytesdawehner
#14 2107137-14.patch12.72 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,130 pass(es), 396 fail(s), and 50 exception(s).
[ View ]
#14 interdiff-2107137-14.txt894 bytesdamiankloip
#12 2107137-12.patch12.72 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#12 interdiff-2107137-12.txt4.44 KBdamiankloip
#10 access_check-2107137-10.patch7.79 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,684 pass(es).
[ View ]
#10 interdiff.txt1.97 KBtim.plunkett
#7 interdiff.txt2.28 KBdawehner
#7 access_check-2107137-7.patch8.45 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,808 pass(es).
[ View ]
#3 access-2107137-3.patch7.58 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,897 pass(es).
[ View ]

Comments

I am sorry but the issue talks about routing, but the codeexample shows an entity access controller.

This is a minimal implementation of a route access controller:

<?php
/**
* @file
* Contains Drupal\router_test\Access\TestAccessCheck.
*/
namespace Drupal\router_test\Access;
use Drupal\Core\Access\AccessCheckInterface;
use Symfony\Component\Routing\Route;
use Symfony\Component\HttpFoundation\Request;
/**
* Access check for test routes.
*/
class TestAccessCheck implements AccessCheckInterface {
  /**
   * Implements AccessCheckInterface::applies().
   */
  public function applies(Route $route) {
    return array_key_exists('_access_router_test', $route->getRequirements());
  }
  /**
   * Implements AccessCheckInterface::access().
   */
  public function access(Route $route, Request $request) {
    // No opinion, so other access checks should decide if access should be
    // allowed or not.
    return static::DENY;
  }
}

You are right. :) Entity access controllers are dealt with over at #2105557: Add an admin_permission to EntityType annotations to provide simple default access handling so I'll use your code example instead for this issue.

Updating the issue summary...

Status:Active» Needs review
Issue tags:+phpunit
StatusFileSize
new7.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,897 pass(es).
[ View ]

Let's see whether this passes.

Issue summary:View changes

x

FYI, I opened an issue for pretty much the same thing a while back (in june), and it got closed (won't fix) - #2011424: Allow access methods on controllers to be used in routes

Just a few quotes from over there:

From an architectural point of view I'm opposed to couple routes with services.

The problem with tightly coupling it to the controller is that the controller resolution happens *after* access checking right now.

The problem with tightly coupling it to the controller is that the controller resolution happens *after* access checking right now.

Well, if we specify a callback directly this is not a problem.

Status:Needs review» Needs work

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Routing/ToolbarController.php
@@ -27,4 +29,12 @@ public function subtreesJsonp() {
+    return (\Drupal::currentUser('access toolbar') && ($hash == _toolbar_get_subtrees_hash())) ? AccessInterface::ALLOW : AccessInterface::DENY;

This line looks wrong. currentUser() returns a user object, doesn't it? Don't you mean currentUser()->hasPermission()?

And don't you mean to inject the current user service anyway? :-)

Otherwise this seems reasonable, with the caveat (that should be loudly documented) that this *will* result in two instances of the controller object if the access check is on the same class. (I don't know if it's worth trying to collapse that or not.)

Status:Needs work» Needs review
StatusFileSize
new8.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,808 pass(es).
[ View ]
new2.28 KB

Why should toolbar have a test ...

Good catch! (Regarding the current user I was just lazy.

Status:Needs review» Reviewed & tested by the community

Let's do.

Status:Reviewed & tested by the community» Needs work

+++ b/core/tests/Drupal/Tests/Core/Access/CustomAccessCheckTest.php
@@ -0,0 +1,127 @@
+class TestController {
+

Needs docblock, and afaik it's not kosher to have two classes in the same file with PSR-0?

+++ b/core/modules/toolbar/lib/Drupal/toolbar/Routing/ToolbarController.php
@@ -7,12 +7,43 @@
+class ToolbarController implements ContainerInjectionInterface {
+
+  /**
+   * The current user.
+   *
+   * @var \Drupal\Core\Session\AccountInterface
+   */
+  protected $user;
+
+  /**
+   * Constructs a new ToolbarController instance.
+   *
+   * @param \Drupal\Core\Session\AccountInterface $user
+   *   The current user.
+   */
+  public function __construct(AccountInterface $user) {
+    $this->user = $user;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('current_user')
+    );

Ick. Don't like all this boilerplate at all, and certainly not as setting a precedent of what other controllers are going to have to do. Is the current user service part of ContainerBase? If so, can we just extend from that and hide this complexity from user-space code?

Status:Needs work» Needs review
StatusFileSize
new1.97 KB
new7.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,684 pass(es).
[ View ]

It's okay for classes used by only one test class.
Fixed to use Controllerbase

Status:Needs review» Reviewed & tested by the community

You know the amount of code you save on the actual controller is basically the same as the amount of stuff you need to write more when writing a unit test.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new4.44 KB
new12.72 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Me and Daniel spoke about this on IRC this morning, and agreed it makes sense to make the access checking a bit more robust if we are adding something like this. We don't want unexpected access behaviour if people decide to return random things from these ad-hoc access callbacks.

Adding a new AccessException, unit test coverage in AccessManagerTest, and some changes to the interface constants.

+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
@@ -461,6 +461,67 @@ public function testCheckNamedRouteWithNonExistingRoute() {
+  /**
+   * Data provider for testCheckException.
+   *
+   * @return array
+   */
+  public function testCheckExceptionProvider() {
+    return array(
+      array(TRUE),
+      array(FALSE),
+      array(NULL),
+      array(array()),
+      array(array(1)),
+      array('string'),
+      array(0),
+      array(1),
+    );
+  }

This is really great but it should be providerTestCheckException if you don't want to execute this function as actual test.

StatusFileSize
new894 bytes
new12.72 KB
FAILED: [[SimpleTest]]: [MySQL] 58,130 pass(es), 396 fail(s), and 50 exception(s).
[ View ]

HA, You are so right :)

Serves me right for rushing a unit test!

Status:Needs review» Reviewed & tested by the community

Great, thank you!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 2107137-14.patch, failed testing.

Issue tags:+8.x-alpha4

tagging.

+++ b/core/lib/Drupal/Core/Access/AccessInterface.php
@@ -21,7 +21,7 @@
-  const ALLOW = TRUE;
+  const ALLOW = 'TRUE';
@@ -29,7 +29,7 @@
-  const DENY = NULL;
+  const DENY = 'NULL';
@@ -38,7 +38,7 @@
-  const KILL = FALSE;
+  const KILL = 'FALSE';

This change makes no sense to me, and is likely responsible for all of the fails.

Yeah, as mentioned, me and daniel talked about this earlier today but I forgot to mention that :-) . Could be out of scope of this issue though past just enforcing the access return values. Rerolling.

Status:Needs work» Needs review
StatusFileSize
new584 bytes
new7.94 KB
PASSED: [[SimpleTest]]: [MySQL] 59,440 pass(es).
[ View ]

Let's skip this change for now and do that on a follow up.

Status:Needs review» Reviewed & tested by the community

Ah, ok, we're going for reverting all of that, ok: #2108829: Make AccessManager stricter with values returned from access checkers

I think this one is back to RTBC in that case.

+1 for RTBC

Title:Fix the DX of router access checkersFix the DX for declaring custom access checkers

The title seems a little too vague for what the patch is?

Status:Reviewed & tested by the community» Needs review

Sorry, this fell way off my radar. :(

a) Shouldn't core/modules/toolbar/lib/Drupal/toolbar/Access/SubtreeAccess.php be removed now? That'd make it easier to see the DX impact of this patch.

b) I don't see Crell's concern at #6 addressed in documentation. That also sounds sub-optimal. I asked Tim about whether we could work around this and he said "um [...] it might involve some creativity." Given Crell subsequently RTBCed this despite that limitation, I'm guessing he thinks this is acceptable to push to a follow-up?

c) There are a couple of minor language things in the class PHPDoc:

+ * Defines a access checker that allows to specify a custom function for access.

Should be:

+ * Defines an access checker that allows specifying a custom method for access.

Also, "Edit" and "Toolbar" should be capitalized.

I can fix those on commit (unless this gets re-rolled for a/b) but moving to "needs review" for the others.

StatusFileSize
new8.98 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new1.6 KB

a) Shouldn't core/modules/toolbar/lib/Drupal/toolbar/Access/SubtreeAccess.php be removed now? That'd make it easier to see the DX impact of this patch.

You are totally right here!

b) I don't see Crell's concern at #6 addressed in documentation. That also sounds sub-optimal. I asked Tim about whether we could work around this and he said "um [...] it might involve some creativity." Given Crell subsequently RTBCed this despite that limitation, I'm guessing he thinks this is acceptable to push to a follow-up?

Regarding the documentation, this is a quote from the patch:

+ * You should only use it when you are sure that the access callback will not be
+ * reused. Good examples in core are edit or toolbar module.

Status:Needs review» Needs work

The last submitted patch, access-2107137-28.patch, failed testing.

Oh, ok. I was specifically referring to:

"with the caveat (that should be loudly documented) that this *will* result in two instances of the controller object if the access check is on the same class."

I didn't see that in the docs.

Status:Needs work» Needs review
StatusFileSize
new9.43 KB
PASSED: [[SimpleTest]]: [MySQL] 59,379 pass(es).
[ View ]
new457 bytes

Ups.

Issue tags:-8.x-alpha4

This still doesn't have the docs asked for in #30.
Also, it seems we missed the cut here.

webchick: Title callbacks already have the same issue. We should probably try to figure out a good way to address this "reused callable" problem, but I am OK with it not being part of this issue to resolve. (It probably requires a property cache or wrapper cache on the controller resolver spin-off class we wrote.)

In practice, it should only cause a problem if:

1) The class in question is especially memory-costly (which they really should never be) or
2) Someone mistakenly tries to reuse data from the access or title method in the content method, or vice versa.

There is probably a philosophical argument to be made as well that it's a good thing that they're separate objects since the resulting objects are not highly cohesive. I'll leave that for when Anthony Ferrara is back in town, though. :-)

StatusFileSize
new682 bytes
new9.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,452 pass(es).
[ View ]

If you do something costly in your constructors you are doing it wrong. Symfony 2.3 has the concept of lazy services (see http://symfony.com/doc/current/components/dependency_injection/lazy_serv...)
which basically are just initialized once you really use them so constructing them would also be no issue.

If we decide do keep one instance of a controller we really have to be aware of the problem that we basically have created singletons which for sure causes
issues on things like entity routes where you reuse the controller.

Status:Needs review» Reviewed & tested by the community

#34 is sufficiently loud for now. :-)

Title:Fix the DX for declaring custom access checkersChange notice; Fix the DX for declaring custom access checkers
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

This does cut down on boilerplate, I'm a bit surprised there aren't more examples in core, but on the other hand the less of these the better since generic access checks should be generic. Committed/pushed to 8.x, thanks!

Could use a change notice for the API addition.

Status:Active» Needs review

Added a change notice: https://drupal.org/node/2117181

Issue summary:View changes

Revamped based on /actual/ access checker. :P

Title:Change notice; Fix the DX for declaring custom access checkersFix the DX for declaring custom access checkers
Issue summary:View changes
Status:Needs review» Fixed
Issue tags:-Needs change record

Change notice looks good to me.

Status:Fixed» Closed (fixed)

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