Use parameter matching via reflection for access checks instead of pulling variables from request attributes

Why the extra method? Why not just map the arguments directly to the access method? That would mean the interface is less useful on it, but that would be true of arguments() as well so it's a wash. But splitting the methods makes it inconsistent with other places we use the callable resolver style.

I'm open to this change if we can make it cleanly.

One issue with using the access() method is that it also receives $route and $account, which aren't on the request attributes. I started #2223473: Simplify AccessManager methods to act on either route or request, but not both in order to remove the $route at least, since that actually is on the request attributes (just not named 'route').

Why the extra method? Why not just map the arguments directly to the access method? That would mean the interface is less useful on it, but that would be true of arguments() as well so it's a wash. But splitting the methods makes it inconsistent with other places we use the callable resolver style.

It is the old disussion. Either you have an interface, in which case you need default values, or you have a separate setter or whatever.
To be honest I think the DX would be worse than before if you would have to write an extra method, which by some magic is executed before the main access method.

We could leverage the request info to get some of the parameters or just accept that the request object is part of the api. If you look at the proposed patch it will be one parameter so an extra method really feels like an overhead.

The attributes() method is also setting stateful data, which therefore disqualifies the request object as a service. So I'm -1 to that.

So we either pass the request (as now) or declare access() to be an attributes-derived method and figure out how to document that in the interface/API spec.

or declare access() to be an attributes-derived method

That's the option I'd prefer: change it from being interface-defined to a non-interface-defined callable that receives arguments via reflection only. But we still need to figure out how to get $route and $account in there for implementations that need it. I think the use case for $route is minimal, and the few implementations that need it can use RequestInfo::getRouteObject($request) instead. Not sure how to best handle $account though. I kind of think we should go back to an _account attribute on $request, but I don't think I'm ready to reopen that battle up quite yet. In lieu of that, perhaps this means we need a separate AccessCallableResolver service rather than reusing ControllerResolver? Or are there any other ideas?

I've felt for a while that ControllerResolver needs to be split up; one service that does just the callable resolution from an arbitrary string, which ControllerResolver then leverages. Then ControllerResolver can be responsible for the argument mapping, while TitleResolver, AccessResolver, or whatever can do their own argument mapping with a different set of magic values.

That should be done at least partially upstream in Symfony; that doesn't block this issue though, as we could have an AccessResolver already. Actually we should probably continue to insist that it's an "access" method on objects of that interface and just do the attribute resolution with $account as the magic case.

Moving to non-interface method would be interesting, especially if a module provides multiple access checkers. This though
would call issues with the applies method.

I've felt for a while that ControllerResolver needs to be split up; one service that does just the callable resolution from an arbitrary string, which ControllerResolver then leverages. Then ControllerResolver can be responsible for the argument mapping, while TitleResolver, AccessResolver, or whatever can do their own argument mapping with a different set of magic values.

Just to be clear, there is an issue: #2165475: Provide a generic class resolver
If we would move that upstream we would have to override it anyway 100%, but sure let's upon up on, and don't rely on it being there.

Work in progress. Provide feedback if you like, or you can wait for a more complete patch.

just a quick note
I would argue that you want to have the account required on there, as it is an edge case not take the current user into account, if you are honest.

This will need profiling where there's lots of access checks on the same page (i..e menu links/tabs). We're caching menu link blocks by default now, so can take a bit of hit there, although primary/secondary menu variables, breadcrumbs, tabs are still not at all.

Far enough along for bot to test, but does not yet implement #11.

Simplified the access() signatures for the easy checkers. There are still some tricky ones left.

Fixed the other bits of the tests.

1. +++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.php
   @@ -7,20 +7,20 @@
    class DefaultAccessCheck implements RoutingAccessInterface {
   +  use DefaultAccessCallbackTrait;
    
   

   I really hate that you have to use both the interface as well as the interface ... Having the interface here does not help as a lot. What about making the interface optional and just used in the really edge-cases like the custom one?

2. +++ b/core/modules/book/lib/Drupal/book/Access/BookNodeIsRemovableAccessCheck.php
   @@ -9,14 +9,14 @@
    class BookNodeIsRemovableAccessCheck implements AccessInterface {
   +  use DefaultAccessCallbackTrait;
   

   I wonder whether we can let the DefaultAccessCallbackTrait implement the AccessInterface. Having to implement both is REALLY odd

This converts all the remaining access checks to get route parameters via their signature rather than via $request->attributes. $request->attributes is still being used for internal attributes that aren't route parameters, because removing those is outside the scope of this issue (see #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API for that). And Edit module has access checks that still do funky things with $request, but I updated the @todo to link to #1837388: Provide a ParamConverter than can upcast any entity. for fixing that.

I would argue that you want to have the account required on there, as it is an edge case not take the current user into account

We have 7 access checks that don't depend on $account: CsrfAccessCheck, DefaultAccessCheck, ThemeAccessCheck, BookNodeIsRemovableAccessCheck, FormatDisableCheck, CronAccessCheck, UpdateManagerAccessCheck, and those all seem fairly legit to me, so I wouldn't call that an edge case.

This will need profiling where there's lots of access checks on the same page

Yeah, that would be good. The current patch does the reflection for every invocation, but we could add static caching (in the AccessArgumentsResolver service) of the reflection results per callable, so a menu with the same access checker running lots of times (e.g., a menu with a lot of node links) wouldn't need to do all that reflection for every link.

I wonder whether we can let the DefaultAccessCallbackTrait implement the AccessInterface.

Sadly, PHP traits can't implement interfaces; they can only implement the methods, but the using class must be the one to specify whether it implements the interface or not.

What about making the interface optional and just used in the really edge-cases like the custom one?

I thought about doing that by making AccessManager::performCheck() check to see if the interface is implemented, and if not, then fall back to the default of array($this->checks[$service_id], 'access'). Then we wouldn't need DefaultAccessCallbackTrait either. However, that felt wrong because it creates an implied interface (that the access() method exists and means what it means) rather than an explicit one. Also, the interface is what contains the constants (ALLOW, DENY, KILL), and it's nice for the access checkers to be able to refer to them via static::.

Here's a thought: If it's too wonky to do for the checker services due to the interface, what about focusing on the custom access callbacks? They're intended for the one-off cases. They may be more common in practice so more relevant for the typical developer anyway. (Note: I haven't actually checked to see if that's the case, I'm just wondering if it is.)

I agree that having the constants available is a huge win.
Couldn't we also write down the used methods into the service definition, so the interface would be just the constants?

I like #26. Will work on that after getting this green.

For simplicity, just forcing the method name to access(). That's no more constricting than what's in HEAD. The flexibility of #26 might be a good follow up to work on, but I'd rather not hold this up on it.

I think all that's left now is profiling and optimizing the arguments resolver, unless there's additional feedback.

Here's a scenario I tested for profiling: standard install profile, give anon users permission to "Administer site configuration", "Use the administration pages and help", and "Use the administration toolbar". Then, as anon, visit the home page, and see the URL for the toolbar JSONP request (/toolbar/subtrees/INSERT_HASH_HERE/en). Then ab -c1 -n100 that URL. This results in a few dozen menu links being rendered and not much else happening. On my machine with xdebug disabled:

HEAD: 105ms
Patch: 108ms

With patch, AccessArgumentsResolver::getArguments() is called 48 times, and I'm guessing that that's where the bulk of the time difference is, in which case that's ~60us per invocation, and I suspect can be optimized per #22.2.

Based on discussion with webchick and xjm, raising this to beta blocker, since it's a child of #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. We'll need to file more children of that issue, and not every one necessarily needs to block beta, but access checkers is one of the more common module-developer-facing ones.

It is great to get rid of the trait madness, though I don't really get why we don't do it right in the beginning and put the callable onto the service

does not apply after #2235347: Field instance operations are out of order

Noticed this though.

+++ b/core/tests/Drupal/Tests/Core/Route/RoleAccessCheckTest.php
@@ -161,14 +161,14 @@ public function testRoleAccess($path, $grant_accounts, $deny_accounts) {
       $subrequest = Request::create($path, 'GET');
       $message = sprintf('Access granted for user with the roles %s on path: %s', implode(', ', $account->getRoles()), $path);
-      $this->assertSame(AccessCheckInterface::ALLOW, $role_access_check->access($collection->get($path), $subrequest, $account), $message);
+      $this->assertSame(AccessCheckInterface::ALLOW, $role_access_check->access($collection->get($path), $account), $message);
...
       $subrequest = Request::create($path, 'GET');
       $message = sprintf('Access denied for user %s with the roles %s on path: %s', $account->id(), implode(', ', $account->getRoles()), $path);
-      $has_access = $role_access_check->access($collection->get($path), $subrequest, $account);
+      $has_access = $role_access_check->access($collection->get($path), $account);

we get rid of the usage of subrequest but not its creation?

Just a reroll. Doesn't yet address #33 or #34.

Fixes #34.

Re #33: I think doing so would require some nontrivial changes to AccessManager that would be better reviewed independently in a separate issue. If I'm wrong about that, and it's actually more straightforward than I assume, can you post the patch for it?

HAHA, yeah i really think it is a bad idea to switch all stuff to custom access checkers, obviously. It is good that we got get rid of that.

Almost every other checker is used 4 or less times, most 1-2 times.

Even if it is hard to accept, people will write modules outside of /core which will depend on our existing stuff. We should not make their lives increadible hard for the sake of purity.

I don't follow the implications of #38. Are you suggesting we move more things to custom checkers and just make custom checkers do the reflection magic, or something else...?

Straight reroll. Will answer #44 in following comment.

I support converting most of the module-specific access check services currently tagged access_check into custom access checkers. For example, I see absolutely no benefit in BookNodeIsRemovableAccessCheck or ContactPageAccess being tagged services. To me, those are examples of ones that should clearly fall into the "custom" category.

For the two that Tim chose to convert in #38 though: NodeAddAccessCheck and NodeRevisionAccessCheck, I'm a little less sure. I actually wonder if we want to abstract those into generic entity access checkers that other entity types could leverage for per-bundle or cross-bundle creation checking and operation-on-revision checking.

Because I think each specific access check conversion to either "make custom" or "make more generic" or "leave alone" should get its own review, I'd prefer to defer all such conversions to a separate issue from this one (not even a follow up, as that work can be done independently of this issue).

The way those conversions got lumped into this issue is the comment in #37:

I really don't feel comfortable with calling that access method without any checks. If it's named anything else (or missing), [the resulting error is] not very helpful.

Would it be sufficient for resolving this issue to make that error more helpful? For example, in AccessManager::performCheck(), throw a friendly AccessException if !is_callable($callable)?

Per #36, I still think we can have a follow up for making it possible to specify the method name as another attribute on the access_check tag, to help with the DX of whatever checks we decide to keep as tagged services. I'd just like that to be a follow up, since I think we'll need to do a little refactoring of AccessManager's addCheckService() and friends, which I don't think will be that big a deal, but just not something I want cluttering this patch. If you insist on it being done in this issue though, I'll go along with it.

Happily rerolled for shortcut's conversion to PSR-4.

it is *really* messy to do this on compile-time

Like this? What's messy about it?

Given #42 and #46, reverting the conversions of Node module access checkers to custom ones. Let's have a separate issue to refactor them if desired.

Reroll.

Am working on optimization per #31 next.

This is the (rather ugly) optimization I mentioned in #22.2. It doesn't seem to improve the ab numbers of the #31 scenario by much though. So both this and #58 need to be profiled for real, to see what's causing the difference.

So I tried xhprof'ing on my machine, and it appeared that #59's approach of trying to statically cache and reuse reflection results didn't improve anything because reflection isn't what's slow. Actually, it appeared that parameter bag's get(), has(), and array_key_exists() were some of the slowest contributors, so here's a different micro-optimization. It discards #59, so interdiff is relative to #58.

However, I'm not posting xhprof results because I'm having issues with my machine setup, where I'm using PHP 5.5, and I can't get xhprof to work without segfaulting unless I also enable the xdebug.so extension, but doing that makes wall time numbers meaningless.

Also, when I test locally, I get as much noise between individual runs on the same codebase as the signal between HEAD and patch.

Hoping someone else can profile and post cleaner results. The goal is to profile a request that checks access on many menu links (#31 is one way to do that, and doesn't need to be done as anonymous, that was just for the benefit of running ab). The biggest culprit should be the inclusive time of AccessArgumentsResolver::getArguments(). Other differences are likely to be noise.

I did some quick profiling here using xhprof-kit and the scenario outlined in #31. This is roughly an average result. I had wt results as low as +1.3% and as high as +2.3%, but the function calls were always consistent.

=== 8.x..8.x compared (5361b94b74ee9..5361b9bbb4efb):

ct  : 30,919|30,919|0|0.0%
wt  : 209,958|209,738|-220|-0.1%
cpu : 196,764|196,517|-247|-0.1%
mu  : 23,989,528|23,989,528|0|0.0%
pmu : 24,019,136|24,019,136|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5361b94b74ee9&...

=== 8.x..access-2222719-60 compared (5361b94b74ee9..5361b9d05fc0c):

ct  : 30,919|32,068|1,149|3.7%
wt  : 209,958|213,657|3,699|1.8%
cpu : 196,764|200,472|3,708|1.9%
mu  : 23,989,528|24,038,480|48,952|0.2%
pmu : 24,019,136|24,068,152|49,016|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5361b94b74ee9&...

(edited to add missing link for the actual comparison).

Thanks, Cottser! That does look like a clean result.

I think the best way to analyze this is to sort by function calls diff: http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?sort=ct&run1=5361b9....

Adding up the exclusive wall diff of the top 13 lines (calls diff >= 47) is 3.3ms of the 3.7ms total. Remaining differences include things like the load of AccessArgumentsResolver.php (0.145ms), plus noise (note that #61's comparison of 2 different runs of HEAD showed 0.2ms of noise).

So, if we ignore noise and the loading of a PHP file, and just look at what would scale linearly with number of access checks performed, 3.3ms is 1.6% of the total time of a request whose only job is to render 40ish menu links (the toolbar JSONP request). @catch: is that acceptable?

This patch hasn't been RTBC'd yet, but assigning to catch for feedback on performance cost.

One possibility, if we're concerned about the performance impact, is that we can create a new interface (e.g., UnreflectedAccessInterface) with an access() method that's hardcoded to the same signature as in HEAD ($route, $request, $account), and we make AccessManager::performCheck() check if the service implements that interface first, and only if it doesn't, do the whole AccessArgumentsResolver lookup.

In core, we could then at a minimum make PermissionAccessCheck and EntityAccessCheck use that interface, and I think that would take care of the vast majority of menu links, so that we'd only have a relatively small number of AccessArgumentsResolver lookups happening for any given request.

But then, each contrib access checker would need to make a decision as to whether to implement UnreflectedAccessInterface or allow reflection to be used. Is the optimization worth adding the extra decision and code branch?

Alternatively, we could do what tim.plunkett suggested, and only implement reflection for custom access checks, and move many of our core ones to that. That would remove a decision point for contrib: they either implement a 'access_check' service or a custom callback, just as in HEAD, and based on that one decision, they know whether to implement an interface-defined access() signature or a reflection-resolved one. However, then we'd be coupling the DX of the signature to whatever other factors are involved in deciding between tagged service and custom callback, and is that making things easier or harder on contrib?

Note that the entire purpose of this issue's attempt to support reflection-based signature is to eliminate $request->attributes->get('foo') as a common thing for contrib to deal with. If something like $route_match->getArgument('foo') (from #2238217: Introduce a RouteMatch class) within access() methods would be seen as acceptable DX, we could alternatively "won't fix" this.

One possibility, if we're concerned about the performance impact, is that we can create a new interface (e.g., UnreflectedAccessInterface) with an access() method that's hardcoded to the same signature as in HEAD ($route, $request, $account), and we make AccessManager::performCheck() check if the service implements that interface first, and only if it doesn't, do the whole AccessArgumentsResolver lookup.

This sounds like something we could do as a follow-up including after release?

Assuming the performance regression here is correct, I think that's acceptable given we have critical issues to cache all menu output by default - including the access checks.

That won't apply to local tasks etc. but there's less of them .

Yay! Ok, anything left to do on the patch then before RTBC?

Removed even the commented out access() method from the interface, and inlined access() docs into each using class. RTBC, anyone?

+++ b/core/lib/Drupal/Core/Access/AccessArgumentsResolver.php
@@ -0,0 +1,126 @@
+      // Even if there's no type hint, use the upcasted value if available;
+      // otherwise, fall back to the raw value.
+      if (isset($upcasted_route_arguments[$parameter_name])) {
+        return $upcasted_route_arguments[$parameter_name];
+      }
+      if (isset($raw_route_arguments[$parameter_name]) || array_key_exists($parameter_name, $raw_route_arguments)) {
+        return $raw_route_arguments[$parameter_name];
+      }
+    }

This opens up the problem as we basically solved in #1906810: Require type hints for automatic entity upcasting already.

Agreed, this seems wrong - if you don't type hint, you should only get the raw value:

// Even if there's no type hint, use the upcasted value if available;

Also, an updated summary would help to review.

Fixes #67/#68. Working on updating the summary.

Updated the summary. And here's a small improvement I discovered while writing it.

Sigh. Found a landmine. Sidestepping it here, but will open a separate issue if there isn't one already.

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.php
@@ -18,9 +16,14 @@
+   *
+   * @return One of the constants from \Drupal\Core\Access\AccessInterface.

Do we have a proper way to do that? Everybody uses their own way to document it.

Assigning to catch to review the profiling.

+ * @return One of the constants from \Drupal\Core\Access\AccessInterface.

Do we have a proper way to do that? Everybody uses their own way to document it.

No need to invent something. @return documents the data type, not the origin. The description can point to the origin.

 * @return string
 *   A \Drupal\Core\Access\AccessInterface constant value.

I don't have much to add on the profiling beyond #64 - it's a noticeable regression but we have caching issues in progress, and the interface idea if the uncached performance turns out to be a problem.

Let's fix those docs. I haven't done a final review of the patch yet, but that doesn't have to be me either.

I'll clean up the docs; I think @sun's suggestion is spot on.

Did a quick read for any other docs gate issues or other docs cleanups while I'm at it; few notes...

1. +++ b/core/lib/Drupal/Core/Access/AccessArgumentsResolver.php
   @@ -0,0 +1,123 @@
   +   *   The value of the requested parameter value.
   +   * @throws \RuntimeException
   

   blank line

2. +++ b/core/lib/Drupal/Core/Access/AccessArgumentsResolver.php
   @@ -0,0 +1,123 @@
   +    // @todo Remove this once AccessManager::checkNamedRoute() is fixed to not
   +    //   leak _raw_variables from the request being duplicated.
   

   Issue for this?

3. +++ b/core/lib/Drupal/Core/Access/AccessArgumentsResolver.php
   @@ -0,0 +1,123 @@
   +   * @param callable $callable
   ...
   +   * @return \ReflectionFunctionAbstract
   

   Missing descriptions

4. +++ b/core/lib/Drupal/Core/Access/AccessArgumentsResolver.php
   @@ -0,0 +1,123 @@
   +   * Called by getArgument() if an argument value can't be resolved.
   ...
   +  protected function handleUnresolvedArgument(\ReflectionParameter $parameter) {
   

   Needs a one-line summary about what it actually does. Also missing parameter documentation.

5. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -46,6 +45,13 @@ class AccessManager implements ContainerAwareInterface {
   +   * Array of access check method names keyed by service id.
   

   s/id/ID/g

6. +++ b/core/lib/Drupal/Core/Access/AccessManager.php
   @@ -328,10 +339,46 @@ protected function checkAny(array $checks, $route, $request, AccountInterface $a
   +   * @return string
   +   *   Either the AccessInterface::ALLOW, AccessInterface::DENY, or
   +   *   AccessInterface::KILL constant.
   

   This is documented differently from the other one.

7. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
   @@ -39,9 +38,16 @@ function __construct(CsrfTokenGenerator $csrf_token) {
   +   * Checks access.
   

   (and elsewhere) This is sort of a vague description.

8. +++ b/core/lib/Drupal/Core/Access/CsrfAccessCheck.php
   @@ -39,9 +38,16 @@ function __construct(CsrfTokenGenerator $csrf_token) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/lib/Drupal/Core/Access/CustomAccessCheck.php
   @@ -33,25 +33,41 @@ class CustomAccessCheck implements RoutingAccessInterface {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.php
   @@ -18,9 +16,14 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.php
   @@ -29,6 +29,15 @@ class EntityAccessCheck implements AccessInterface {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php
   @@ -42,7 +42,16 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/lib/Drupal/Core/Theme/ThemeAccessCheck.php
   @@ -18,10 +15,15 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/book/lib/Drupal/book/Access/BookNodeIsRemovableAccessCheck.php
   @@ -36,14 +34,15 @@ public function __construct(BookManagerInterface $book_manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/config_translation/lib/Drupal/config_translation/Access/ConfigTranslationOverviewAccess.php
   @@ -43,7 +43,16 @@ public function __construct(ConfigMapperManagerInterface $config_mapper_manager)
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
   @@ -47,10 +46,17 @@ public function __construct(ConfigFactoryInterface $config_factory, UserDataInte
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationManageAccessCheck.php
   @@ -37,14 +37,29 @@ public function __construct(EntityManagerInterface $manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/content_translation/lib/Drupal/content_translation/Access/ContentTranslationOverviewAccess.php
   @@ -36,9 +35,16 @@ public function __construct(EntityManagerInterface $manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php
   @@ -36,22 +36,33 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php
   @@ -36,22 +36,33 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.php
   @@ -36,17 +35,24 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php
   @@ -63,21 +62,25 @@ public function __construct(EntityManagerInterface $entity_manager, Connection $
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/quickedit/lib/Drupal/quickedit/Access/EditEntityAccessCheck.php
   @@ -38,12 +36,19 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/quickedit/lib/Drupal/quickedit/Access/EditEntityFieldAccessCheck.php
   @@ -37,17 +36,28 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/rest/lib/Drupal/rest/Access/CSRFAccessCheck.php
   @@ -41,9 +41,16 @@ public function applies(Route $route) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/shortcut/src/Access/ShortcutSetSwitchAccessCheck.php
   @@ -9,18 +9,24 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/system/lib/Drupal/system/Access/CronAccessCheck.php
   @@ -18,10 +15,14 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/system/tests/modules/router_test_directory/lib/Drupal/router_test/Access/DefinedTestAccessCheck.php
   @@ -18,9 +16,14 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/system/tests/modules/router_test_directory/lib/Drupal/router_test/Access/TestAccessCheck.php
   @@ -18,9 +15,11 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/tracker/lib/Drupal/tracker/Access/ViewOwnTrackerAccessCheck.php
   @@ -18,12 +17,16 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccessCheck.php
   @@ -36,9 +33,11 @@ public function __construct(Settings $settings) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/user/lib/Drupal/user/Access/LoginStatusCheck.php
   @@ -18,9 +17,16 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php
   @@ -18,11 +17,17 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/user/lib/Drupal/user/Access/RegisterAccessCheck.php
   @@ -18,9 +17,16 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
   @@ -22,9 +21,16 @@
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   
   +++ b/core/modules/views/lib/Drupal/views/ViewsAccessCheck.php
   @@ -27,12 +26,15 @@ public function applies(Route $route) {
   +   * @return One of the constants from \Drupal\Core\Access\AccessInterface.
   

   These are the ones to change. Wow but we repeat that a lot... feels like a code smell... I'll ask effulgentsia to clarify why this is.

9. +++ b/core/lib/Drupal/Core/Routing/Access/AccessInterface.php
   @@ -8,28 +8,12 @@
   +  // @todo Remove this interface since it no longer defines any methods?
   

   So we're now using the interface as a container for three constants. Is there an issue to explore this?

10. +++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
    @@ -633,11 +659,18 @@ protected static function convertAccessCheckInterfaceToString($constant) {
    + * An interface with a defined access() method for mocking.
    

    Defines. :)

Re #81.2: #2265939: [sechole] AccessManager::checkNamedRoute() leaks attributes from current request. I still need to file issues for the other two @todos that are missing issue links.

Here we go. I addressed everything in #81. Couple unrelated things I noted during the process.

• I noted that FormModeAccessCheck and ViewModeAccessCheck have incorrect class docblocks. Will file a followup issue for that.
• I also noticed that \Drupal\quickedit\Access\EditEntityAccessCheck and EditEntityFieldAccessCheck should probably be renamed to indicate they're specifically for quickedit now that the module has been renamed (as maybe should their relevant methods possibly other classes in the namespace); I was confused at first since there's a different checker for entity CRUD.

I also noticed that there are some places where we're not properly documenting optional parameters (comparing the @param docs to the signatures). The docblock should indicate when the parameter is optional and what happens in the default case when it is not provided. E.g.:

+++ b/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php
@@ -63,21 +62,25 @@ public function __construct(EntityManagerInterface $entity_manager, Connection $
+   * Checks access.
+   *
+   * @param \Symfony\Component\Routing\Route $route
+   *   The route to check against.
+   * @param \Drupal\Core\Session\AccountInterface $account
+   *   The currently logged in account.
+   * @param \Drupal\node\NodeInterface $node
+   *   The node.
+   * @param string $node_revision
+   *   The node revision.
...
+  public function access(Route $route, AccountInterface $account, NodeInterface $node = NULL, $node_revision = NULL) {

Should $node really be optional here, and is the revision really a string? Also, the parameters should be marked as optional in the docblock if they are.

And the rest:

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php
@@ -36,22 +36,33 @@ public function __construct(EntityManagerInterface $entity_manager) {
+   * @param string $form_mode_name
+   *   The form mode.
+   * @param string $bundle
+   *   The bundle.
...
+  public function access(Route $route, Request $request, AccountInterface $account, $form_mode_name = 'default', $bundle = NULL) {

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php
@@ -36,22 +36,33 @@ public function __construct(EntityManagerInterface $entity_manager) {
+   * @param string $view_mode_name
+   *   The view mode.
+   * @param string $bundle
+   *   The bundle.
...
+  public function access(Route $route, Request $request, AccountInterface $account, $view_mode_name = 'default', $bundle = NULL) {

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.php
@@ -36,17 +35,24 @@ public function __construct(EntityManagerInterface $entity_manager) {
+   * @param \Drupal\node\NodeTypeInterface $node_type
+   *   The node type.
...
+  public function access(AccountInterface $account, NodeTypeInterface $node_type = NULL) {

Filed the remaining todos that were missing issue links: #2266817: Deprecate empty AccessInterface and remove usages and #2266809: Make QuickEditEntityFieldAccessCheck::access() use the $account that's passed in, and fixed #83, but did not file the two issues mentioned at the top of #83.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeRevisionAccessCheck.php
@@ -68,15 +68,19 @@
    *   The route to check against.
    * @param \Drupal\Core\Session\AccountInterface $account
    *   The currently logged in account.
+   * @param int $node_revision
+   *   (optional) The node revision ID. If not specified, but $node is, access
+   *   is checked for that object's revision.
    * @param \Drupal\node\NodeInterface $node
-   *   The node.
-   * @param string $node_revision
-   *   The node revision.
+   *   (optional) A node object. Used for checking access to a node's default
+   *   revision when $node_revision is unspecified. Ignored when $node_revision
+   *   is specified. If neither $node_revision nor $node are specified, then
+   *   access is denied.
    *
    * @return string
    *   A \Drupal\Core\Access\AccessInterface constant value.
    */
-  public function access(Route $route, AccountInterface $account, NodeInterface $node = NULL, $node_revision = NULL) {
+  public function access(Route $route, AccountInterface $account, $node_revision = NULL, NodeInterface $node = NULL) {
     if ($node_revision) {
       $node = $this->nodeStorage->loadRevision($node_revision);

The order of the arguments switched around here; does that not matter/impact calling code because of upcasting? (I can see how this is a more logical optional order based on the docs.)

Filed:

• #2267571: FormModeAccessCheck and ViewModeAccessCheck have incorrect class docblocks
• #2267621: Deprecate and rename EditEntityFieldAccessCheck now that the module is named Quick Edit

Circular issue references are circular.

Talked to @effulgentsia about #85 and the answer is "yes that's the point". :) So AFAIK all the followups and docs improvements are taken care of now.

Yay, thanks @tim.plunkett!

Filed #2268537: Add unit tests for AccessArgumentsResolver since as @tim.plunkett points out there's already integration test coverage, but unit tests for the new class would be good.

Thanks for the tests. The 2 tiny non-test changes look great. For the tests:

1. +++ b/core/tests/Drupal/Tests/Core/Access/AccessArgumentsResolverTest.php
   @@ -0,0 +1,238 @@
   +    // Test an optional parameter with no default.
   +    $data[] = array(
   +      function($foo = 'foo') {}, new Request(), array('foo'),
   +    );
   

   s/default/provided value/?

2. +++ b/core/tests/Drupal/Tests/Core/Access/AccessArgumentsResolverTest.php
   @@ -0,0 +1,238 @@
   +  /**
   +   * Tests getArgument() with a 'route' parameter with no typehint and a value.
   +   */
   +  public function testGetArgumentRouteNoTypehintAndValue() {
   +    $callable = function($route) {};
   +    $request = new Request();
   +    $request->attributes->set('route', 'foo');
   +
   +    $arguments = (new AccessArgumentsResolver())->getArguments($callable, $this->route, $request, $this->account);
   +    $this->assertSame(array('foo'), $arguments);
   +  }
   +
   +  /**
   +   * Tests getArgument() when upcasting is bypassed.
   +   */
   +  public function testGetArgumentBypassUpcasting() {
   +    $callable = function(Route $route = NULL) {};
   +
   +    $request = new Request();
   +    $request->attributes->set('route', NULL);
   +
   +    $arguments = (new AccessArgumentsResolver())->getArguments($callable, $this->route, $request, $this->account);
   +    $this->assertSame(array(NULL), $arguments);
   +  }
   

   I don't think it's clear what the purpose of these two tests are, especially the last one. Are we trying to test a generic parameter name and the ability for it to be in ->attributes as NULL, even if the callable typehints? Or are we specifically testing the functionality of 'route' as a named attribute overriding $this->route? Related, do we also want a test for in that latter case, to still be able to receive $this->route into the callable via a Route $pick_any_name_you_like parameter?

Thanks.

This look like a good DX win and it helps us make progress on #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. Given that catch feels comfortable with the performance issue, I decided to commit it to 8.x. Thanks everyone!

We certainly should improve our horrible documentation now, see #2611272: Add documentation to \Drupal\Core\Routing\Access\AccessInterface how to implement it