All route enhancers are run on every request

I kinda talked with lsmith about that but he seems to be not perfectly comfortable to add this lazyness to CMF.

I'll try to subclass the required classes to implement it. Note: We should do the same for route filters as well.
This speeds up things like not requiring AcceptHeaderMatcher for quite a lot of requests. (yes, HTML requests are still kinda special)

It might be possible to add one single proxy route enhancer to the dynamic router which then forwards applyRouteEnhancers() to lazy loaded drupal route enhancers. Like this we could avoid subclassing dynamic router.

Pretty sure on admin pages they run more than once because of breadcrumb building, on a deep admin page eg admin/structure/types/manage/page/fields they might even run six times. Hope I'm wrong on that.

Just posting work so far, not really helpful at all yet.

There is no reason that I block progress on that issue.

@kgoel wanted to work on this issue, just in case someone wants to directly work on this issue.

@kgoel - still keen to work on this?
I'm willing to take a look too.

Still need to take care route filters and add applies method in ParamConversionEnhancer.php, EntityRouteEnhancer.php, and AuthenticationEnhancer.php

1. +++ b/core/lib/Drupal/Core/Routing/Enhancer/ParamConversionEnhancer.php
   @@ -90,4 +90,16 @@ public static function getSubscribedEvents() {
   +   * Determines if enhancers apply to a route.
   +   *
   +   * @param \Symfony\Component\Routing\Route $route
   +   *  The route to consider attaching to.
   +   *
   +   * @return bool
   +   *   TRUE if the check applies to the passed route, False otherwise.
   
   +++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   @@ -0,0 +1,118 @@
   +   * Determines if enhancers apply to a route.
   +   *
   +   * @param \Symfony\Component\Routing\Route $route
   +   *  The route to consider attaching to.
   +   *
   +   * @return bool
   +   *   TRUE if the check applies to the passed route, False otherwise.
   

   This can be replaced by {inheritdoc}.

2. +++ b/core/lib/Drupal/Core/Routing/Enhancer/ParamConversionEnhancer.php
   @@ -90,4 +90,16 @@ public static function getSubscribedEvents() {
   +    return True;
   

   TRUE

3. +++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   @@ -0,0 +1,118 @@
   +class LazyRouteEnhancer implements BaseRouteEnhancerInterface, ContainerAwareInterface{
   

   space missing before {

4. +++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   @@ -0,0 +1,118 @@
   +   * @return \Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface[]|\Drupal\Core\Routing\Enhancer\RouteEnhancerInterface[]
   

   Incomplete docs.

+++ b/core/lib/Drupal/Core/Routing/Enhancer/ParamConversionEnhancer.php
@@ -90,4 +90,10 @@ public static function getSubscribedEvents() {
+   */
+  protected function applies(Route $route) {
+    return ($route->getOption('parameters')) {
+  }
 }

can we cast this to a bool or even use $route->hasOption('parameters') maybe?

Sure, hasOption is better since it returns bool.

@kgoel
If possible, try to post interdiffs, even the progress might be small, but it makes it possible to follow your work a bit.

long interdiff....since the old branch but will keep in mind to post interdiff from now on.

+++ b/core/lib/Drupal/Core/Routing/LazyRouteFilter.php
@@ -0,0 +1,98 @@
+use use Symfony\Component\Routing\Exception\RouteNotFoundException;

This will cause a runtime issue.

Thanks Lucas! To be on the safe side, I blame late night patch :p

This doesn't remove an API, just technically provides an alternate, preferred approach. Non-lazy enhancers would still work, I think. That means there's no API change here, and doesn't solve a *current* significant performance issue (as the big one was already addressed). So... why is this critical? Seems like a major to me.

Worked a bit on adding @todos about the next steps we should do and fixed some small parts on the way.

Has this been profiled to show improved performance? I don't understand why it is critical otherwise. I reviewed some of the comments.

1. +++ b/core/lib/Drupal/Core/Routing/Enhancer/RouteEnhancerInterface.php
   @@ -0,0 +1,30 @@
   + * An route enhance service to determine route enhance rules for particular
   + * routes.
   

   This should be on one line. What about "Determines route enhance rules."?

2. +++ b/core/lib/Drupal/Core/Routing/Enhancer/RouteEnhancerInterface.php
   @@ -0,0 +1,30 @@
   +   * Declares whether the route enhancer applies to a specific route or not.
   

   This is a bit wordy for me. What about "Declares if the route enhancer applies to the given route."?

3. +++ b/core/lib/Drupal/Core/Routing/Enhancer/RouteEnhancerInterface.php
   @@ -0,0 +1,30 @@
   +   *   TRUE if the check applies to the passed route, False otherwise.
   

   "False otherwise" is probably unnecessary.

4. +++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   @@ -0,0 +1,103 @@
   +   * Registers a new Enhancer by Enhancer.
   

   I don't understand what this means.

5. +++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   @@ -0,0 +1,103 @@
   +   * For each route, gets a list of applicable enhancer to the route.
   

   Should be "enhancers to the route".

6. +++ b/core/lib/Drupal/Core/Routing/LazyRouteFilter.php
   @@ -0,0 +1,98 @@
   +   * Registers a new Filter by Filter.
   

   This is unclear to me.

7. +++ b/core/lib/Drupal/Core/Routing/LazyRouteFilter.php
   @@ -0,0 +1,98 @@
   +   * For each route, gets a list of applicable enhancer to the route.
   

   "enhancers"

8. +++ b/core/lib/Drupal/Core/Routing/RouteFilterInterface.php
   @@ -0,0 +1,29 @@
   +   * Declares whether the route filter applies to a specific route or not.
   +   *
   +   * @param \Symfony\Component\Routing\Route $route
   +   *  The route to consider attaching to.
   +   *
   +   * @return bool
   

   Same as above.

Still need to address -

# @todo: We need a event listener which listens to RoutingEvents::ALTER
+ # And asks LazyRouteFilter::setFilters().

Do I need to create a new event listener?

Do I need to create a new event listener?

A new one is probably a good idea here :)

This could be why the tests are failing:

+++ b/core/lib/Drupal/Core/EventSubscriber/RouteEnhancerSubscriber.php
@@ -0,0 +1,53 @@
+    $this->routeEnhaner->setEnhancers($event->getRouteCollection());

s/routeEnhaner/routeEnhancer/

@almaudoh - Thanks for catching that! However, test is failing because of

route_filter_subscriber:
      class: Drupal\Core\EventSubscriber\RouteFilterSubscriber
        arguments: ['@route_filter']
        tags:
          - { name: event_subscriber }

There is something wrong with registering listener as a service and I am trying to figure out what have I missed here :)

@kgoel
Are you sure its fine to use 4 spaces as intentation instead of 2, as the other part of the file?
Did you tried to put a print statement/debugger into RouteFilterSubscriber::onRequest() and see whether its triggered when you call drush cr?

Fixed space and spell.

.

I don't think its called when I tried to run drush cr and this is what I am getting on my local -

PHP Catchable fatal error: Argument 1 passed to Drupal\Core\DrupalKernel::findSitePath() must be an instance of Symfony\Component\HttpFoundation\Request, null given, called in drupal-8/core/includes/utility.inc on line 40 and defined in drupal-8/core/lib/Drupal/Core/DrupalKernel.php on line 307

Thanks @dawehner for your help!

hmmm....I am not sure what's wrong at or around line 649 with arguments in core.services.yml .

In Url::createFromRequest(), line 263, \Drupal::service('router.no_access_checks')->matchRequest($request); returns an array without the '_raw_variables' key set.

There are various things which have been wrong in my initial patch, see interdiff, ...

I have added route enhancer back for ParamConversionEnhancer.

Now things install at least

A small fix for the patch, but a great step for the bot.

Priority matters.

Let's fix it + minor nitpicks.

1. +++ b/core/core.services.yml
   @@ -641,21 +651,31 @@ services:
      route_enhancer.authentication:
        class: Drupal\Core\Routing\Enhancer\AuthenticationEnhancer
        tags:
   -      - { name: route_enhancer, priority: 1000 }
   +      - { name: lazy_route_enhancer, priority: 1000 }
   

   Does this belong as an enhancer (unrelated) - should it be hard-coded like the access subscriber for security reasons?

2. +++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   @@ -0,0 +1,103 @@
   + * For any route request, route listener service takes long response time as
   

   The grammar here is a bit patchy

3. +++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   @@ -0,0 +1,103 @@
   +// @todo Add some simple integration test, which checks some of the _entity_form
   +//   routes to include the expected options.
   

   can we get a follow up for this or do you intend to do it here?

4. +++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
   @@ -0,0 +1,103 @@
   +        $this->enhancers[$service_id] = $this->container->get($service_id);
   

   is there a chance this could lead to stuff getting serialized inadvertently?

5. +++ b/core/lib/Drupal/Core/Routing/LazyRouteFilter.php
   @@ -0,0 +1,99 @@
   +
   

   extra space here

Thank you for your review!

Does this belong as an enhancer (unrelated) - should it be hard-coded like the access subscriber for security reasons?

There are various issues around that point. Hard coding is probably not a solution (much like hard coding access made things impossible / much harder)

The grammar here is a bit patchy

Improved a bit.

can we get a follow up for this or do you intend to do it here?

It works, let's just get rid of the todos

is there a chance this could lead to stuff getting serialized inadvertently?

MH, its the same as every other service, ... we rely on the DependencySerializationTrait

Worked on the issue summary

Looks good; only one central question:

+++ b/core/core.services.yml
@@ -513,6 +513,16 @@ services:
+  lazy_route_filter:

@@ -646,21 +656,31 @@ services:
+  lazy_route_enhancer:

Why add a new service instead of making the existing one lazy?

I think the answer is what catch said in #27:

Being able to fix this without an API change is good and the patch looks encouraging.

Which is indeed great, but it also makes me question the value like Crell did in #23:

That means there's no API change here, and doesn't solve a *current* significant performance issue (as the big one was already addressed).

It's great that the current patch keeps disruption at zero. But that is not without consequences:

1. it doesn't actually fix the performance problem, because it allows the same performance-problematic API to be used
2. it ambiguates the DX: people have to know the difference and when to use which (well, there's no reason to not use the lazy version, but since both exist, they'll have to at least consider both)

If we were to do an API change instead, the entire change would boil down to a single additional method being required, which could even choose to simply return TRUE. IMHO that disruption cost is very low, because the changes are trivial, and doesn't cause a ripple effect in other parts of core. And there'd be no ambiguity in the DX.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterLazyRouteEnhancers.php
   @@ -0,0 +1,38 @@
   + * Registers all route enhancers onto the lazy route enhancers.
   

   s/all route/all lazy route/

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterLazyRouteFilters.php
   @@ -0,0 +1,38 @@
   + * Registers all route filters onto the lazy route filter.
   

   s/all route filters/all lazy route filters/

3. +++ b/core/lib/Drupal/Core/Routing/Enhancer/ParamConversionEnhancer.php
   @@ -90,4 +90,11 @@ public static function getSubscribedEvents() {
   +  public function applies(Route $route) {
   +    return TRUE;
   +  }
   

   Can't this check for the presence of {slugs}?

it doesn't actually fix the performance problem, because it allows the same performance-problematic API to be used

This route was chosen, because we don't want to hack the dynamic router of symfony CMF, which is fine.

it doesn't actually fix the performance problem, because it allows the same performance-problematic API to be used

Right, but do you really think that there aren't ton of examples where people can do crap? Just imagine if someone uses a service in a event subscriber, boom!
So what about renaming the tag from lazy_route_enhancer and route_enhancer to non_lazy_route_enhancer?

Can't this check for the presence of {slugs}?

We can't because a lot of code relies on the existance of '_raw_variables' being out there.

So what about renaming the tag from lazy_route_enhancer and route_enhancer to non_lazy_route_enhancer?

I think that'd help a bit.

The DX would still be worse than it is today though, but I guess route enhancers are not very commonly written anyway, and the clarification you suggested would help a bit. What about marking the non-lazy one as deprecated in 9.x? That sends a soft signal, subtly helping people to decide.

We can't because a lot of code relies on the existance of '_raw_variables' being out there.

:(

I couldn't find any code that'd cause problems with this though; either I'm overlooking something, or that big amount of code relying on the existence of _raw_variables doesn't affect us checking for the presence of slugs? And controllers relying on raw variables automatically implies they're not using the upcasted parameter? (Perhaps I'm saying something stupid now, but without diving into routing system internals, I think what I wrote makes sense?)

I couldn't find any code that'd cause problems with this though; either I'm overlooking something, or that big amount of code relying on the existence of _raw_variables doesn't affect us checking for the presence of slugs? And controllers relying on raw variables automatically implies they're not using the upcasted parameter? (Perhaps I'm saying something stupid now, but without diving into routing system internals, I think what I wrote makes sense?)

I don't remember which code broke, but there was certainly quite a lot of it, see earlier fails. It was more generic code than simple controllers,

Any chance that's simply no longer true due to changes in that code?

Other reviewers: do you share the concerns I raised in #72.1? Do you agree that what dawehner proposed in #73 (his lazy vs non-lazy naming proposal) would help?

Any chance that's simply no longer true due to changes in that code?

I highly doubt that, but let's try it out.

Let's use the other one, but also rename the tag.

I agree with #76that using non-lazy would be better. @dawehner in #73 you said "non_lazy_route_enhancer" but in the code #79 you used not_lazy_route_enhancer. If you ask me, I think "non_lazy_xx" sounds better than "not_lazy_xx".

I have to agree with almaudoh.

Meh

Meh

I think this needs a CR for the API change+addition? After that, this is RTBC.

Done

I clarified the CR further.

No further remarks.

Awesome. :) Passing this to catch for review, since he's our performance guru.

1. +++ b/core/core.services.yml
   @@ -525,7 +525,17 @@ services:
   +      - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
   ...
   +      - { name: non_lazy_route_filter, call: addRouteFilter }
   

   The call here is wrong.

2. +++ b/core/core.services.yml
   @@ -525,7 +525,17 @@ services:
   +  lazy_route_filter:
   

   let's call this route_filter.lazy_collector

3. +++ b/core/core.services.yml
   @@ -659,6 +669,16 @@ services:
   +  lazy_route_enhancer:
   

   Let's call this route_enhancer.lazy_collector

There we go

Committed 2b1f9f4 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

diff --git a/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
index 1f27b8d..51d0b68 100644
--- a/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
+++ b/core/lib/Drupal/Core/Routing/LazyRouteEnhancer.php
@@ -27,7 +27,7 @@ class LazyRouteEnhancer implements BaseRouteEnhancerInterface, ContainerAwareInt
   use ContainerAwareTrait;
 
   /**
-   * Array of enhancers keyed by service id.
+   * Array of enhancers service IDs.
    *
    * @var array
    */
@@ -40,6 +40,12 @@ class LazyRouteEnhancer implements BaseRouteEnhancerInterface, ContainerAwareInt
    */
   protected $enhancers = NULL;
 
+  /**
+   * Constructs the LazyRouteEnhancer object.
+   *
+   * @param $service_ids
+   *   Array of enhancers service IDs.
+   */
   public function __construct($service_ids) {
     $this->serviceIds = $service_ids;
   }
diff --git a/core/lib/Drupal/Core/Routing/LazyRouteFilter.php b/core/lib/Drupal/Core/Routing/LazyRouteFilter.php
index 7c2f404..288e09b 100644
--- a/core/lib/Drupal/Core/Routing/LazyRouteFilter.php
+++ b/core/lib/Drupal/Core/Routing/LazyRouteFilter.php
@@ -25,7 +25,7 @@ class LazyRouteFilter implements BaseRouteFilterInterface, ContainerAwareInterfa
   use ContainerAwareTrait;
 
   /**
-   * Array of filter keyed by service id.
+   * Array of route filter service IDs.
    *
    * @var array
    */
@@ -38,6 +38,12 @@ class LazyRouteFilter implements BaseRouteFilterInterface, ContainerAwareInterfa
    */
   protected $filters = NULL;
 
+  /**
+   * Constructs the LazyRouteEnhancer object.
+   *
+   * @param $service_ids
+   *   Array of route filter service IDs.
+   */
   public function __construct($service_ids) {
     $this->serviceIds = $service_ids;
   }
@@ -48,7 +54,6 @@ public function __construct($service_ids) {
    * @param \Symfony\Component\Routing\RouteCollection $route_collection
    *   A collection of routes to apply filter checks to.
    */
-
   public function setFilters(RouteCollection $route_collection) {
     /** @var \Symfony\Component\Routing\Route $route **/
     foreach ($route_collection as $route) {
diff --git a/core/lib/Drupal/Core/Routing/RouteFilterInterface.php b/core/lib/Drupal/Core/Routing/RouteFilterInterface.php
index f671a2e..bbbee1f 100644
--- a/core/lib/Drupal/Core/Routing/RouteFilterInterface.php
+++ b/core/lib/Drupal/Core/Routing/RouteFilterInterface.php
@@ -16,13 +16,13 @@
 interface RouteFilterInterface extends BaseRouteFilterInterface {
 
   /**
-   * Declaresif the route filter applies to the given route.
+   * Determines if the route filter applies to the given route.
    *
    * @param \Symfony\Component\Routing\Route $route
    *  The route to consider attaching to.
    *
    * @return bool
-   *   TRUE if the check applies to the passed route, False otherwise.
+   *   TRUE if the check applies to the passed route, FALSE otherwise.
    */
   public function applies(Route $route);

Doc fixes made on commit.