Force all route filters and route enhancers to be non-lazy

I strongly agree that simplifying this would improve DX — I've had to step through this several times in the past year, and it was hard.

If this also would improve performance, hurray!

\o/

Makes sense so far.

Added a minor initialization change which should assist with some of the fails.

#11 & #13 removed services that other services depended upon. So we also needed to remove those depending services. Let's see if this passes.

Note: this has the potential to make #2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent a lot easier, because some of the necessary routing system bugfixes would have been done by this issue. And we'd have simplified the routing system at the same time. So this helps the API-First Initiative.

Also: this is a sister issue of #2472337: Provide a lazy alternative to service collectors which just detects service IDs, which introduced the "service ID collector" concept. So associating that CR with this issue.

The changes I made #15 were too simplistic. I just removed the services that no longer worked. I should have checked whether they actually were safe to remove. They're not. #15 causes lots of REST test failures (YAY REST test coverage!).

Figuring out how to resolve this correctly…

The problem with #11/#13/#15 is that they don't determine anymore during route building which filters/enhancers should be applied to a particular route.

+++ b/core/lib/Drupal/Core/Routing/Router.php
@@ -276,8 +273,13 @@ protected function getInitialRouteCollection(Request $request) {
   protected function applyRouteEnhancers($defaults, Request $request) {
-    foreach ($this->getRouteEnhancers() as $enhancer) {
-      $defaults = $enhancer->enhance($defaults, $request);
+    $enhancer_ids = $defaults[RouteObjectInterface::ROUTE_OBJECT]->getOption('_route_enhancers');

@@ -333,8 +312,17 @@ protected function sortRouteEnhancers() {
     // Route filters are expected to throw an exception themselves if they
     // end up filtering the list down to 0.
-    foreach ($this->getRouteFilters() as $filter) {
-      $collection = $filter->filter($collection, $request);
+    $filter_ids = [];
+    foreach ($collection->all() as $route) {
+      $filter_ids = array_merge($filter_ids, $route->getOption('_route_filters') ?: []);
+    }

… but the patch so far (quoting code from #10, which passed) is still assuming that that work is actually done.

So either:

1. This must stop relying on route building pre-calculating this
2. This must start instantiating all route filters/enhancers always, and calling Route(Filter|Enhancer)Interface::applies() at runtime.

For now, going with the simplest option: number 2. In theory, it should be slower, because we're doing more work on every request. Profiling will have to show whether it's faster, slower, or the same.

But turns out that this:

+++ b/core/core.services.yml
@@ -864,10 +854,12 @@ services:
-      - { name: service_collector, tag: non_lazy_route_enhancer, call: addRouteEnhancer }
-      - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
+      - { name: service_id_collector, tag: non_lazy_route_enhancer, call: addRouteEnhancerId }
+      - { name: service_id_collector, tag: route_enhancer, call: addRouteEnhancerId }
+      - { name: service_id_collector, tag: non_lazy_route_filter, call: addRouteFilterId }
+      - { name: service_id_collector, tag: route_filter, call: addRouteFilterId }

… does not work at all. When you use service_id_collector, you can't specify a call. The collected service IDs are always passed to the service constructor. So the fact that #10 passes shows that it caused no functional changes (all the actual route filter/enhancer stuff still worked because #11+#13+#15 hadn't happened yet). This is exactly why we're doing this simplification! :)

This then actually points to the next problem: the current patch is collecting the service IDs for both route_filter and non_lazy_route_filter, and pretending like it's merging them together in the appropriate order. But it's not doing any of that. We have to retain BC, which is easy enough: the non-lazy ones should always run before the lazy ones. That's how it works in HEAD. We can easily retain that.

So I'm afraid this:

I'm not surprised by that. We now respect priorities unlike before.

is not at all correct. It's also the conclusion I'd have jumped to if I didn't actually debug the code though, so I completely understand! :) But hopefully the above explains sufficiently clearly what's actually been happening.

Attached is a patch that makes things work again. At least mostly. Looking forward to reviews/see how this patch will continue!

BTW #15 had "CI aborted" not due to some CI problem, but because it caused a majority of tests to fail, which caused the test run to take super long:

01:50:00.021 Build timed out (after 110 minutes). Marking the build as aborted.
01:50:00.028 Build was aborted

I'm fairly confident that #20 will at least successfully complete testing. It will still have test failures (I know that some REST tests will fail, more about that later), but most tests will pass.

In REST, I was seeing 405s instead of 404s when there were zero matching routes. This is because:

    $collection = $this->getInitialRouteCollection($request);
    $collection = $this->applyRouteFilters($collection, $request);

The route collection $collection returned by the first method call contained zero routes. The first filter to be applied was the MethodFilter … which throws a 405 in case of zero routes at the end of it.

So #13 is somehow changing things: "empty route collection" no longer results in a 404. I don't know yet how exactly, but this fixes it. And that in turn fixes all REST tests.

I'll leave it to @larowlan and @dawehner to fix the remaining failures.

See #2858482-39: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent, this is now blocking that issue.

Looking into this more. For now, copying the old priority onto the service that needs it most, ParamConversionEnhancer:

+++ b/core/core.services.yml
@@ -997,21 +989,11 @@ services:
-  route_enhancer.lazy_collector:
-    class: Drupal\Core\Routing\LazyRouteEnhancer
-    tags:
-      - { name: non_lazy_route_enhancer, priority: 5000 }
...
   route_enhancer.param_conversion:
     class: Drupal\Core\Routing\Enhancer\ParamConversionEnhancer
     arguments: ['@paramconverter_manager']
     tags:
-      - { name: route_enhancer }
+      - { name: route_enhancer, priority: 5000 }

#27: HAH! Of course @tim.plunkett manages to turn 183 failures into 0 failures with a single line change :D

#28:

Router::$sortedEnhancers and Router::$sortedFilters used to exist. What should we do with them?

They're an implementation detail that we don't need anymore. I don't see a problem with them being gone? Oh, #2810303: Reunite the router: One router to rule them all added them and made them available via public getters, darn. But AFAICT that was a mere accident? The API is \Symfony\Component\Routing\RouterInterface, not \Drupal\Core\Routing\Router. So I still think it's fine to remove them.

Router::addRouteEnhancer and Router::addRouteFilte no longer exist. We could add them back and then let them do nothing?

These were definitely implementation details. The only reason those were public, is because the old service_collector pattern needed public methods to inject them. You can still add route enhancers/filters: tag them appropriately. This is what matters.

+++ b/core/lib/Drupal/Core/Routing/Router.php
@@ -148,6 +120,9 @@ public function match($pathinfo) {
+    if ($collection->count() === 0) {
+      throw new ResourceNotFoundException(sprintf('No routes found for "%s".', $this->currentPath->getPath()));
+    }

This additional condition is a bit weird, IMHO, what about the changes in the code?

I agree. See the explanation in #24. I didn't find a more elegant way to solve it. You know the routing system better, you probably can figure out what code was doing this before? I already did much of the debugging — see #24 :)

I think we should avoid doing that. This is all why we had the lazy router filters/enhancers in the first place. Otherwise I don't see why we actually have to change the router at all. This is exactly what it is doing right now :)

Fair!

#29: I don't understand this change in direction?

I got this tell yesterday from @dawehner:

14:55:37 <WimLeers> .
14:55:37 <drupalbot> WimLeers: 18 hours 35 min ago <dawehner> tell WimLeers Do you think its really ok to disable the lazy thing from any router filter/enhancer in https://www.drupal.org/node/2883680 ? In case we can simplify the patch even more

Thoughts below.

I think we should avoid doing that. This is all why we had the lazy router filters/enhancers in the first place. Otherwise I don't see why we actually have to change the router at all. This is exactly what it is doing right now :)

Fair!

OTOH, the issue title says get rid of lazy route filters and route enhancers, which is why I thought this was part of the plan. And I'm not sure it'd be that much slower since:

1. Lazy route filters ContentTypeHeaderMatcher and MethodFilter already run 100% of the time (that's 66% of our route filters). Only RequestFormatRouteFilter is actually lazy.
2. Lazy route enhancer ParamConversionEnhancer runs 100% of the time and it's the most costly one. The other route enhancers do seem to benefit from being lazy.

Essentially: ignoring the applies() method on route filters and enhancers that Drupal layered on top would make things simpler: because then we're back to just the original concept, not our forked version of it.

But that's of course a bigger change, and should not delay this issue — I think it's out of scope to change that here. I simply took a shortcut in #20 to get the patch to mostly work. I still agree with @dawehner in #28.

Tim told me yesterday that they switched away from using lazy route filters for page manager and just run all the time. Given that I think its not realistic to keep them lazy anymore indeed.

Oh, interesting!

Its not a full change of direction. It just changes from initialising the services in the constructor, to using the tagged handler capability to inject the actual objects.

D'oh, right!

I guess its fine to drop those things in the next minor version, right?

IMO yes.

Also keep in mind, the access aware router is probably on top of that as well, so people cannot just directly access those methods, unless they inject this specific user.

Exactly! That means the likelihood of somebody using the router.no_access_checks service directly and using that (newly added in the previous minor!) addRouteEnhancer() method is as close to zero as it can get.

Does that mean the only thing that remains to be done here is profiling?

Page Manager had to switch to non-lazy route filters, in order to use priority.
http://cgit.drupalcode.org/page_manager/commit/?id=292cddfeca17bea8b95fb...

The key to this change is that your ::filter() method should exit as soon as possible. I essentially moved the logic from the old ::applies() method into the top of ::filter()

  public function applies(Route $route) {
    $parameters = $route->getOption('parameters');
    return !empty($parameters['page_manager_page_variant']);

  public function filter(RouteCollection $collection, Request $request) {
    $routes = $collection->all();
    // Only continue if at least one route has a page manager variant.
    if (!array_filter($routes, function (Route $route) {
      return $route->hasDefault('page_manager_page_variant');
    })) {
      return $collection;
    }

Page Manager had to switch to non-lazy route filters, in order to use priority.

I see, so also because of this bug, which was also surfaced in #2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent and #2839658: LazyRouteFilter doesn't handle priority.

The key to this change is that your ::filter() method should exit as soon as possible. I essentially moved the logic from the old ::applies() method into the top of ::filter()

Yep. And that's how Symfony's route filters/enhancers are actually meant to be used.

We should deprecate Drupal's extending interfaces for Drupal 9, and we can keep calling applies() in Drupal 8 to maintain BC.

Thanks for the clarifications Tim!

1. +++ b/core/lib/Drupal/Core/Routing/Enhancer/FormRouteEnhancer.php
   @@ -2,25 +2,24 @@
   +use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface as BaseRouteEnhancerInterface;
   ...
   +class FormRouteEnhancer implements BaseRouteEnhancerInterface {
   

   Guessing this is an artifact of refactoring, but you shouldn't need the alias now (here and a couple other places)

2. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -148,6 +122,9 @@ public function match($pathinfo) {
   +    if ($collection->count() === 0) {
   +      throw new ResourceNotFoundException(sprintf('No routes found for "%s".', $this->currentPath->getPath()));
   +    }
   

   Do we still need this addition?

3. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -333,7 +279,8 @@ protected function sortRouteEnhancers() {
   +      // @todo should we respect ::applies()? The problem is that we can't, because no route has been matched yet. So we should probably remove \Drupal\Core\Routing\RouteFilterInterface?
   

   Hmm. Still need to figure this out

This should make the CI test run at least succeed (although I think there will still be a number of failures because #39.2 was removed — but we'll see!).

Boatloads of failures like this in the test results:

HTTP response expected 404, actual 405

And not just in REST.

My analysis from #24 is relevant again. So bringing back that fix.

#48 sounds sensible.

Rerolled to make one more test pass.

👏 for pushing this one forward again! ❤️

And WOW! YAY! GREEN PATCH! Love the fact that you were able to make virtually no changes to MethodFilter.

I think the most important thing remaining is profiling? (Besides reviews from the routing system maintainer Tim Plunkett.) Thoughts on how we should do the profiling? Would benchmarking a bunch of URLs (frontpage, node page, REST) instead of profiling be sufficient?

1. +++ b/core/lib/Drupal/Core/Routing/Enhancer/FormRouteEnhancer.php
   @@ -2,25 +2,24 @@
   -  public function applies(Route $route) {
   -    return $route->hasDefault('_form') && !$route->hasDefault('_controller');
   ...
   +    $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];
   +    if (!$route->hasDefault('_form') || $route->hasDefault('_controller')) {
   +      return $defaults;
   +    }
   

   This is a bit uglier, but I get it.

2. +++ b/core/lib/Drupal/Core/Routing/Router.php
   @@ -3,9 +3,9 @@
   -use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface as BaseRouteEnhancerInterface;
   +use Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface;
   
   @@ -276,47 +253,16 @@ protected function getInitialRouteCollection(Request $request) {
   +      if ($enhancer instanceof RouteEnhancerInterface || $enhancer->applies($defaults[RouteObjectInterface::ROUTE_OBJECT])) {
   

   This conditional is confusing.

   All legacy implementations will pass the first part, and not actually run the applies?

   I'd expect it to be something more like

   use Drupal\Core\Routing\Enhancer\RouteEnhancerInterface as LegacyRouteEnhancerInterface;
   if ($enhancer instanceof LegacyRouteEnhancerInterface && !$enhancer->applies($defaults[RouteObjectInterface::ROUTE_OBJECT)) {
     continue;
   }
   
   $enhancer->enhance($defaults, $request);
   

3. +++ b/core/tests/Drupal/KernelTests/Core/Routing/ContentNegotiationRoutingTest.php
   @@ -49,36 +49,36 @@ public function testContentRouting() {
   -      ['conneg/simple.json', '', 'application/json'],
   -      ['conneg/simple.json', 'application/xml', 'application/json'],
   -      ['conneg/simple.json', 'application/json', 'application/json'],
   +//      ['conneg/simple.json', '', 'application/json'],
   +//      ['conneg/simple.json', 'application/xml', 'application/json'],
   +//      ['conneg/simple.json', 'application/json', 'application/json'],
   

   Can't commit this with all these commented out.

4. +++ b/core/tests/Drupal/Tests/Core/Entity/Enhancer/EntityRouteEnhancerTest.php
   @@ -62,13 +67,9 @@ public function testEnhancer() {
   -    $route = $this->getMockBuilder('Symfony\Component\Routing\Route')
   -      ->disableOriginalConstructor()
   -      ->getMock();
   -
   -    $route->expects($this->any())
   -      ->method('getOptions')
   -      ->will($this->returnValue($options));
   +    $route = new Route('/test');
   +    $route->setOptions($options);
   +    $route->setDefaults($defaults);
   

   😍

This is a BC break of sorts, because applies() is moved from compile-time to runtime.

The trigger_error and deprecation notice do not indicate this.

The only other possibility for #56.1 I see would be to NOT deprecate it, and only make the switch to checking at runtime.
It results in cleaner code, but still a Drupalism.

If that's not preferred, then this is fine.

But we should definitely clarify the change in behavior that exists within the BC layer.

BC break is not quite accurate. But it could be possible that people were lazy and wrote slow crappy code in there knowing it was compile-time only? And now it is run on every route.

Not sure about the @todo yet.

Overall looks like an improvement, but some small questions / comments

1. +++ b/core/core.services.yml
   @@ -823,16 +823,6 @@ services:
   -  route_filter.lazy_collector:
   -    class: Drupal\Core\Routing\LazyRouteFilter
   -    tags:
   -      - { name: non_lazy_route_filter }
   

   Yay - This combination of service name and tag confused me every time I looked at it.

2. +++ b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
   @@ -4,13 +4,13 @@
   +use Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface as BaseRouteFilterInterface;
   

   Why does the Symfony interface need to be aliased here? Becuase the Drupal interface is in the same namespace?

Yay - This combination of service name and tag confused me every time I looked at it.

Haha, exactly! Very recognizable 😀

So there are only two remaining questions here I think:

1. Does this affect performance? (Needs profiling)
2. Is calling applies() at run time instead of at compile time a BC break? (See #59 + #60 + #61 + #62)

I benchmarked two cases.

Frontpage: ab -c 1 -n 100 http://d8/

Before

Concurrency Level:      1
Time taken for tests:   0.367 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      1321200 bytes
HTML transferred:       1151700 bytes
Requests per second:    272.44 [#/sec] (mean)
Time per request:       3.671 [ms] (mean)
Time per request:       3.671 [ms] (mean, across all concurrent requests)
Transfer rate:          3515.09 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     3    4   0.6      3       6
Waiting:        3    3   0.6      3       6
Total:          3    4   0.7      3       7
WARNING: The median and mean for the processing time are not within a normal deviation
        These results are probably not that reliable.

After

Concurrency Level:      1
Time taken for tests:   0.369 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      1321200 bytes
HTML transferred:       1151700 bytes
Requests per second:    270.98 [#/sec] (mean)
Time per request:       3.690 [ms] (mean)
Time per request:       3.690 [ms] (mean, across all concurrent requests)
Transfer rate:          3496.24 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     3    4   0.5      3       5
Waiting:        3    3   0.4      3       5
Total:          3    4   0.5      4       6
ERROR: The median and mean for the processing time are more than twice the standard
       deviation apart. These results are NOT reliable.

REST response: ab -c 1 -n 100 -H 'Authorization: Basic SOMEHASH' http://d8/node/2?_format=hal_json

Before

Concurrency Level:      1
Time taken for tests:   6.497 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      282600 bytes
HTML transferred:       180300 bytes
Requests per second:    15.39 [#/sec] (mean)
Time per request:       64.971 [ms] (mean)
Time per request:       64.971 [ms] (mean, across all concurrent requests)
Transfer rate:          42.48 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    60   65   3.3     64      80
Waiting:       60   65   3.3     64      80
Total:         60   65   3.3     65      80

After

Concurrency Level:      1
Time taken for tests:   6.425 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      282600 bytes
HTML transferred:       180300 bytes
Requests per second:    15.56 [#/sec] (mean)
Time per request:       64.254 [ms] (mean)
Time per request:       64.254 [ms] (mean, across all concurrent requests)
Transfer rate:          42.95 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    60   64   3.1     63      77
Waiting:       60   64   3.1     63      77
Total:         60   64   3.1     63      77

To me, this is sufficiently reassuring that we're not regressing in a meaningful way, that there are no significant performance changes. If the routing system maintainers want to see more specific data, then they can ask for that.

I still believe nothing could have relied on that actively ...

I agree.

I'd RTBC this patch, but given the above, I think we need a routing system maintainer to RTBC.

Also, changing some issue metadata:

Its hard to debug

-> major

The priorities are not taken into account properly

-> bug

+++ b/core/lib/Drupal/Core/Routing/Router.php
@@ -333,7 +281,8 @@ protected function sortRouteEnhancers() {
+      // @todo should we respect ::applies()? The problem is that we can't,because no route has been matched yet. So we should probably remove \Drupal\Core\Routing\RouteFilterInterface?

Let's just get rid of the @todo. The linked CR is sufficient.

I'll mark this RTBC as soon as that is removed!

Thanks! While I was about to RTBC I looked at the issue title again.
ClassProvider isn't a thing, I think it meant ClassResolver?
Except we don't actually use that!

Is the new issue title better?

1. +++ b/core/core.services.yml
   @@ -864,10 +854,12 @@ services:
   -    arguments: ['@router.route_provider', '@path.current', '@url_generator']
   +    arguments: ['@router.route_provider', '@path.current', '@url_generator', '@class_resolver']
   

   The class resolver isn't actually used!

2. +++ b/core/core.services.yml
   @@ -864,10 +854,12 @@ services:
          - { name: service_collector, tag: non_lazy_route_enhancer, call: addRouteEnhancer }
   +      - { name: service_collector, tag: route_enhancer, call: addRouteEnhancer  }
          - { name: service_collector, tag: non_lazy_route_filter, call: addRouteFilter }
   +      - { name: service_collector, tag: route_filter, call: addRouteFilter }
   

   Should any of these tags have @todo/@deprecated to eventually consolidate theme?

Thanks!
On commit that "Todo" could be made "@todo", but close enough :)

@tim.plunkett Thanks for your attention to detail 👍

Adding the related issue for #72.2/#73.

Also, 🎉🎉🎉! Can't wait for this to land. This has been blocking #2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent for 3 months (since #2883680-39: Force all route filters and route enhancers to be non-lazy), which in turn is blocking #2853460: Simplify RequestHandler: make it no longer ContainerAware, split up ::handle(), which in turn is blocking #2737751: Refactor REST routing to address its brittleness. Once this is in, we'll finally be able to land a whole bunch of important REST technical debt ❤️

Thank you so much, Daniel & Tim!

1. +++ b/core/core.services.yml
   @@ -823,16 +823,6 @@ services:
        deprecated: The "%service_id%" service is deprecated. You should use the 'router.no_access_checks' service instead.
   -  route_filter.lazy_collector:
   -    class: Drupal\Core\Routing\LazyRouteFilter
   -    tags:
   -      - { name: non_lazy_route_filter }
   -    parent: container.trait
   -  route_filter_subscriber:
   -    class: Drupal\Core\EventSubscriber\RouteFilterSubscriber
   -    arguments: ['@route_filter.lazy_collector']
   -    tags:
   -      - { name: event_subscriber }
   

   Should some of these services be left in and deprecated, except without the tags? Seems academic though, can't imagine a case they'd be used.

2. +++ b/core/lib/Drupal/Core/Routing/Enhancer/EntityRevisionRouteEnhancer.php
   @@ -5,16 +5,19 @@
   -   * {@inheritdoc}
   +   * Returns whether the enhancer runs on the current route.
   +   *
   +   * @return bool
       */
   -  public function applies(Route $route) {
   +  protected function applies(Route $route) {
        // Check whether there is any entity revision parameter.
        $parameters = $route->getOption('parameters') ?: [];
        foreach ($parameters as $info) {
   @@ -31,6 +34,10 @@ public function applies(Route $route) {
   
   @@ -31,6 +34,10 @@ public function applies(Route $route) {
      public function enhance(array $defaults, Request $request) {
        /** @var \Symfony\Component\Routing\Route $route */
        $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];
   +    if (!$this->applies($route)) {
   +      return $defaults;
   +    }
   +
   

   Why not inline the logic rather than making protected?

Also I'd prefer to see profiling on here than ab, ab rarely gives useful information on death by a thousand cuts-type changes (which this probably is one way or the other).

+++ b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
@@ -4,13 +4,13 @@
 use Symfony\Component\Routing\RouteCollection;
+use Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface as BaseRouteFilterInterface;
 
 /**
  * Filters routes based on the HTTP Content-type header.
  */
-class ContentTypeHeaderMatcher implements RouteFilterInterface {
+class ContentTypeHeaderMatcher implements BaseRouteFilterInterface {
 

Not really keen on exposing the CMF interface directly, especially when we have to alias it. Could we make a new empty core interface and use that everywhere instead?

1)
I don't see any of these as valuable to leave in, especially the subscribers.

2)
I think the intent was to go for "the most clear diff"? Which I appreciate actually, as someone who reads old commits to derive intent...

Profiling I leave to someone else.

+++ b/core/modules/system/tests/modules/accept_header_routing_test/src/Routing/AcceptHeaderMatcher.php
@@ -2,10 +2,9 @@
-use Drupal\Core\Routing\RouteFilterInterface;
+use Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface;

We already have things like Symfony\Cmf\Component\Routing\RouteProviderInterface and Symfony\Cmf\Component\Routing\RouteObjectInterface sprinkled throughout code, I don't think this is a problem. That said, I wouldn't mind reinventing CMF ourselves, last I checked there's not much too it and we had to subclass most of the important stuff...

But, in this case the aliasing only affects classes in the \Drupal\Core\Routing directory, this is the only change other code will have to make.

AFAIC the only thing left from CMF that we use with no alteration is RouteObjectInterface, and *only* for the ROUTE_NAME/ROUTE_OBJECT constants.
I'd propose creating an empty RouteEnhancerInteface and RouteFilterInterface in Drupal\Core\Routing that extends the CMF ones, but leaves us open to eventually decoupling. The current CR and code change move us *closer* to CMF which is bad.

Also I'd prefer to see profiling on here than ab, ab rarely gives useful information on death by a thousand cuts-type changes (which this probably is one way or the other).

Profiling I leave to someone else.

Yeah I totally agree. Being able to see where things actually improved/got worse is also super valueable.

I did benchmarking with ab because I know how to pick a common and edge case of routing. To profile this properly, one needs to very thoroughly understand the routing system, because you need to understand the moving parts affected by this patch and the way they are called, in detail.
I can take this on, but I'd frankly be more comfortable/have more trust in the profiling results done by somebody who knows the routing system in more detail than I do.

@dawehner: Thanks for profiling.

. Maybe we should add route filters explicitly?

This sounds like a good plan to add to the docs.

At least for me it made it more readable to have this apply method not inline, but on the other hand I was also just lazy.

:) this and smaller diff are both OK explanations. I had to double take on it because on first glance I was "why is this being made protected, how will it get called?" based on earlier discussions in the issue about just calling applies() all the time which is why I brought it up.

@timplunkett

AFAIC the only thing left from CMF that we use with no alteration is RouteObjectInterface, and *only* for the ROUTE_NAME/ROUTE_OBJECT constants.
I'd propose creating an empty RouteEnhancerInteface and RouteFilterInterface in Drupal\Core\Routing that extends the CMF ones, but leaves us open to eventually decoupling. The current CR and code change move us *closer* to CMF which is bad.

Yes this is where I'm going with it. This doesn't take us from zero to some coupling, but it takes us from some to more when we probably want to move in the other direction medium term.

Opened #2917331: Decouple from Symfony CMF.

@dawehner: 👌

Ideally that would be a follow-up issue. But if we put out a CR saying "stop using our RFI and REI and use the ones from CMF" then we're just creating more work for ourselves.

I propose we figure out what to do with only those two interfaces first, and then move onto the rest of the decoupling.

Awesome, thanks! Here's some additional cleanup for that latest change.

Signing off on this from the perspective of the Routing maintainer.

1. +++ b/core/lib/Drupal/Core/Entity/Enhancer/EntityRouteEnhancer.php
   @@ -2,20 +2,28 @@
   +    $route = $defaults[RouteObjectInterface::ROUTE_OBJECT];
   +    if ($route->hasDefault('_controller') ||
   +      !($route->hasDefault('_entity_form')
   +        || $route->hasDefault('_entity_list')
   +        || $route->hasDefault('_entity_view')
   +      )) {
   +      return $defaults;
   
   @@ -30,17 +38,6 @@ public function enhance(array $defaults, Request $request) {
   -  public function applies(Route $route) {
   -    return !$route->hasDefault('_controller') &&
   -      ($route->hasDefault('_entity_form')
   -        || $route->hasDefault('_entity_list')
   -        || $route->hasDefault('_entity_view')
   -      );
   

   I like the way we made this protected in the revision flavour. I'd be in support of doing that here too. It's more readable. Same in a couple of other places like the field UI one.

2. +++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
   @@ -27,22 +19,34 @@ public function filter(RouteCollection $collection, Request $request) {
   +      if (($supported_formats = array_filter(explode('|', $route->getRequirement('_format')))) && in_array($format, $supported_formats)) {
   

   Can $format be built from user-supplied data, i.e. ?_format=json in which case should we use the third arg to in_array here?

3. +++ b/core/lib/Drupal/Core/Routing/RouteFilterInterface.php
   @@ -2,13 +2,18 @@
   + * See https://www.drupal.org/node/2894934'
   

   nit: extra trailing '

Do we need to do profiling here?

For profiling: see #83.

The new interfaces in #90 look great and just enough to stop us digging a hole and filling it in again, we can/should do any further decoupling in the follow-up.

Looks great.

-use Symfony\Component\Routing\Route;
This needs to be added back to EntityRouteEnhancer and FormRouteEnhancer

Once more, with feeling

Committed 1cbbff1 and pushed to 8.5.x. Thanks!

Updated CR metadata & published it: https://www.drupal.org/node/2894934.

This also unblocked #2858482 — see #2858482-41: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent!

Couple of questions:

1. The deprecation message says "deprecated in 8.4.0" but looking at the commit above, it is only in 8.5 so far. Is the plan to commit this to 8.4 or does the message need changing?
2. The commit was on 24th October, but my daily contrib testing at 8.5 only started to fail for the deprecation message from 10th November. See https://www.drupal.org/pift-ci-job/809070 Is there something else I am missing, as to why tests passed during this intervening time?

Thanks
Jonathan

@jonathan1055: the test fails started when #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code was committed. That is the issue that activated test failures due to use of deprecated API.

@dawehner: Is there a recommended way of doing this and maintaining backward compatibility with 8.4.x? Simply replacing class MyRouteEnhancer implements RouteEnhancerInterface { with class MyRouteEnhancer implements EnhancerInterface { will break on 8.4.x since the EnhancerInterface doesn't yet exist in that branch.

Thanks almaudoh. The link to that issue is helpful.

I am trying to fix an occurrence of the deprecated code in the Rules module, and followed the example in the CR.

However I think there might be an error in the after example for the 'use' statement, because the class Drupal\Core\Routing\Enhancer\EnhancerInterface does not exist. I think it should be Drupal\Core\Routing\EnhancerInterface which does exist and allows my 3rd-party tests which implement Rules conditions and actions to pass again, after failing at 8.5.

Apologies for writing to this closed issue, but it seemed the best to get the attention of those who were involved in the CR for this change.

This is causing a problem as described in #6 of #2922757-6: Replace deprecated RouteEnhancerInterface, then remove @group legacy where after fixing the code to remove the deprecated class, it runs in 8.5 but fails in 8.4 and 8.3

Fortunately there appears to be a simple solution - just add the two new interface files to 8.4 and 8.3. I have tested this locally and it allows tests to pass at 8.5, 8.4 and 8.3 after switching to the new classes. Here is the patch for 8.4 and 8.3 (not 8.5) and it should pass but won't actually prove anything yet as there are no Core tests in 8.4 and 8.3 which use the new class.

Please can a maintainer re-open this issue so that I can test the patch at 8.4.x and maybe even add a new test to cover it. We can't just leave this at 8.5 only. Thanks.

Also having this same issue at #2922871: Fix test failures in D8.5 due to use of deprecated APIs. Basically, the new interface Drupal\Core\Routing\EnhancerInterface needs to exist in D8.4 so contrib can switch to the new API, right now we have a situation where tests fail either in 8.5 or in 8.4

@almaudoh,
Can you apply my core patch from 112 locally, and see if this allows your tests to pass at 8.4 and 8.3? It works for me, testing Rules and Scheduler, so I am hopeful it will work for all. But it is always good to get more evidence.

Thanks
Jonathan

Given that this issue is closed I have raised #2924899: Backport \Routing\EnhancerInterface to core 8.4 for the backport to 8.4 and 8.3 and uploaded the patch from #112 above to that issue.

#108:

The deprecation message says "deprecated in 8.4.0" but looking at the commit above, it is only in 8.5 so far. Is the plan to commit this to 8.4 or does the message need changing?

Well-spotted! Created #2926412: Follow-up for #2883680: deprecation message indicates the wrong deprecation introduction version.

It broke the page_manager module #2918564: Update 'page_manager.variant_route_filter' service according to core changes. Please help with the review/commit so that we can fix it before 8.5.0,

This also has broken people's views - see #2959370: View with user/% path breaks login/logout