Create a route for every variant and use a "route filter" to apply selection rules

Use case four, auto building documentation for a rest api

I don't know how we turn this issue into an actionable task, but I agree with the premise of it. The one caveat I'd add is that, in the Symfony model, the result of routing is not "the resource being displayed" but "the controller/action responsible". The resource to be displayed is, frequently, a route parameter at that point. That is, Symfony routing is not a full on WebMachine model but we can sort of use it as one, and we should.

AFAICT, this was fixed in #2352155: Remove HtmlFragment/HtmlPage, where a new RenderEvents::SELECT_PAGE_DISPLAY_VARIANT was introduced.

However, this issue feels a bit more broad than that, so if this is a premature closing, feel free to reopen.

Personally, I think we should keep this open for now. This was more a "warning" about what we shouldn't do when moving functionality from page_manager into Drupal core. And since the book still isn't closed on that (even if it's 8.1.x or 8.2.x, etc), I think we need to still keep this at the back of our minds.

A quick question for @Wim: Looking at the diagram, it appears as though the main content is already selected and built before page variant selection, right?

Anyway, when orginally discussing this issue, we decided that the standards for page_manager in contrib and page_manager in core are different. In contrib, a multi-stage router is not ideal, but acceptable. But it's something we shouldn't introduce to core!

At MWDS, @Crell explained to me that some of the new Drupal 8 routing system features were designed specifically to allow Page Manager to be implemented without introducing a multi-stage router.

Moving this issue to Page Manager (because core can already handle this) and updating the issue summary with a concrete plan!

This might be related to #2549997: Check for replacing existing routes should not remove default arguments

use a "route filter" to apply selection rules
-> this screams 'route' cache context. i.e. please ensure that anything Page Manager caches that happens after (and depends on) this filtering/selection logic, has the 'route' cache context in its cacheability metadata. Definitely any response generated by Page Manager that depends on this filtering/selection logic should have that cache context.

(This is just an FYI for when you reach that point. See https://www.drupal.org/developing/api/8/cache/contexts and related handbook pages for more information.)

Thanks, Wim! :-)

TL;DR
#2551633: Make variants into their own config entity should be blocked on this
#2561339: Consider referring to "routes" rather than "pages" in the UI? could be a direct result on this
This issue will diverge us from D7 architecture and really change the way we think of Page Manager

----

After many lengthy discussions yesterday about the merits of #2551633: Make variants into their own config entity, I think it is important to pursue this issue first.
One major thing we need to think about here is whether we still need the distinct concepts of pages vs variants.
At what point do they just become the same thing?

I'd like to have a call about this, or make it the major focus of our next team call, we need to make a decision on this soon.

One major thing we need to think about here is whether we still need the distinct concepts of pages vs variants.

So, conceptually, if multiple pages exist for the same route, but with different selection criteria, that then just means we go from "selecting a variant of a page to show" to "selecting a page to show".

As a total noob in this area, that sounds much simpler, and probably also easier to explain to site builders, and hopefully can also result in a slightly simpler UI.

One major thing we need to think about here is whether we still need the distinct concepts of pages vs variants.
At what point do they just become the same thing?

That is a really interesting idea!

I personally can't manage a call this weekend (staying at my in-laws) but we should for sure discuss this at the next SCOTCH meeting.

I'd be happy to be part of this conversation, but I'm still in Bulgaria for the next week and Milwaukee next week. When is the next SCOTCH meeting?

I land in philly about halfway through the meeting, not going to make that

However, working on a patch for this

Ok, let's discuss at the SCOTCH meeting (Tuesday @ 12pm CDT) with whoever is there, and then try to put together another meeting where we can get Tim (for sure) and Crell (if it works out). Here's a Doodle:

http://doodle.com/poll/qsu4cet8g7nd63y9

If folks are available, we could even do later in the day on Tuesday?

Hopefully, it doesn't have to be next week because it'd be great to have a decision on this as it affects work on lots of other things...

Just a WIP. Unit tests will fail, so not testing.

We discussed this issue on a call today, and someone (Kris?) asked the question about how you create a menu item that refers to a Page if the page is made up of multiple routes with unique route names since the menu item takes a route name. We should attempt to first make sure this is possible before merging this patch, and if it is possible, we should create some automated tests to make sure it works.

More progress.

Here, at @EclipseGc's request, is a test module proving that route filtering will work for this -- https://github.com/phenaproxima/route-filter-test

Ok, so my specific worries here are totally allayed by working with Phenaproxima's code. ++ to this issue.

Eclipse

Just a small tweak to allow menu links to have somewhere consistent to point to.

VariantRouteFilter has this code:
$contexts = $page->getExecutable()->getContexts();

That dispatches an event that triggers RouteParamContext, which tries to pull the node value from the request attributes.

Just one problem.

Request attributes are not populated until AFTER route filters finish running.

See \Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher::matchRequest().
$attributes = $this->finalMatcher->finalMatch($collection, $request); is what populates $request->attributes, and that is directly after route filtering.

No idea what to do now.

Let's implement our own "routing"!

We gather the contexts from the event and call setContexts() on the display_variant during selection.

In HEAD, that happens within the controller, and by the time our #pre_render runs it is still the same instance of the display_variant.

With this patch, selection runs during routing, and by the time our #pre_render runs it is a DIFFERENT instance of the display variant, so it has ZERO contexts.

---

So, here's a hack!

Updating a bunch of docs.

Wrote some tests, which revealed some improvements to be made.

Wrong interdiff.

Consolidated the get/setContext calls.
Also, ensured that we always load the correct page entity.

The only remaining @todos are in \Drupal\page_manager\Routing\VariantRouteFilter::prepareRequest(), will hopefully track down Crell.

Replaced the param converter with running all of the route enhancers.
Next up is the last @todo of preventing request munging.

Ughhh wrong interdiff again.

Fixing tests, duh.

One less @todo, one remaining!

Found a major bug.

If you provide a page with one variant for /node/{node}, it will run on /node/add, despite that route having no page/variant info on it.
The route collection would have 2 routes in it, the correct route for /node/add, and the variant for /node/{node}. The correct route would be removed.
If the /node/{node} variant has a selection condition depending on {node}, it will throw a ContextException.
If it doesn't have any problematic selection conditions, it will be selected, and will 404.

With this patch, when we encounter a route with NO page/variant info, we don't remove it, we move it to the end of the collection.
Additionally, we catch all ContextExceptions and remove those routes from the collection.

This gets us 2/3 of the way there. The final problem is when you have a route with no problematic selection conditions, as stated above...

+++ b/src/Routing/VariantRouteFilter.php
@@ -0,0 +1,142 @@
+    if ($collection->count()) {
+      $this->prepareRequest($collection, $request);
+    }
+

In the other case you could totally do an early eturn so we maybe could make prepareRequest the non conditional test

Great idea!
Still figuring out the fail.

This prevents custom page_manager access from running on overridden routes. We should update the UI to hide that section in that case.

It also does not remove non-page_manager routes from the collection, it just moves the to the end. This helps with the /node/add problem.

Our blocker for that is #2600666: Route definitions using entity type paramconverters with serial IDs should add an integer requirement, which fixes /node/add completely.

Working around 2600666, and adjusting the tests for the access change mentioned in #43.

Removed the call to finalMatch thanks to some code from @dawehner. \Drupal\page_manager\Routing\RouteAttributes should probably move to CTools (or really to Symfony CMF).

Once I got it all passing, I was able to refactor filter() to not be a rat's nest of nested conditions. Still not ideal using the $page_manager_route_found flag, but it works.

I think this is ready for commit.

Committed after in-person review from @EclipseGc.
Thanks to @EclipseGc, @dsnopek, @dawehner, and @sdboyer for help with this!

#21+#22: So @phenaproxima's module proves route filtering works. But this is not called at runtime. So… how would this work? Wow, apparently route filters are executed at runtime. I find that quite frightening performance-wise. But D8 core itself uses it too, so… I guess it's okay.

#27: "own routing" -> even more frightening! Especially if this means routing (and param converting) happens twice… Does it happen twice? If not, great, but let's prove that using test coverage? #37 seems to confirm it does happen twice…

I think it's crucial to look at the performance for a route X:

• without Page Manager
• with Page Manager not overriding route X
• with Page Manager overriding route X and having a single variant
• with Page Manager overriding route X and having multiple variants

@Wim: There's some things that will run twice because of this change, due to limitations in some core and/or Symfony CMF code. It was decided at the BADCamp sprint that this was acceptable for now, and that we'd open some issues afterwards to fix it. Can we do general performance profiling on a new issue?

@tim.plunkett: Since you're the most familiar with this code, could you create the necessary follow-ups to prevent doing stuff twice? Thanks!

Can we do general performance profiling on a new issue?

Of course.

But I think it's critical to know what the performance impact is. Are we talking say ~1 ms or ~10 ms? Magnitude of impact is what matters most here.

I opened #2605282: Ensure that upcasting only runs one extra time. to help us mitigate the impact of this change.

Also opened #2605286: Profile the recent changes to Page Manager routing to do some investigation.