Improve efficiency of access checker matching on routes

Nice, that's exactly what I thought would help here.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -69,13 +75,9 @@ public function setChecks(RouteCollection $routes) {
+      if (isset($this->requirementMap[$key]) && $this->checks[$this->requirementMap[$key]]->applies($route)) {
+        $checks[] = $this->requirementMap[$key];
       }

Couldn't we even get rid of the applies method, or do we need this flexibility? One question is: how do we deal with possible multiple access checkers which applies to a single requirements key.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -134,4 +136,25 @@ protected function loadCheck($service_id) {
+      if ($applies_to = $this->checks[$service_id]->appliesTo()) {

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.phpundefined
@@ -23,6 +23,13 @@ public function applies(Route $route) {
+  public static function appliesTo() {

I don't see a reason why this should actually be a static method, as we already have the object available.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.phpundefined
@@ -100,8 +100,8 @@ public function rebuild() {
-      $this->dispatcher->dispatch(RoutingEvents::ALTER, new RouteBuildEvent($collection, $module));
       $this->dumper->addRoutes($collection);
+      $this->dispatcher->dispatch(RoutingEvents::ALTER, new RouteBuildEvent($collection, $module));

This change feels really wrong.

Hm, that limits access checkers to apply to statically defined routes. It for example doesn't allow me to write more intelligent/dynamic checks, especially when adding checks to routes that I don't define.

Which, I think, was the main reason why we went with applies() before.

Couldn't we combine it? Have appliesTo(), allow it to be an empty array (which would match everything, not nothing) and then call applies() when appliesTo() matches, with a default implementation that says return TRUE. Possibly with different names.

So basically that's what damian proposed in the first version of the patch :) I see that this still improves performance, that is great.

+++ b/core/lib/Drupal/Core/Access/AccessCheckInterface.phpundefined
@@ -41,13 +41,22 @@
+   * Declares the route requirement keys this access checker applies to.
...
+   * @return array
+   *   An array of route requirement keys this access checker applies to. An
+   *   empty array will check all routes using the apply method.
+   */
+  public function appliesTo();

This comment should explain why we need both applies and appliesTo. Maybe also mention that return something does lead to a not called apply() method().

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -34,6 +35,13 @@ class AccessManager extends ContainerAware {
+   *
...
+  protected $requirementMap = array();

Some docs missing :(

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -71,13 +79,27 @@ public function setChecks(RouteCollection $routes) {
+    foreach ($route->getRequirements() as $key => $value) {
+      if (isset($this->requirementMap[$key])) {
+        foreach ($this->requirementMap[$key] as $service_id) {
+          if (empty($this->checks[$service_id])) {
+            $this->loadCheck($service_id);
+          }
+          if ($this->checks[$service_id]->applies($route)) {
+            $checks[] = $service_id;
+          }
+        }

Can we have a one liner comment above, please?

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.phpundefined
@@ -19,12 +19,19 @@ class DefaultAccessCheck implements AccessCheckInterface {
   public function applies(Route $route) {
-    return array_key_exists('_access', $route->getRequirements());
+    return TRUE;
   }

maybe we could add a baseclass in a follow up?

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.phpundefined
@@ -19,12 +19,19 @@ class DefaultAccessCheck implements AccessCheckInterface {
+   * Implements AccessCheckInterface::access().

{@inheritdoc}

+++ b/core/modules/rest/lib/Drupal/rest/Access/CSRFAccessCheck.phpundefined
@@ -21,7 +21,8 @@ class CSRFAccessCheck implements AccessCheckInterface {
-    if (array_key_exists('_access_rest_csrf', $requirements)) {
+
+    if (array_key_exists('_entity_create_access', $requirements)) {

This change seems wrong.

There could be also made some unit tests, for the new functionality.

We can still iterate over some of the internal details if needed, but for now this is fine.
Does someone know whether it is worth to test this internal behavior (could be used by using mocks).

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.phpundefined
@@ -16,15 +16,22 @@
+  /**
+   * Implements AccessCheckInterface::access().

{@inheritdoc}

Before we go any further, let's check with higher authority.

Dries: This is a DX improvement. It is not critical, but it is useful. I'm pretty sure it can be done without breaking an existing API; just expanding it. It does not require any follow-ups, but we could likely simplify/refactor several one-off checkers as a result of this. It MAY involve a change to the route that uses those one-offs, but since they're one-offs by definition no one else is going to be using them so no already-ported contribs should be affected. Go/no go?

The only thing that would break is existing custom access checkers.

And if we really have to, we could even make that part optional, by making the method optional, but I'd prefer to not do that as many contrib module authors with custom access checkers will then forget to implement that.

tagging

Proove me wrong, but as the interface for the AccessCheckInterface does not change at all, people don't even have a need to port their code to the static access check interface.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -196,4 +219,38 @@ protected function loadCheck($service_id) {
+      // Empty arrays will not register anything.
+      if (is_subclass_of($this->checks[$service_id], 'Drupal\Core\Access\StaticAccessCheckInterface')) {
+        foreach ((array) $this->checks[$service_id]->appliesTo() as $key) {
+          $this->staticRequirementMap[$key][] = $service_id;
+        }

Yes, I'm a bit confused now, this already seems to be a completely separate interface with only appliesTo().

Are the interfaces really up to date?

Also, why don't you use instanceof here? is_subclass_of() looks strange as it's an interface.

Sounds like this interface should extend from the other and just define this additional method?

But yes, if we have a separate interface, then it's not an API change, the only problem is educating developers to use that interface when they should... which should be possible by a) updating all core access checkers and examples (in change notices, documentation) and b) have good documentation on the interfaces, e.g. document on the non-static interface that in most cases, the static extension should be used.

It feels kind of wrong to have all the static::ALLOW ... constants in the same interface && the actual access method. Can't they be moved to a base interface?

Ah, so the interfaces are exclusive, you can't mix the two methods? Works for me if there's no use case for that. But yes, we should have a common interface then I think.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -194,7 +194,7 @@ protected function checkAny(array $checks, $route, $request) {
+      if ($service_access === AccessCheckInterface::ALLOW) {

Oh php actually cares about it sometimes, nice! Btw. should we better link to AccessInterface::ALLOW here?

This is no longer funny, d.o...

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -196,4 +219,38 @@ protected function loadCheck($service_id) {
+   * @return array
+   *   An array of requirement keys mapped to access checkers. In the format
+   *   '_access_key' => 'service_id'.

If you look at the actual code this does not return anything, as it just sets the information onto the object.

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.phpundefined
@@ -13,17 +13,17 @@
-   * {@inheritdoc}
+   * Implements AccessCheckInterface::access().

Let's keep {@inheritdoc}

Thank you.

Hold on a sec. This
1) Is an API change, based on the number of classes modified.
2) Doesn't address the DX issue of _my_custom_thing: 'TRUE', which is the really dumb part.

I don't think a non-critical-path performance tweak without benchmarks justifies an API change at this point when it doesn't fix the DX issue.

Router building is critical path when you don't have a router, or if you're the person who ended up triggering the router build, but this does need profiling.

Neither non-bc breaking API additions, nor performance improvements need to be assigned to Dries.

Try to also provide the actual test code ... The points from crell got explained. Especially this issue is not about improving the DX so let's add a follow up, if this is still valid. This could be a problem of the route system in general, let's see.

Thanks for the profiling, that's a very nice improvement.

Committed/pushed to 8.x, thanks!