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:


/**
 * @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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

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;
  }
}
webchick’s picture

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...

dawehner’s picture

Status: Active » Needs review
Issue tags: +PHPUnit
FileSize
7.58 KB

Let's see whether this passes.

dawehner’s picture

Issue summary: View changes

x

damiankloip’s picture

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.

dawehner’s picture

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.

Crell’s picture

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.)

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.45 KB
2.28 KB

Why should toolbar have a test ...

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

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Let's do.

webchick’s picture

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?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.97 KB
7.79 KB

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

dawehner’s picture

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.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.44 KB
12.72 KB

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.

dawehner’s picture

+++ 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.

damiankloip’s picture

FileSize
894 bytes
12.72 KB

HA, You are so right :)

Serves me right for rushing a unit test!

dawehner’s picture

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.

webchick’s picture

Issue tags: +8.x-alpha4

tagging.

tim.plunkett’s picture

+++ 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.

damiankloip’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
584 bytes
7.94 KB

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

damiankloip’s picture

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.

tim.plunkett’s picture

+1 for RTBC

damiankloip’s picture

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

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

Xano’s picture

tim.plunkett’s picture

webchick’s picture

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.

dawehner’s picture

FileSize
8.98 KB
1.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.

webchick’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.43 KB
457 bytes

Ups.

tim.plunkett’s picture

Issue tags: -8.x-alpha4

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

Crell’s picture

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. :-)

dawehner’s picture

FileSize
682 bytes
9.61 KB

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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

#34 is sufficiently loud for now. :-)

catch’s picture

Title: Fix the DX for declaring custom access checkers » Change 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.

dawehner’s picture

Status: Active » Needs review

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

dawehner’s picture

Issue summary: View changes

Revamped based on /actual/ access checker. :P

Crell’s picture

Title: Change notice; Fix the DX for declaring custom access checkers » Fix 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.