Page Manager routing is overly complex and brittle: breaks REST, active trail, fallback pages

This should fix it.

Looks great, and looks identical to #2730497: REST Views override existing REST routes in approach.

+++ b/tests/src/Unit/PageManagerRoutesTest.php
@@ -191,12 +191,13 @@ public function testAlterRoutesOverrideExisting($page_path, $existing_route_path
+    $collection->add("$route_name.POST", new Route($existing_route_path, ['default_exists' => 'default_value'], $requirements, ['parameters' => ['foo' => ['type' => 'bar']]], '', [], ['POST']));

Changing ['POST'] to ['GET', 'POST'] breaks the test. Maybe this logic isn't completely correct?

What about this? Honestly I'm not sure what to do here.

Why not just go with the solution we have in core for now? We can refine it later.

I ran into issues with routes that contain _format. The following patch fixes that for me but I have absolutely no idea why..

My problem: I have a module that adds a format to the /node/{node} route. Page manager is also enabled. For some reason when page manager is enabled it no longer recognizes the path with the _format param. Instead we get:

{
"message": "Not acceptable format: my_custom_format"
}

With this patch and a cache clear, that route is working as expected again. Using Drupal 8.2 and latest alpha of page manager

Made some typos in the patch above

Oh boy. Not making the best impression. I should not upload patches I don't test locally.

Steps to reproduce:

Install contenthub & page_manager

composer config repositories.drupal composer https://packages.drupal.org/8
composer require drupal/acquia_contenthub:~1.0
composer require drupal/page_manager:~1.0@alpha
drush en acquia_contenthub
drush en page_manager
drush en page_manager_ui

Setup the Drupal Site

Make sure you have the Article Content Type
go to admin/config/services/acquia-contenthub/configuration
Click the Publish Checkbox of "CONTENT » ARTICLE"
Click save at the bottom of the page
Create an Article with random content

Verify that the rest endpoint is working

Clear caches
Go to /node/1?_format=acquia_contenthub_cdf
Verify you see JSON output

Import Page Manager Config

page_manager.page.node_view.yml

uuid: 8201fae0-e836-41a9-968f-38c6c6fbbe7a
langcode: en
status: true
dependencies: {  }
_core:
  default_config_hash: RCVWP-yHwxSNiQORMIabDgHMEVqOMW58w80BQgRFJ4k
id: node_view
label: 'Node view'
description: 'When enabled, this overrides the default Drupal behavior for displaying nodes at <em>/node/{node}</em>. If you add variants, you may use selection criteria such as node type or language or user access to provide different views of nodes. If no variant is selected, the default Drupal node view will be used. This page only affects nodes viewed as pages, it will not affect nodes viewed in lists or at other locations.'
use_admin_theme: false
path: '/node/{node}'
access_logic: and
access_conditions: {  }
parameters:
  node:
    machine_name: node
    type: 'entity:node'
    label: Node

page_manager.page_variant.node_view-http_status_code-0.yml

uuid: 3ea5427a-c9cf-4282-ac3e-c6ca0f46b412
langcode: en
status: true
dependencies:
  config:
    - page_manager.page.node_view
  module:
    - ctools
id: node_view-http_status_code-0
label: News
variant: http_status_code
variant_settings:
  id: http_status_code
  uuid: e4fa4ee3-c61d-47a3-95f5-ad398ac05bb1
  label: null
  weight: 0
  status_code: 200
page: node_view
weight: 0
selection_criteria:
  -
    id: 'entity_bundle:node'
    bundles:
      article: article
    negate: false
    context_mapping:
      node: node
selection_logic: and
static_context: {  }

Verify we can no longer access our special url

Go to /node/1?_format=acquia_contenthub_cdf
Verify you see
{"message":"Not acceptable format: acquia_contenthub_cdf"}

As it turns out, the _format handling already was happening in \Drupal\Core\Routing\RequestFormatRouteFilter::filter()
However, route filters ignore any sort of priority when they are managed by LazyRouteFilter (which is by default).
And RequestFormatRouteFilter was running too late, which meant page_manager's VariantRouteFilter was picking the variant to use before factoring in _format info (as the last patch tried to fix).

In order to order route filters, they must be declared as "non_lazy_route_filter".

Once that was fixed, it became clear that \Drupal\page_manager\Routing\VariantRouteFilter::routeWeightSort() was mangling the order prepared by RequestFormatRouteFilter. Changing it to preserve order by default fixed that problem.

Finally, while writing test coverage, it became clear that display variants don't have a generic schema to fall back on. Opened #2838130: Display variants are missing a generic schema but added a workaround here.

#22 didn't actually solve the problem described in #21 though, but another issue with ordering.
Expanded the test coverage, and removed the reordering from VariantRouteFilter which should also fix #21.

Confirming this solves the issue of #21! Thanks Tim!

In going back through the whole flow of the patch, I noticed some conspicuous issues, and went to fix them as well. Technically could be out of scope, but the issue is basically "PM routing doesn't work like you think it does and is bad" now...

+++ b/src/Routing/PageManagerRoutes.php
@@ -139,7 +139,10 @@ protected function findPageRouteName(PageInterface $entity, RouteCollection $col
+        !$collection_route->hasRequirement('_format')) {

I keep wondering if this is the only differentiator that makes a route unique. Do we have any other differentiators how a route could be different other than _format?

Not sure either, really.
Also super glad that last patch failed, because I made a mistake and the unit tests didn't cover it.

Working on this

Okay this fixes everything... except PageManagerTranslationIntegrationTest, which seems to currently pass by coincidence.
I went back and found when it was added: #2342923: content_translation fails when node/% is overridden with a route.
No idea what was going on in that issue, but just removing the test for now.

It should become easier to fix the following now:
#2634276: Multiple routes break active trail and possibly other behaviors
#2782661: Page variant for node view causes internal error in other content types
#2659948: Remove support for paths with %, provide validation and description for path

Turns out #2634276: Multiple routes break active trail and possibly other behaviors is the fix for PageManagerTranslationIntegrationTest, adding that back in.

#2782661: Page variant for node view causes internal error in other content types is also fixed here.

Not making the % change because that needs UI stuff too, and is an easy standalone fix

Okay tracked down the problem. 'base_route_name' is only tracked and cleared for overridden routes, and yet we were adding it to all routes. That's unnecessary, and now interferes with the new checking in VariantRouteFilter.

A good amount of the interdiff is just undoing changes to tests, sorry.

Silly bug around weight being 0, fix and coverage included

Hm. Does actually does seem to partially fix the problem reported in #2837833: 'Required contexts without a value: node' when editing menu, but only partially and in a way that breaks my workaround.

In the site that I was debugging, I have a page_manager page with a path like /foo/{type} and then multiple variants with a selection condition on the path. Without this, it fails to match anything, resulting in url = base:foo/bar in the menu_tree table.

Now with the patch, it always uses the first route name, so the menu_tree table contains route_name = page_manager....foo_bar but then type=othervalue in route_param_key and same for route_parameters.

The problem is that this means my workaround to look up active trail based on base:foo/bar, if there is nothing matching the route name, now fails and the active trail only works for the first variant and not the others. In this case those variants could easily be completely different pages without an argument, but in case of overriden node/{node} or similar pages, with selection conditions for e.g. the bundle, that's not an option.

This keeps the base route name for all managed routes by switching to an "overridden_route_name" for the filtering.

Also finally fixed the sorting bugs, yowch.

1. +++ b/src/Routing/PageManagerRoutes.php
   @@ -60,20 +60,22 @@ protected function alterRoutes(RouteCollection $collection) {
   +        $defaults['overridden_route_name'] = $overridden_route_name;
   

   Likely should add some docs here

2. +++ b/src/Routing/PageManagerRoutes.php
   @@ -84,26 +86,20 @@ protected function alterRoutes(RouteCollection $collection) {
   +      $defaults['base_route_name'] = $base_route_name;
   ...
   -            // When adding multiple variants, the variant ID is added to the
   -            // route name. In order to convey the base route name for this set
   -            // of variants, add it as a parameter.
   -            'base_route_name' => $route_name,
   

   This comment should be moved, not deleted.

Unassigning now that it passes, will get to the docs next week if no one else does.

Thanks!

I think I did find another problem. Some of my tests failed, and from what I see, this seems to happen because somehow this breaks the route name for views that override routes. I can see this on /admin/content for example, the local tasks and actions there are gone.

Not quite sure what is happening. Tell me if you can't reproduce.

I also had some other test fails that were because the route names changed as the special case for the first variant was removed. I can live with that, updated my code, just FYI.

I also noticed that the page manager variant filter isn't within the lazy route filter like core's route filters are. Not sure if that's some optimization in core that this module wasn't updated for or if that is actually on purpose?

Ah. The page variant is moved here, so that seems on purpose. Maybe that makes it run on pages where it shoudn't do anything now and we need some other way of checking that?

Walked through this with @EclipseGC, these are the only changes.

I also noticed that the page manager variant filter isn't within the lazy route filter like core's route filters are. Not sure if that's some optimization in core that this module wasn't updated for or if that is actually on purpose?

+++ b/page_manager.services.yml
@@ -19,9 +19,10 @@ services:
-      - { name: route_filter }
+      # Run as late as possible to allow all other filters to run first.
+      - { name: non_lazy_route_filter, priority: -1024 }

The ordering of lazy route filters is nondeterministic (because of \Drupal\Core\Routing\LazyRouteFilter)
In order to not recreate the functionality of those other route filters, we need ours to run last.
The only way to order route filters is to use non-lazy route filters, and rely on the normal priority system.

I have no idea how this interacts with Views, do I really want to know?

The key takeaway from this change is that the {router} table entries now all use page manager naming scheme, which is page_manager.page_view_{page_id}_{variant_id}. PageManagerRoutes no longer affects existing routes.

VariantRouteFilter now does all of the manipulation, and it's last action (for routes overriding existing stuff) is to spoof the route name of the route we're overriding.

Maybe that makes it run on pages where it shoudn't do anything now and we need some other way of checking that?

Not sure what "that", "it", "it", and "that" mean in this context, sorry!

To address #43, maybe something like this?

diff --git a/src/Routing/VariantRouteFilter.php b/src/Routing/VariantRouteFilter.php
index c4b6837..845b108 100644
--- a/src/Routing/VariantRouteFilter.php
+++ b/src/Routing/VariantRouteFilter.php
@@ -84,12 +84,15 @@ public function filter(RouteCollection $collection, Request $request) {
     $routes = $collection->all();
     uasort($routes, [$this, 'routeWeightSort']);
 
+    $page_manager_routes_exist = FALSE;
     $variant_route_name = $this->getVariantRouteName($routes, $request);
     foreach ($routes as $name => $route) {
       if (!$route->hasDefault('page_manager_page_variant')) {
         continue;
       }
 
+      $page_manager_routes_exist = TRUE;
+
       // If this page manager route isn't the one selected, remove it.
       if ($variant_route_name !== $name) {
         unset($routes[$name]);
@@ -101,6 +104,10 @@ public function filter(RouteCollection $collection, Request $request) {
       }
     }
 
+    if (!$page_manager_routes_exist) {
+      return $collection;
+    }
+
     // Create a new route collection by iterating over the sorted routes, using
     // the overridden_route_name if available.
     $result_collection = new RouteCollection();

I'm out for the rest of the weekend, but will be around next week.

I can confirm that this fixes the problem for me. I haven't figured out what exactly causes it though, couldn't reproduce on a new empty standard installation.

All my tests are now passing when including that interdiff, so everything OK as far as I'm concerned :)

So it fails with #44 but #44 + #47 passes?
The change makes sense, but I can't actually figure out how to write a unit test for it. Seems like a sort problem, possibly...

Yes, exactly.

But yes, I also couldn't reproduce manually, not sure what exactly is necessary to get that bug.

Can I see the failing testcase? I can step through it with xdebug and see what's going on.

Well, it's a version issue.

Tried to simplify but I still had to resort to a version_compare :(

Removed the applies() because Drupal\Core\Routing\RouteFilterInterface is only used by lazy route filters.

I was finding that I was unable to override existing routes (either in a new page, or even adding a new variant to the Node View page.) This ended up being the closest related issue I could find (which is surprising - maybe I missed something in my search.) On the plus side, the latest patch on this issue appears to resolve the problem and allows me to override existing routes.

Switching to array_multisort since uasort is completely unstable and differs between PHP 5 and 7.

I'm marking this RTBC because I intend to commit it. But hopefully @Berdir and @Nick_vh could double check that everything is as expected still?

Looks good with manual testing, my tests are running now.

Everything looks good on my side, go ahead :)

All good here as well. Thanks!

Wow, this one required a lot of work and a ton of debugging! Probably worth marking Major :)

Great job :)

Thanks everyone!
Marking major, adding credit.

Pushed!
Closing #2634276: Multiple routes break active trail and possibly other behaviors and #2782661: Page variant for node view causes internal error in other content types as they should be fixed now.

Should we backport this to core?

#2730497: REST Views override existing REST routes

PM

+++ b/src/Routing/PageManagerRoutes.php
@@ -139,7 +144,10 @@ protected function findPageRouteName(PageInterface $entity, RouteCollection $col
+      if (($path === $route_path || $path === $route_path_outline) &&
+        (!$collection_route->getMethods() || in_array('GET', $collection_route->getMethods())) &&
+        !$collection_route->hasRequirement('_format')) {

Views

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -237,8 +237,8 @@ public function alterRoutes(RouteCollection $collection) {
+      if (!$route->hasDefault('view_id') && ('/' . $view_path == $route_path) && (!$route->getMethods() || in_array('GET', $route->getMethods()))) {

I guess we could indeed add the ->hasRequirement('_format') part to Views.

What is the recommended process for patching the affected modules in the Acquia Lightning distribution? It appears as if the files are in a different structure? Is it safe to go through and update the a .. b .. paths in the patch to match the lightning folder structure?

@CabKab: Please open an issue in the Lightning issue queue, that's far too general a question for the Page Manager project.