Introduce a RouteMatch class

1. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,70 @@
   +
   +  public function getRequest() {
   +    return $this->request;
   +  }
   +
   

   i don't see the scope of the request object.

2. +++ b/core/modules/block/block.module
   @@ -107,7 +107,7 @@ function block_page_build(&$page) {
   +  if (\Drupal::routeMatch()->getRouteName() != 'block.admin_demo') {
   

   Just in general it seems wrong to have global state for the matched route. We should at least have an easy way to reconstruct it. if the request changes etc.

In Szeged @dawehner and me thought about extending the Symfony\Component\Routing\RequestContext and use it in the router.request_context service. This was in the context of decoupling UrlGenerator from the request service. Maybe this could help in this case also. Although a problem with that approach might be that RequestContext::fromRequest is called before routing. Thus the route related request attributes are not populated at this point in time, I guess.

Thus the route related request attributes are not populated at this point in time, I guess.

A possible solution to that problem could be to also extend Symfony\Component\HttpKernel\EventListener\RouterListener and update the route match service or whatever at the end of onKernelRequest.

Sorry for my last review, that one had a really bad quality.

So, the concept of a route match should include the matched route, the values of that route's parameters (i.e., arguments), and also, in my opinion, the request object, in case code that receives a $route_match object wants to consider other HTTP info like query parameters or POST data

In the case that someone actually wants request specific data why not just let him deal with the request object directly. I don't see why we
need an additional layer for that. I hope we won't disable to get the request if you want it.

I suppose alternatively, we could require use cases like AjaxBasePageNegotiator to supply route enhancers to map the stuff they care about into route arguments.

Mh, wouldn't that require everyone who wants to use anything from the request to write another route enhancer which will then be fired on every request and potentially in many more places. This seems to also be no real win, because it is getting harder to follow what is going on.

The current patch doesn't retain it as state. \Drupal::routeMatch() reconstructs it each time. However, once the instantiation is put into RouterListener, per #5, then the only way to change it in response to a $request change will be to invoke $router->matchRequest(), which I think would be a desirable restriction: a route match shouldn't change arbitrarily, it should be created as part of routing, and then be immutable.

Right, good point. For now living on \Drupal is probably okay, but why don't we actually move that into some routing bit? \Drupal\Core\Routing\UrlMatcher seems to be one potentially good fitting place. \Drupal just feels like a class which should be deprecated kind of by definition.

1. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,70 @@
   +    $this->request = $request ?: new Request();
   

   Do you think it would be also possible to have the request be real optional? a request object could point to something which actually does not exists in reality.

2. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,70 @@
   +
   +    // @todo Remove when
   +    //   \Symfony\Component\HttpKernel\EventListener\RouterListener is
   +    //   overridden to pass route arguments to the RouteMatch constructor
   +    //   directly rather than sticking them in $request->attributes:
   +    //   https://drupal.org/node/2124749.
   +    foreach ($this->request->attributes->all() as $key => $value) {
   +      if (!array_key_exists($key, $this->arguments) && substr($key, 0, 1) !== '_') {
   +        $this->arguments[$key] = $value;
   +      }
   +    }
   +    if ($this->request->attributes->has('_raw_variables')) {
   +      foreach ($this->request->attributes->get('_raw_variables')->all() as $key => $value) {
   +        if (!array_key_exists($key, $this->rawArguments) && substr($key, 0, 1) !== '_') {
   +          $this->rawArguments[$key] = $value;
   +        }
   +      }
   +    }
   

   Just in general: it would be better to not execute some crazy logic inside the constructor but yeah in this case it is not really a problem.

3. +++ b/core/lib/Drupal/Core/Theme/DefaultNegotiator.php
   diff --git a/core/lib/Drupal/Core/Theme/ThemeNegotiator.php b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   index 054d6be..41cf9ee 100644
   
   index 054d6be..41cf9ee 100644
   --- a/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   
   --- a/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   
   +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   @@ -7,8 +7,7 @@
   +use Drupal\Core\Routing\RouteMatch;
   

   I really hate that we now no longer depend on http foundation but on actual our own bit untop of CMF and component routing. Could we maybe at least move the route match into a component? There is nothing 100% specific about that, as it is a nice way to access these data.

4. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiator.php
   @@ -97,36 +88,21 @@ protected function getSortedNegotiators() {
   -  public function getActiveTheme() {
   

   oh yeah I sucked totally when writing that code. This should really be on the interface for example. On the other hand I think it is a good idea for now to not recalculate the active theme, everytime this is called.

1. +++ b/core/core.services.yml
   +++ b/core/core.services.yml
   @@ -161,7 +161,7 @@ services:
   
   @@ -161,7 +161,7 @@ services:
        class: Drupal\Core\Http\Client
      theme.negotiator:
        class: Drupal\Core\Theme\ThemeNegotiator
   -    arguments: ['@access_check.theme', '@request_stack']
   +    arguments: ['@access_check.theme']
   

   I was expecting to see a RouteMatchStack here. Every time a new request object is pushed into the request stack, KernelEvents::REQUEST fires. We should either be subscribing to that event and pushing a new routematch object into our stack, or we should be just requiring the request_stack as a dependency.

2. +++ b/core/includes/theme.inc
   @@ -102,8 +102,8 @@ function drupal_theme_initialize() {
   +  $route_match = \Drupal::routeMatch();
   
   +++ b/core/lib/Drupal.php
   @@ -188,6 +188,25 @@ public static function request() {
   +    return new \Drupal\Core\Routing\RouteMatch(
   +      $request->attributes->get(\Symfony\Cmf\Component\Routing\RouteObjectInterface::ROUTE_NAME),
   +      $request->attributes->get(\Symfony\Cmf\Component\Routing\RouteObjectInterface::ROUTE_OBJECT),
   +      array(),
   +      array(),
   +      $request
   +    );
   
   +++ b/core/lib/Drupal/Core/Cache/ThemeCacheContext.php
   @@ -53,7 +55,16 @@ public static function getLabel() {
      public function getContext() {
   ...
   +    $route_match = new RouteMatch(
   +      $this->request->attributes->get(RouteObjectInterface::ROUTE_NAME),
   

   Every time we call this we get a new route-match object, regardless of whether the request has changed or not. That surely leads to memory leaks.

3. +++ b/core/lib/Drupal.php
   @@ -188,6 +188,25 @@ public static function request() {
   +    $request = static::request();
   ...
   +      $request->attributes->get(\Symfony\Cmf\Component\Routing\RouteObjectInterface::ROUTE_NAME),
   

   Shouldn't this use the request stack instead?

4. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,70 @@
   +  protected $routeName;
   +  protected $route;
   +  protected $arguments;
   +  protected $rawArguments;
   +  protected $request;
   ...
   +  public function getRouteName() {
   ...
   +  public function getRouteObject() {
   ...
   +  public function getArgument($parameter_name) {
   ...
   +  public function getArguments() {
   ...
   +  public function getRawArgument($parameter_name) {
   ...
   +  public function getRawArguments() {
   ...
   +  public function getRequest() {
   

   Missing docblocks

5. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,70 @@
   +    $this->arguments = $arguments;
   +    $this->rawArguments = $raw_arguments;
   ...
   +        $this->arguments[$key] = $value;
   ...
   +          $this->rawArguments[$key] = $value;
   

   In my opinion this is worse DX. We are replacing a domain object (param bag) with an arbitrary array. Anyone who gets all the arguments back then needs to do the isset() dance. The Param Bag is far superior

6. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,70 @@
   +    return isset($this->arguments[$parameter_name]) ? $this->arguments[$parameter_name] : NULL;
   ...
   +    return isset($this->rawArguments[$parameter_name]) ? $this->rawArguments[$parameter_name] : NULL;
   

   This illustrates my point, this code wouldn't need the isset() dance if we kept the properties as param bags.

7. +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
   @@ -61,9 +60,9 @@ public function __construct(CsrfTokenGenerator $token_generator, ConfigFactoryIn
   -    return ($route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT))
   +    return ($route = $route_match->getRouteObject())
   
   +++ b/core/modules/block/block.module
   @@ -107,7 +107,7 @@ function block_page_build(&$page) {
   -  if (\Drupal::request()->attributes->get(RouteObjectInterface::ROUTE_NAME) != 'block.admin_demo') {
   +  if (\Drupal::routeMatch()->getRouteName() != 'block.admin_demo') {
   

   This looks much better

8. +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
   @@ -71,8 +70,8 @@ public function applies(Request $request) {
   -    if (($ajax_page_state = $request->request->get('ajax_page_state'))  && !empty($ajax_page_state['theme']) && !empty($ajax_page_state['theme_token'])) {
   +  public function determineActiveTheme(RouteMatch $route_match) {
   +    if (($ajax_page_state = $route_match->getRequest()->request->get('ajax_page_state'))  && !empty($ajax_page_state['theme']) && !empty($ajax_page_state['theme_token'])) {
   

   This is a step backwards. We are interested in a property of the request here, i.e. something formerly accessed with $_GET. I don't want to have to go via RouteMatch for that, because I'm genuinely interested in a property of the Request, not of the routing system.

9. +++ b/core/modules/forum/forum.module
   @@ -104,8 +104,7 @@ function forum_menu_local_tasks(&$data, $route_name) {
   +    $forum_term = \Drupal::routeMatch()->getArgument('taxonomy_term');
   
   +++ b/core/modules/node/node.module
   @@ -572,9 +572,9 @@ function node_revision_delete($revision_id) {
   +  $route_match = \Drupal::routeMatch();
   
   @@ -584,7 +584,7 @@ function node_is_page(NodeInterface $node) {
   +  if (($node = \Drupal::routeMatch()->getArgument('node')) && $node instanceof NodeInterface) {
   
   @@ -1072,16 +1072,14 @@ function node_block_access(Block $block, $operation, AccountInterface $account,
   +    $route_match = \Drupal::routeMatch();
   
   +++ b/core/modules/rdf/rdf.module
   @@ -351,7 +351,7 @@ function rdf_preprocess_user(&$variables) {
   +  if (\Drupal::routeMatch()->getRouteName() == $uri->getRouteName()) {
   
   +++ b/core/modules/system/tests/modules/menu_test/menu_test.module
   @@ -153,7 +153,7 @@ function menu_test_theme_page_callback($inherited = FALSE) {
   +  $active_theme = \Drupal::service('theme.negotiator')->determineActiveTheme(\Drupal::routeMatch());
   
   +++ b/core/modules/tour/tour.module
   @@ -82,8 +82,8 @@ function tour_preprocess_page(&$variables) {
   +  $route_match = \Drupal::routeMatch();
   

   Just so it is explicity clear every one of these calls creates a new RouteMatch object in memory. We can't do that. We have to have a RouteMatch stack service in or before this patch.

10. +++ b/core/modules/node/node.module
    @@ -1072,16 +1072,14 @@ function node_block_access(Block $block, $operation, AccountInterface $account,
    +    if ($route_match->getRouteName() == 'node.add') {
    +      $node_type = $route_match->getArgument('node_type');
    

    This looks good. Whilst the _something attributes are obvious overloading, the 'node' attributes one could argue are valid - but only in the context of routing - so moving them to RouteMatch makes sense.

11. +++ b/core/modules/shortcut/shortcut.module
    @@ -337,10 +337,9 @@ function shortcut_preprocess_page(&$variables) {
    +  if (\Drupal::routeMatch()->getRouteName()) {
    +    $item['href'] = \Drupal::request()->attributes->get('_system_path');
    

    This is a step backwards - I assume there will be methods on RouteMatch to get the system path in one of the associated issues?

+++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
@@ -0,0 +1,70 @@
+    $this->request = $request ?: new Request();

I think this will interfere with the work being done in #2016629: Refactor bootstrap to better utilize the kernel. We should stop pretending the request is optional. In that issue, we make it clear that a request is needed to create a DrupalKernel. It is required before we even decide what settings.php to use. I think the $request parameter has to be required here.

I was expecting to see a RouteMatchStack here.

I 100% agree. And that's a fundamental part I don't get about all these issues:

1. Symfony admitted that Request as a scope + service was a bad idea, and introduced RequestStack.
2. @msonnabaum, me, and many others actively discussed the concept of a ContextStack a dozen of times by now, whereas each Context in the stack would offer access to (1) the route, (2) the request, (3) the current user, (4) the current language, and (5) *whichever* additional context contrib comes up with (cf. Organic groups, Domain Access, etc.pp.).
3. But yet, we're trying to shoehorn the Request into some other one-off, routing-specific, state-ful objects, which are guaranteed to not resolve the overall problem at all...?

Can someone explain to me why we do not just simply do the following?

1. Copy/paste RequestStack into ContextStack.
2. Introduce a Context domain object that pretty much works like ParameterBag, but only operates with values that are objects implementing a ContextInterface.
3. Request is subclassed to implement that interface.
4. $context->get('request')->...
5. $context->get('route')->...
6. $context->get('user')->...
7. $context->get('language')->...

→ Clean context injection for the entire application stack.

→ Secondary: Fixing ESI, sub-requests, HttpKernelTestBase, etc.pp.

→ Secondary: Epic milestone for SCOTCH, layouts, and "Panels" in D8.

The RequestStack concept is proven to work and resolves a whole bunch of problems (for Drupal).

By now, my impression is that we have a communication break down somewhere. That is, unless the above was "blocked" by some insane purism of "Let's not subclass Symfony, contribute upstream instead", which would be hilarious, because (1) Symfony-based PHP frameworks may not share our requirements, (2) that's an epic effort, and (3) we're trying hard to release.

I do not want to get OT here, but please note that RequestStack is a special case and we do not need more *Stack classes in order to implement context properly.

There is consensus that the synchronized request service was a bad idea (and also container scopes which were introduced to make synchronized services actually work). And it certainly is true that introducing the request_stack will solve many problems. I'd like to stress that the actual win for services consuming the request stack instead of the synchronized request service is that they may store a reference at construction time and never need to update it subsequently. Whether this is a stack or not is not relevant at all (it is only relevant to the symfony HttpKernel).

Please note that there is already a pattern in Symfony demonstrating a solution on how to decouple context from request and we are using it already: This is what the Symfony\Component\Routing\RequestContext is all about. Services consuming the router.request_context service always can access up-to-date information about the request which is relevant in the service context without actually accessing the request itself (or the request-stack). Starting from Symfony 2.4, the router.request_context is updated from within the Symfony\Component\HttpKernel\EventListener\RouterListener::onKernelEvent handler.

Anyone please make sure to try understanding the context-concepts in Symfony and only after that consider introducing new ones. I very much believe that new *_stack services beside the request_stack are not necessary.

+++ b/core/core.services.yml
@@ -453,10 +457,10 @@ services:
   ajax_response_renderer:
     class: Drupal\Core\Ajax\AjaxResponseRenderer
   router_listener:
-    class: Symfony\Component\HttpKernel\EventListener\RouterListener
+    class: Drupal\Core\EventSubscriber\RouterListener
     tags:
       - { name: event_subscriber }
-    arguments: ['@router']
+    arguments: ['@router', '@route_match_stack']
   content_negotiation:
     class: Drupal\Core\ContentNegotiation
   view_subscriber:

+++ b/core/lib/Drupal/Core/EventSubscriber/RouterListener.php
@@ -0,0 +1,130 @@
+/**
+ * Performs routing.
+ */
+class RouterListener implements EventSubscriberInterface {
+

I'm opposed to swapping out the RouterListener completely. If we do that, chances are that inner workings of corresponding Drupal and Symfony components begin to diverge. See #2223189: Use regular upstream HttpKernel instead of Drupal's custom for an example. Also note that this would get in the way of #2223593: Decouple the router.request_context service from the request service..

The way router.request_context works in Symfony 2.4 is like follows:

• The router request context contains data required by both, the router and the url matcher.
• It is updated with data from the request from within router listener before a request is matched. Note that Drupal currently implements the old (Symfony < 2.4) approach, #2223593: Decouple the router.request_context service from the request service. will change that.

We could do exactly the same with the router matcher, except that the data is updated from within the on-requests-event handler after routing obviously. The symfony router listener does not care to remove the request context in a finish-request-event, therefore it is unclear to me why our router listener should pop a router matcher stack there.

We do not need any new stack-services.

#20 patch no longer applies - reroll - this patch is just Auto-merging fixes.

So not expect anything to run/pass.

Ok so now the error messages are up to date I will reroll...

Adds some tags.

Changes to comments

So the way forward is far from clear .. but writing docs to gain perspective..

On my local machine it is still possible to push this patch into error by browser testing of

Drupal\simpletest\Tests\OtherInstallationProfileTestsTest

#2223593: Decouple the router.request_context service from the request service. went in.

Untagging in favor of that one.

+++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
@@ -0,0 +1,65 @@
+  protected function getRouteMatch() {
+    $request = $this->requestStack->getCurrentRequest();
+    if (!$request->attributes->has('_route_match')) {
+      /** @var \Symfony\Component\Routing\Route $route_object */
+      $route_object = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);

Why storing the object itself on the route - is this for performance?

1. +++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
   @@ -0,0 +1,89 @@
   +   */
   +  public function getArgument($parameter_name) {
   +    return isset($this->getParameterNames()[$parameter_name]) ? $this->requestStack->getCurrentRequest()->attributes->get($parameter_name) : NULL;
   +  }
   ...
   +  public function getArguments() {
   +    return new ParameterBag(array_intersect_key($this->requestStack->getCurrentRequest()->attributes->all(), $this->getParameterNames()));
   

   I wonder whether getArgument should fallback to the raw argument in case there is no upcasted value?

2. +++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
   @@ -0,0 +1,89 @@
   +  protected function getParameterNames() {
   +    $names = array();
   +    if ($route = $this->getRouteObject()) {
   +      // Variables defined in path and host patterns are route parameters.
   

   Given that we will call getParametersNames for each get...Argument call I wonder whether we should a a little bit of caching here.

I like the look of this so far.

+++ b/core/modules/shortcut/shortcut.module
@@ -344,10 +344,8 @@ function shortcut_preprocess_page(&$variables) {
   // pages).
-  // Load the router item corresponding to the current page.
-  $request = \Drupal::request();
   $item = array();
-  if ($route = $request->attributes->get(RouteObjectInterface::ROUTE_NAME)) {
+  if (\Drupal::routeMatch()->getRouteObject()) {

Minor, but shouldn't be also be losing the use statement at the top of the file for these conversions?

In broad strokes I approve of this plan. Thanks, Alex! Some comments below:

1. +++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
   @@ -0,0 +1,89 @@
   +class CurrentRouteMatch extends RouteMatch {
   

   This class needs a docblock to explain why it's different than RouteMatch, because it's not at all obvious.

   That there's three *RouteMatch classes suggests it needs an interface.

2. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,55 @@
   +class RouteMatch {
   

   CurrentRouteMatch has a lot of @inheritdoc statements, but this parent has no docs to inherit.

3. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,55 @@
   +  protected $routeName;
   +  protected $route;
   +  protected $arguments;
   +  protected $rawArguments;
   

   Needs docblocks.

To the follow-up questions:

- I don't think we should inject RouteMatch into controllers automatically. Most of the data there that the typical use case for a controller would need is already mapped by ControllerResolver. The second most common request-specific thing a controller would want is GET parameters, but those aren't on RouteMatch anyway. (You need the Request for that.)

This wasn't tagged beta blocker however I think it should be, do we still need #2256669: [meta] Finalize module-facing API of getting the matched route name and a matched route argument for a request open though?

There's details to work out here but it doesn't look like we're going to do anything radically different requiring a different issue to figure out.

We left the tag on #2256669: [meta] Finalize module-facing API of getting the matched route name and a matched route argument for a request because we weren't sure which of the two proposals would be the correct solution and @effulgentsia wanted to give contributors a chance to evaluate both. So I'm okay with marking it fixed/duplicate/etc.

The feedback we gave on the recent Drupal 8 call was that the name RouteMatch was confusing for both Dries and myself to understand the object's purpose; any further thoughts around that?

RouteFinder? RouteLocator?

Rerolled for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

What about something like RouteRequestMatcher or RouteRequestInfo or something? Something that indicates that it gets routing information related to a given request. If a getter were returning this object, I'd call it something like getRequestRouteInfo(), I think.

The class name should not be a verb, because it's not an object that does anything. It is (from the POV of someone using it) a read-only data object with useful data. RouteMatch seems weird to me too but I won't lose sleep if we go with that. That ZF2 uses that name means it will be familiar to at least somebody. If Symfony didn't already use the name for something else I'd suggest "RequestContext".

I would prefer to keep Request a separate service/object. I could probably throw in something like the Law of Demeter here, but I'll just say "what effulgentsia said". :-) Input object and output object are different things doing different things.

Thank you @effulgentsia for working so hard on this. I very much like the new approach.

In my opinion the name very well describes the purpose of the class. In some programming languages the result of applying a regular expression to a string is an object providing easy access to substrings matched by the pattern (e.g. Match in Python). It seems very natural to me that the process of routing (i.e. applying a set of route patterns to an URL) results in a RouteMatch object. Routing is closely related to pattern matching. This is also very apparent when looking at the corresponding interfaces of the Symfony routing component.

Ok, so until someone comments with a compelling reason for adding getRequest() to RouteMatchInterface, I'm leaving it out.

I very much agree. I suspect that most of the theme negotiators do not really require the request-object at all, so maybe it would be possible to remove that from the ThemeNegotiatorInterface in a follow-up.

1. +++ b/core/core.services.yml
   @@ -265,6 +265,9 @@ services:
   +  route_match:
   +     class: Drupal\Core\Routing\CurrentRouteMatch
   +     arguments: ['@request_stack']
   

   How about current_route_match? Similar to current_user.

2. +++ b/core/includes/menu.inc
   @@ -481,7 +480,7 @@ function menu_local_tasks($level = 0) {
   -    $route_name = \Drupal::request()->attributes->get(RouteObjectInterface::ROUTE_NAME);
   +    $route_name = \Drupal::routeMatch()->getRouteName();
   

   Yeah! This is so much better, as well as all the other conversions in this patch.

3. +++ b/core/lib/Drupal.php
   @@ -188,6 +188,16 @@ public static function request() {
   +   * @return \Drupal\Core\Routing\RouteMatch
   
   +++ b/core/lib/Drupal/Core/Cache/ThemeCacheContext.php
   @@ -16,6 +17,13 @@
   +   * @var \Drupal\Core\Routing\RouteMatch
   

   Use interface instead of class name here.

4. +++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
   @@ -0,0 +1,55 @@
   +  public function getArgument($parameter_name) {
   ...
   +  public function getArguments() {
   ...
   +  public function getRawArgument($parameter_name) {
   ...
   +  public function getRawArguments() {
   

   I think we should use Parameter or ParameterValue instead of Argument here. In the Symfony documentation parameter is used in the context of routing and argument when discussing controller related things.

Rerolled with changes from #64.

Whoops. Wrong patch. Use this one instead. Same interdiff.

+++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
@@ -0,0 +1,89 @@
+  public function getArgument($parameter_name) {

+++ b/core/lib/Drupal/Core/Routing/RouteMatchInterface.php
@@ -0,0 +1,66 @@
+  public function getParameter($parameter_name);

CurrentRouteMatch still uses the old method names, RouteMatchInterface has the new ones.

1) I looked at #68 and saw the invalid service definition warning .... fixed

2) Applied Znerol suggested fixes.

limited local testing DRUPAL MATCH PATH is green

+++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
@@ -0,0 +1,55 @@
+class RouteMatch implements RouteMatchInterface {
...
+  protected $routeName;
+  protected $route;
+  protected $arguments;
+  protected $rawArguments;
...
+  public function __construct($route_name, Route $route, $arguments = array(), $raw_arguments = array()) {
...
+  public function getRouteName() {
...
+  public function getRouteObject() {
...
+  public function getParameter($parameter_name) {
...
+  public function getParameters() {
...
+  public function getRawParameter($parameter_name) {
...
+  public function getRawParameters() {

doc block missing.

Missing doc blocks supplied.

Did go through this again and noted the following things:

There are no PHPunit tests for CurrentRouteMatch. Not sure whether it would make sense to assert a couple of things, especially because CurrentRouteMatch::getParameterNames() seems to do a substantial amount of work.

+++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
@@ -0,0 +1,89 @@
+class CurrentRouteMatch implements RouteMatchInterface {

+++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
@@ -0,0 +1,108 @@
+class RouteMatch implements RouteMatchInterface {

+++ b/core/lib/Drupal/Core/Routing/RouteMatchInterface.php
@@ -0,0 +1,66 @@
+interface RouteMatchInterface {

No documentation for classes / interfaces. Out of those three the RouteMatchInterface is the most important to document accurately.

How about something like this:

/**
 * Provides an interface for classes representing the result of routing.
 *
 * Routing is the process of selecting the best matching candidate from a
 * collection of routes for an incoming request. The relevant properties of a
 * request include the path as well as a list of raw parameter values derived
 * from the URL. If an appropriate route is found, raw parameter values will be
 * upcasted automatically if possible.
 *
 * The route match object contains useful information about the selected route
 * as well as the raw and upcasted parameters derived from the incoming
 * request.
 */

+++ b/core/lib/Drupal/Core/Routing/RouteMatch.php
@@ -0,0 +1,108 @@
+  protected $arguments;
...
+  protected $rawArguments;
...
+   * @param array $arguments
+   *  The arguments array.
+   * @param array $raw_arguments
+   *   The raw $arguments array.
+   */
+  public function __construct($route_name, Route $route, $arguments = array(), $raw_arguments = array()) {
+    $this->routeName = $route_name;
+    $this->route = $route;
+    $this->arguments = new ParameterBag($arguments);
+    $this->rawArguments = new ParameterBag($raw_arguments);
+  }
...
+    return $this->arguments->get($parameter_name);
...
+    return clone($this->arguments);
...
+    return $this->rawArguments->get($parameter_name);
...
+    return clone($this->rawArguments);

Let's improve consistency and also rename the instance variables from $arguments to $parameters.

Referring to #75 :-

I agree CurrentRouteMatch::getParameterNames() needs testing ..

Can reviewers comment ...

This patch is set to alter some core functionality which will be set until then next API change .. in ahem Drupal 9.0 ( fingers crossed )
but I don't feel qualified to add the appropriate @api tags.

So here is my changes :-

1) RouteMatchInterface I have added the suggested text because it looks good to me ( minor tweek - upcasted has become upcast - because it reads better to me )

2) I have added placeholder comments for the other missing class and interfaces ... they feel a little bare, so please feel free to augment/correct.

3) $arguments thas become $parameters.

76: 2238217_76.patch queued for re-testing.

In the 20 minutes between #76 and #77 I get 6 commits !!!

This is in the middle of the sprint mayhem that is Druplicon Austin 2014.

But as then as NOW this patch applies and passes my minimal match path test I used in #72.

The test report in #76 is too vague and not what I am finding locally so I am retesting.

I was going to reroll this since the bootstrap patch broke it but I feel like there is a problem in the current approach. Going to try and grab some people at the con and see if I can get a better grasp on this and better enumerate my concerns on here.

I thought I'd re-roll just to see if we are still passing with the current implementation.

Summary of my concerns and the discussion.

I have some... academic concerns I will skip but the practical problem was that currently we may need to call a method in a Request event listener that wants a RouteMatch object as an argument because of how closely these classes are connected. The happy medium we came up with is to create a RouteMatch::createFromRequest() method and use that in event listeners and other things that may only have a Request available. Being as that will contain the logic for getting route information from request data, the CurrentRouteMatch would call into that method, probably with some for of static cache of RouteMatch data objects.

The route match is the result of the routing process (see #75). Introducing RouteMatch::createFromRequest() does not make much sense. A request event subscriber relying on the route match just needs to run after the router_listener subscriber.

This is correct, it does have to run after the route_listener because the information would not exist on the request. However, the need is real and came up in trying to merge this into the bootstrap where the theme initialization is moved into an event subscriber.

The fact is that we've made the this tight grouping of data between RouteMatch and Request and we are requiring the RouteMatchInterface for accessing it. Event listeners however only have access to request information directly through $e->getRequest(). The fact that we see this here, illustrates how we could run into this in Symfony libraries, external libraries, or possibly in our own internal libraries. As a matter of DX, we need to provide this ability if we're going to implement this concept.

the theme initialization is moved into an event subscriber.

The theme initialization needs to run after routing (see #2016629-29: Refactor bootstrap to better utilize the kernel). There is nothing wrong with injecting the current_route_match into the theme.negotiator.request_subscriber service.

Just resolving a recent conflict.

Cleaned up CurrentRouteMatch to be more readable. No functional changes.

Here's an interdiff from #88 to #89 (@msonnabaum's readability improvements).

+++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
@@ -26,44 +26,62 @@ public function __construct(RequestStack $request_stack) {
+  protected function getCurrentRequest() {
+    return $this->requestStack->getCurrentRequest();
   }

Needs a docblock. :)

Adding the docblock for the new protected method.

+++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
@@ -0,0 +1,116 @@
+    $raw_variables = $this->getCurrentRequest()->get('_raw_variables');
...
+    if ($raw_variables = $this->getCurrentRequest()->get('_raw_variables')) {

I think _raw_variables also lives on request-attributes. This is attempting to fetch them from the query string.

We badly need unit tests here.

Agreed.

Suggestion for the next iteration: CurrentRouteMatch::getCurrentRequestAttributes(), even less demeter for the rest of the class.

Working on merging the patch I made with my understanding of the agreement effulgentsia, crell, msonnabaum, and I had during the code sprint.

Why you got to be a jerk d.o? Patches aren't really deleted... but they got taken out of the list because of a submit conflict. Sorry.

Ok, patch. Diff 1 vs 81, Diff 2 pulls in readability fixes. SplObjectStorage is weird class but it seems like a good method for the internal cache of RouteMatch objects.

Woops. Better patch.

So still to do:

• Find/fix the bug. ;)
• Unit tests.
• Check whether _raw_variables
• We also lost getCurrentRequest() in the cleanups.
• CurrentRouteMatch::getCurrentRequestAttributes() (#93)

• +++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
  @@ -0,0 +1,90 @@
  +   * Load a routeMatch from a request variable.
  

  Case typo here.

• +++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
  @@ -0,0 +1,90 @@
  +   * @var \Symfony\Component\HttpFoundation\RequestStack
  ...
  +   * @var \SplObjectStorage
  

  These members need a one-line description. Also for anyone else who went "huh?" http://www.php.net//manual/en/class.splobjectstorage.php

1) Current bug is drush not having raw variables. @related 3) fixed.
2) Sure
4) Didn't seem to help much in this patch so left it off.
5) 6) fixed.

I'm off this for the evening. Problem currently seems to be that our error pages(403,404) build requests and then kicks off additional renders but those new requests don't get routing information. This breaks when we get to initializing the theme system which in this patch requires a RouteMatch. Seems we could either make a RouteMatch not require routing... or not really sure.

Thanks @neclimdul for your hard work.

We might be able to break this circular dependency by adding an optional $force_theme parameter to drupal_theme_initialize(). Anything which tries to render stuff before routing is then required to initialize the theme layer manually and supply an appropriate theme name. If $force_theme is set, the negotiation step will be skipped.

It is debatable whether it is legit in the first place to render HTML fragments before routing. ExceptionController::on403Html() does this at the moment if no custom 403 page has been configured. Also a lightweight Ajax callback churning out HTML fragments from within a request-event subscriber which runs as early as possible (i.e. before routing) seems a legit use-case. I think that it is justifiable to assign the responsibility of enforcing an appropriate theme in those cases.

Update: stack trace

There is another option if we agree that rendering of HTML fragments should be possible independently of whether a route was found and without actually having to initialize the theming layer manually beforehand. CurrentRouteMatch::getRouteMatch() would have to detect whether a given request already has been routed. If this is not the case, it could return an EmptyRouteMatch (or NoRouteMatch) which just implements stub methods. This actually would reproduces the current behavior.

Instances of EmptyRouteMatch should not be recorded in the $routeMatches map of CurrentRouteMatch. First there is no need for caching an empty implementation, second this allows for code which accesses current route match before or after routing to always get an accurate result.

This might look something like:

+++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
+  protected function getRouteMatch(Request $request) {
+    if (!isset($this->routeMatches[$request])) {
+      if (!$this->isRouted($request)) {
+        return new EmptyRouteMatch();
+      }
+      $this->routeMatches[$request] = RouteMatch::createFromRequest($request);
+    }
+    return $this->routeMatches[$request];
+  }

The isRouted() method would e.g. check for the presence of _raw_variables and/or route name/object.

Note that we also have a third option: go back to #92 and leave out the caching and the coupling from CurrentRouteMatch to RouteMatch for the moment.

What about adding routes for the 403/404 pages? If there's a good reason against that then fine, but those pages are already very strange and the general approach to how they get rendered hasn't changed since i can remember.

Yeah @catch, that seems like the ideal solution but I'm concerned there's a use case hidden in there that would drag this issue into oblivion. Also, from my quick skim, those methods look really complicated which makes things worse.

+++ b/core/lib/Drupal/Core/Routing/CurrentRouteMatch.php
@@ -79,16 +80,34 @@
+      if (!$this->isRouted($request)) {
+        return new NullRouteMatch();
+      }

I like this. I didn't notice at first but since this gets called in the cache miss and returns without settings the cache, we can recover if someone misbehaves and calls this on something route-able before routing happens. This whole block should be very cheap and infrequently called so it should work really well.

If we have consensus at this point, I'm OK with moving this to RTBC.

1) maybe... not sure.
2) cleaned
3) PHPStorm mangling things...
4) I generally try only do assignment during construction but this might be a good place to make an exception.
5) 6) gugh, I was diff-ing off a branch. looks like I messed up and was reverted some stuff. Cleaned up.

copy pasting around... should pay attention.

Nice! Going to take a stab at splitting out the coverage.

@neclimdul wasn't in the list of maintainers on d.o, but is in MAINTAINERS.txt for Queue system, so I added him :)

From the issue summary:

This patch updates all theme negotiators to demonstrate the pattern of $route_match as an injected object.

So, yes. Completing the theme-negotiation API in this issue is in-scope in my opinion.

Note that this will result in a strange implementation of the theme.negotiator.request_subscriber service to be introduced in #2016629: Refactor bootstrap to better utilize the kernel. The event handler will operate on the constructor-injected current_route_match while completely ignoring the passed in GetResponseEvent. We could resolve this in a later step by introducing a new event type similar to KernelEvents::REQUEST having route match on the event. I imagine than a new event capable of propagating route match also would lower the risk that the request event gets overcrowded by contrib subscribers.

Ok got all the fires that kept me from digging into the tests put out.

This breaks out tests into units that can get @covers. @covers all the methods being covered. Uses a base class to cover all the methods in CurrentRouteMatch ever if the current implementation is just proxy-ing, and adds a couple of trivial assertions like checking getParamater() in the CurrentRouteMatch test.

It has 100% coverage of the new classes but you'll have to apply #2287385: Fix PHPUnit coverage tests to see that.

Rerolled for #2278541: Refactor block visibility to use condition plugins, no additional changes.

Thanks for resolving that tim!

Yeah, those methods where from a early refactor and didn't get removed.

Whoops, we can't rely on a route object coming back.

Actually, looking at the interdiff in #129, and at NullRouteMatch, I think we either need to make NullRouteMatch::getRouteObject() return an empty Route object (not sure if that's possible or even desired), or adjust RouteMatchInterface::getRouteObject() to mention that it can return NULL.

Great patch, great progress. I also like the fact that this patch additionally removes the _theme_active request attribute.

1. +++ b/core/lib/Drupal/Core/Theme/AjaxBasePageNegotiator.php
   @@ -71,8 +81,8 @@ public function applies(Request $request) {
   +    if (($ajax_page_state = $this->requestStack->getCurrentRequest()->request->get('ajax_page_state'))  && !empty($ajax_page_state['theme']) && !empty($ajax_page_state['theme_token'])) {
   

   Tiny nitpick and out of scope here. I hate reading code with assignments as conditions. Especially if there is more than one condition.

   In this special case it would be possible to simply move the assignment before the if, that would help readability a lot.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
   @@ -30,24 +30,24 @@
   +   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   +   *   The current route match object.
   ...
   +   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   +   *   The active route match of the site.
   

   Also nitpick, and could be a quick-fix follow-up: Better align the docs here. The active route match of the site is nonsense (just like before the active request of the site was nonsense.)

This is ready in my opinion.

Also minor

+/**
+ * ...
+ */

I fixed the two minor docs issues on commit, the assignment I don't feel strongly about and it's already there.

Committed/pushed to 8.x, thanks!

Two things.

1. RouteMatch doesn't have a dedicated change notice, that's a rather important addition.
2. The removal of ThemeNegotiator::getActiveTheme() wasn't mentioned anywhere, but that was the main entry point for finding out the current theme.

Alternatively, instead of documenting the change to ThemeNegotiator, let's just fix it: #2292217: ThemeNegotiator::determineActiveTheme() should not require a RouteMatch to be passed in

We also need to update existing change records, e.g.:
https://www.drupal.org/node/2192317
https://www.drupal.org/node/2274705
https://www.drupal.org/node/2287827
https://www.drupal.org/node/2203305
And possibly others here:
https://www.drupal.org/list-changes/published/drupal?keywords_descriptio...

Also, should we close #2103301: Add a helper class to introspect a given request as a duplicate now? (And change the CR reference on that issue to reference this one instead.)

Closed that one as duplicate and updated the references.

I had it in my head that there was already a change notice for this but didn't actually check, I think I must have been thinking of the API examples from the comparison issue.

@chx posted #2294777: The name RouteMatchInterface is confusing. Still need those change record updates and the new change record though. :) If we change the name later (and I'm not sure we will) then it's just a couple of string-replaces.

Going to work on this

Hmm, after updating all of the other change notices, I'm not sure what else we can say here.

https://www.drupal.org/node/2295317

I tweaked the CR a little and then published, just polish: https://www.drupal.org/node/2295317/revisions/view/7395605/7396703

Few more things (I haven't followed up on any of this yet):

1. https://www.drupal.org/node/2158619 and/or https://www.drupal.org/node/2139569 need an update (for this and the other issue). Also we should pick one and get rid of the other. ;)
2. https://www.drupal.org/node/2067859 and https://www.drupal.org/node/2023537 need updates I think.
3. Is https://www.drupal.org/node/2020005 covered by a separate issue somewhere? (Not related to this issue.)
4. https://www.drupal.org/node/1539454 should not use that example. :) (That's not related to this issue; just noting here so I don't forget.)
5. We should probably add some sort of note to the bottom of https://www.drupal.org/node/2083979 referencing the RouteMatch CR (also for _system_path).
6. I think we should add an explicit D7-to-D8 example to https://www.drupal.org/node/2203305 for menu_get_item() (this was a deficiency in the original CR but got worse when it was changed to mention pending change records).
7. I've been thinking all weekend that we should have some more sample code documentation for how to use RouteMatch, like a route declaration and then how you would use the methods to get a bit of it, and what it would return. (Especially useful for differentiating parameters from raw parameters.) Rather than add more code snippets that might go stale to the CR, I think a followup docs issue is in order. (I added a link to the class itself in the CR for this reason.)

Tried to address #147.2, please double-check:
https://www.drupal.org/node/2023537/revisions/view/6714611/7396741
https://www.drupal.org/node/2067859/revisions/view/7368167/7396747

Started looking at #147.6 but every usage of menu_get_item() I looked at in D7 core and contrib was part of something completely different enough in D8 that the entire code flow has changed (e.g. using drupal_goto(), futzing with titles, altering local tasks, etc.) So maybe we should reference the relevant CRs for those things and just give a relevant D7 menu_get_item()/D8 RouteMatch example in the CR. Started with this for now:
https://www.drupal.org/node/2203305/revisions/view/7380117/7396859

• In the light of morning #147.5 seems fine.
• For #147.4 I've left a comment on https://www.drupal.org/node/2239009 as I'm not sure what to do with those.
• I addressed #147.1 in https://www.drupal.org/node/2158619/revisions/view/6783809/7398343 and by deleting the duplicate (also updated it for the other issue).
• The issue for #147.3 is #2288665: Remove _maintenance request attribute and replace it with a maintenance mode service so I've updated it to reference that.
• I'll file an issue for #147.7 separately.

So all done. :) Please double-check these and make any corrections/clarifications/improvements needed:

• https://www.drupal.org/node/2203305/revisions/view/7380117/7396859
• https://www.drupal.org/node/2158619/revisions/view/6783809/7398343
• https://www.drupal.org/node/2023537/revisions/view/6714611/7396741
• https://www.drupal.org/node/2067859/revisions/view/7368167/7396747