[meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API

"here's some of them." lol. holy shitballs, i didn't realize it was this bad.

+1 would buy again

Some of those are legitimate. More than 80% of the attributes with no leading _ are actually referring to attributes of the request, like

core/modules/shortcut/lib/Drupal/shortcut/Access/LinkAccessCheck.php:    $menu_link = $request->attributes->get('menu_link');

But yes, +1 to the issue as a whole.

What does "custom" mean?

As Tim said, most are actual request attributes from the parameters/defaults. Those belong there.

There's the "extended defaults", which we're modeling on Symfony: _controller, _content, etc. There isn't really an alternative for those either.

Others are information about the request itself; _system_path, for instance, is not some random service sticking stuff into a global array out of convenience; it's part of the request/routing system. That seems entirely sensible to me to be part of the request, and a whole other service just to hold that seems bad for cohesion.

What properties in particular are there now that you think shouldn't be? Let's itemize them and triage, not say "get rid of 'em!" There probably are some that shouldn't be there, but make the case case-by-case.

Here are some different greps.

grep -hor --exclude-dir core/vendor "request->attributes->...('_[^']*'" core | sort | uniq -c | sort -nr
  18 request->attributes->get('_system_path'
  11 request->attributes->set('_account'
   7 request->attributes->set('_system_path'
   7 request->attributes->get('_entity_type'
   5 request->attributes->set('_raw_variables'
   5 request->attributes->get('_raw_variables'
   4 request->attributes->set('_maintenance'
   3 request->attributes->get('_maintenance'
   3 request->attributes->get('_controller'
   2 request->attributes->get('_legacy'
   2 request->attributes->get('_account'
   1 request->attributes->set('_controller'
   1 request->attributes->set('_authentication_provider'
   1 request->attributes->has('_system_path'
   1 request->attributes->has('_raw_variables'
   1 request->attributes->has('_legacy'
   1 request->attributes->has('_controller'
   1 request->attributes->get('_authentication_provider'
   
   
$ grep -hor --exclude-dir core/vendor "request->attributes->...('_[^']*'" core | grep -o "'.*'" | sort | uniq -c | sort -nr
  26 '_system_path'
  13 '_account'
  11 '_raw_variables'
   7 '_maintenance'
   7 '_entity_type'
   5 '_controller'
   3 '_legacy'
   2 '_authentication_provider'

_account is deprecated already.
_entity_type is used as an internal route parameter for content_translation.

_system_path and _raw_variables might deserve special attention. Especially _raw_variables, since you *always* have to check has() first before accessing it...

The rest aren't used enough to worry about, except that they form the guidelines for how contrib should behave.

_maintenance could, arguably, be turned into a chain-of-responsibility service (akin to breadcrumbs and various other things) instead of a request parameter. At this point that would probably be "more correct", but I don't know what the performance impact of that would be.

_legacy was only to flag a route as being handled by the legacy router. Now that it's gone we probably should remove any remaining traces of it.

_raw_variables is... interesting. I'm actually curious what Symfony fullstack's generator does with that problem space. (It was added largely for the generator.)

_authentication_provider is used by the authentication system because it has to track data from authentication (which is pre-routing) to route access (post-routing), due to the way we configure what routes support what authentication mechanisms. I'm open to suggestions on how else to do it, but given that we have to do it in 2 parts I don't think we have a better alternative than using the Request for that sort of state.

_system_path: I try to figure out why we cannot use $request->getPathInfo() given that
the documentation pretty much does what we want:

* * http://localhost/mysite returns an empty string
* * http://localhost/mysite/about returns '/about'
* * http://localhost/mysite/enco%20ded returns '/enco%20ded'
* * http://localhost/mysite/about?var=1 returns '/about'

_raw_variables is... interesting. I'm actually curious what Symfony fullstack's generator does with that problem space. (It was added largely for the generator.)

Either the original value is replaced or it is stored as different variable. The code shows a upcasted variable in a new name.

* @ParamConverter("zipCode", options={"name": "PostalCode"})

For various usecases we thought really need the not upcasted values in an accessible way.

getPathInfo() is the unfiltered data from the HTTP get request. _system_path is after doing path alias lookups, i18n prefixes, and so forth.

If we're going to move anything else, they should be done before beta1 IMO. Should we bump this to beta blocker?

There's the "extended defaults", which we're modeling on Symfony: _controller, _content, etc. There isn't really an alternative for those either.

Others are information about the request itself; _system_path, for instance, is not some random service sticking stuff into a global array out of convenience; it's part of the request/routing system. That seems entirely sensible to me to be part of the request, and a whole other service just to hold that seems bad for cohesion.

If it's ok to tack _controller, _route, _system_path, etc on Request's attributes, why not just subclass Request and turn them into proper methods? Please think about this from a design perspective, ignoring any perceived compatability issues that arise with subclassing whether they actually exist or not.

Does it seem ok to have Request::getRoute, Request::getController? If it doesn't, I don't see how we could possibly justify throwing them into Request's attributes, regardless of whether or not anyone else is doing it.

It's unstructured data with no interface to enforce it. It's just as bad as anything we've done with magic # methods in form api and render api.

These would probably need to be dealt with individually as this thead is already doing, but I do think we should be aiming to remove all _ attributes from Request.

Request::getPathInfo() is already there, and is the path of the request. ::getPathAfterDealiasing() seems entirely reasonable there as well, which is what _system_path is.

The request object is not an immutable value object. I agree it would be nice if it were, but that's not the architecture.

The request object is not an immutable value object. I agree it would be nice if it were, but that's not the architecture.

That's kind of a non-answer.

_route is a better example. Is it reasonable to have Request::getRoute?

re: #9

Please think about this from a design perspective, ignoring any perceived compatability issues that arise with subclassing whether they actually exist or not.

ok, so first from the abstract perspective - agreed.

the moment at which a datum on the request object becomes important enough that we start thinking about documenting the "magic key" at which it resides is the same moment at which it merits a method. if it has a central conceptual role, then docs on a getter method is the difference between an API and an "array API". so really, i'm just echoing @msonnabaum. again.

practically speaking, though, with Requests being the mutable-state monster that they are, and with runtime mixins not being possible, i don't think it's especially feasible to apply methods to more than a small subset of these properties - the ones in which, in any context that core can reasonably foresee, the underlying properties should be present. the ones that our routing system unambiguously provides are clearly good candidates.

While working on #2177461: Refactor page caching into a service, I was looking for a way to add a simple cache-hit-miss flag onto the request object. Attributes is certainly the most obvious candidate for this sort of thing. However I also share that concern that attributes will become a maintenance nightmare. At the latest when contrib authors also start to dump their stuff there.

However I think that there are legitimate use cases for passing around fragments of data associated with a request. It might be possible to support that by applying a wrapper pattern.

$menu_attributes = new MenuRequestAttributes($request);
$menu_link = $menu_attributes->getMenuLink();

This way each core-subsystem / contrib-module can maintain and document its own dictionary of request attributes in a controlled way.

So, all of us agree this is bad.

#14 makes a proposal, but I don't see how MenuRequestAttributes would work throughout all of core.

Do we have any actionable consensus?

I think it is important to find ways around accessing request attributes directly. It is alarming that an API found its way into core which is built around the request attributes-array (see: BreadcrumbBuilderInterface, BreadcrumbManager, hook_system_breadcrumb_alter). This will blow up sooner or later.

From #2177031: Remove menu_get_item() and every use of it. .

We need to update https://drupal.org/node/2203305 once we have a solution here with before-and-after, at-a-glance, more-hyphens-please code for how to convert menu_get_item() and menu_get_object(). I wonder if those two deserve their own dedicated issue since this is very broad?

Let's be more concret: #2103301: Add a helper class to introspect a given request

Related issue #2103301: Add a helper class to introspect a given request could use architectural review.

New greps, along the lines of #4, but changed to include non-underscores and constants. I'm cutting this inline list to only ones that have 5 or more usages. The attached file contains the full list.

$ grep -hor --exclude-dir core/vendor "\->attributes->...(['A-Z][^,)]*" core | sort | uniq -c | sort -nr
  22 ->attributes->get('_system_path'
  18 ->attributes->get(RouteObjectInterface::ROUTE_NAME
  13 ->attributes->get(RouteObjectInterface::ROUTE_OBJECT
  10 ->attributes->set('entity_type'
  10 ->attributes->set('entity'
  10 ->attributes->get('node'
  10 ->attributes->get('_raw_variables'
   9 ->attributes->set('_system_path'
   7 ->attributes->set('langcode'
   6 ->attributes->get('_theme_active'
   5 ->attributes->set('view_id'
   5 ->attributes->set('display_id'
   5 ->attributes->get('field_name'
   5 ->attributes->get('_entity_type_id'
   ... see attachment for rest

$ grep -hor --exclude-dir core/vendor "\->attributes->...(['A-Z][^,)]*" core |grep -o "(.*" | sort | uniq -c | sort -nr
  33 ('_system_path'
  18 (RouteObjectInterface::ROUTE_NAME
  15 ('_raw_variables'
  14 (RouteObjectInterface::ROUTE_OBJECT
  14 ('node'
  14 ('entity'
  12 ('entity_type'
  11 ('langcode'
   9 ('field_name'
   8 ('_theme_active'
   7 ('view_id'
   7 ('display_id'
   7 ('_maintenance'
   6 ('commented_entity_type'
   5 ('_menu_admin'
   5 ('_entity_type_id'
   ... see attachment for rest

$menu_link = $request->attributes->get('menu_link');

This might be legitimate, but it's not nice. How do I know that there is an attribute called 'menu_link' and how to access it?

Well, for {menu_link} specifically, there's only 1 place in HEAD that has that line of code: Drupal\shortcut\Access\LinkAccessCheck, but there's no core route that uses that access checker. Therefore, should we delete that class?

More generally, there are access checkers that actually are used, and have similar code for other route parameters:

$ grep -hor --exclude-dir core/vendor --include="*Access*" "\->attributes->get(['A-Z][^,)]*" core | sort | uniq -c | sort -nr
   4 ->attributes->get('entity'
   2 ->attributes->get('user'
   2 ->attributes->get('node'
   2 ->attributes->get('langcode'
   2 ->attributes->get('field_name'
   2 ->attributes->get('entity_type'
   2 ->attributes->get('bundle'
   2 ->attributes->get('_raw_variables'
   2 ->attributes->get('_menu_admin'
   2 ->attributes->get('_entity_type_id'
   1 ->attributes->get(RouteObjectInterface::ROUTE_OBJECT
   1 ->attributes->get('view_mode_name'
   1 ->attributes->get('theme'
   1 ->attributes->get('target'
   1 ->attributes->get('source'
   1 ->attributes->get('shortcut_set'
   1 ->attributes->get('node_type'
   1 ->attributes->get('node_revision'
   1 ->attributes->get('menu_link'
   1 ->attributes->get('language'
   1 ->attributes->get('key'
   1 ->attributes->get('form_mode_name'
   1 ->attributes->get('filter_format'
   1 ->attributes->get('field_instance_config'
   1 ->attributes->get('account'
   1 ->attributes->get('_system_path'
   1 ->attributes->get('_controller_request'
   1 ->attributes->get('_authentication_provider'

I think in a lot of these cases, the problem is that we are using an access check service for a specific route, because the CustomAccessCheck service was added somewhat recently. If we convert these route-specific checkers to custom access callbacks, then they can take the attributes they want as callback parameters, just like controllers do. Additionally, if we're left with checkers that should stay as generic services, perhaps we can add a base class or trait that allows generic access checkers to also benefit from ControllerResolver the same way custom access callbacks can?

If we then probe what remains (exclude Access, exclude Test):

$ grep -hor --exclude-dir core/vendor --exclude="*Test*" --exclude="*Access*" "\->attributes->get(['A-Z][^,)]*" core | sort | uniq -c | sort -nr
  19 ->attributes->get('_system_path'
  18 ->attributes->get(RouteObjectInterface::ROUTE_NAME
  12 ->attributes->get(RouteObjectInterface::ROUTE_OBJECT
   8 ->attributes->get('node'
   8 ->attributes->get('_raw_variables'
   3 ->attributes->get('field_name'
   3 ->attributes->get('commented_entity_type'
   3 ->attributes->get('_maintenance'
   3 ->attributes->get('_entity_type_id'
   2 ->attributes->get('theme'
   2 ->attributes->get('taxonomy_term'
   2 ->attributes->get('display_id'
   2 ->attributes->get('_theme_active'
   2 ->attributes->get('_controller'
   1 ->attributes->get('view_mode_name'
   1 ->attributes->get('view_id'
   1 ->attributes->get('user'
   1 ->attributes->get('node_type'
   1 ->attributes->get('langcode'
   1 ->attributes->get('form_mode_name'
   1 ->attributes->get('clean_urls'
   1 ->attributes->get('_view_argument_map'
   1 ->attributes->get('_legacy'
   1 ->attributes->get('_http_statuscode'
   1 ->attributes->get('_form_errors'
   1 ->attributes->get('_authentication_provider'

I don't think there's that much usage to require DX sugar on top of the legitimate usage of getting values for the named parameters of routes. I mean, maybe it would be nice if Symfony added a getAttribute() method to Request, allowing us to remove an arrow, and if someone wants to submit a PR for them to do that, please do, but I don't think it's a big deal, given how relatively rarely core needs to do this outside of access checkers.

'node' sticks out as an exception in part because of some procedural code in node.module (and a line in theme.inc that should move to node.module), and in part because of Views argument plugins. The former can be addressed with a wrapper function in node.module (e.g., _node_from_request()), and the latter could be addressed with a protected helper method in ArgumentDefaultPluginBase (since an argument plugin needing request arguments seems like a potentially common thing).

The above is all just a response to catch's DX concerns in #13/#15.

Now, as far as the issue title, and internal attributes that have nothing to do with route parameters:
1) Is it legit to add arbitrary internal attributes to $request?
2) If it is, then at least for ones that are commonly accessed by application code, we need a proper API around them. Probably some kind of adapter along the lines of #2103301: Add a helper class to introspect a given request.

Actually, CustomAccessCheck already supports custom params like controllers, we just weren't using it.
I've adjusted AccessManager to follow suit.

CustomAccessCheck ones and controllers don't have any problem with this, since the method passed to the controller resolver are not backed by an interface.
However, FormBuilder and AccessInterface have the method, which forces additional params to use the = NULL hack.

If anyone else thinks this is a good idea, I can open a dedicated issue for it and work through more of the conversions.

However, FormBuilder and AccessInterface have the method, which forces additional params to use the = NULL hack.

It feels like we could run into many problems once the upcasting is not done automatically but requires type hinting. In these cases the access checker might expected upcasted information which are not available all the time.

I don't think there's that much usage to require DX sugar on top of the legitimate usage of getting values for the named parameters of routes. I mean, maybe it would be nice if Symfony added a getAttribute() method to Request, allowing us to remove an arrow, and if someone wants to submit a PR for them to do that, please do, but I don't think it's a big deal, given how relatively rarely core needs to do this outside of access checkers.

I agree with you that we overestimate the problem caused by that.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php
@@ -38,20 +38,18 @@ public function __construct(EntityManagerInterface $entity_manager) {
-  public function access(Route $route, Request $request, AccountInterface $account) {
-    if ($entity_type_id = $route->getDefault('entity_type_id')) {
-      $view_mode = $request->attributes->get('view_mode_name');
-
-      if (!($bundle = $request->attributes->get('bundle'))) {
+  public function access(Route $route, Request $request, AccountInterface $account, $view_mode_name = NULL, $bundle = NULL, $entity_type_id = NULL) {

Except for the = NULL part, this is pretty sweet.

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -325,6 +328,41 @@ protected function checkAny(array $checks, $route, $request, AccountInterface $a
+    // Add the route and account to trick the getArguments() method of the
+    // controller resolver.
+    $request->attributes->set('route', $route);
+    $request->attributes->set('account', $account);

This, however, is not, because it makes 'route' and 'account' invalid URL parameters for all routes.

I think both the =NULL and the route/account problems can be fixed by moving the ControllerResolver usage from AccessManager to an access checker base class, where the signature of access() can remain unchanged, and another method gets added as the callable? Or maybe there's another way?

If anyone else thinks this is a good idea, I can open a dedicated issue for it and work through more of the conversions.

Yeah, I think a new issue would be good, since this is not directly addressing this issue's title, only some of the DX concerns raised in comments.

Does it seem ok to have Request::getRoute, Request::getController? If it doesn't, I don't see how we could possibly justify throwing them into Request's attributes, regardless of whether or not anyone else is doing it.

I don't think it's ok to have those as methods on the Request class. Here's why: the Symfony Request class (not just the implementation, but the very concept) is meant to be a very generic abstraction of an HTTP request. As such, it has no awareness of concepts like routes and controllers. Instead, the only concept it has other than what comes from HTTP is attributes, and it's left to the framework/application to decide what to use attributes for. While we could subclass it, that wouldn't really model the fact that the framework/application can be (and in Drupal's case, is) componentized, where each component can define more attributes. So, I think an adapter pattern makes more sense than inheritance in this case, so basically agree with #14, which I also outlined in #2103301-45: Add a helper class to introspect a given request.

To do that would require a method not on an interface, something like this. I'll probably open the follow-up tonight, just wanted to get this out

I'll probably open the follow-up tonight

Any update on that, or are you waiting on more +1s first?

1. +++ b/core/modules/contact/lib/Drupal/contact/Access/ContactPageAccess.php
   @@ -47,18 +55,26 @@ public function __construct(ConfigFactoryInterface $config_factory, UserDataInte
   +   * Process arguments for this access check.
   
   +++ b/core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.php
   @@ -36,13 +42,22 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +   * Process arguments for this access check.
   

   Processes

2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php
   @@ -24,6 +24,9 @@ class ViewModeAccessCheck implements AccessInterface {
   +  protected $entityTypeId;
   +  protected $viewModeName;
   +  protected $bundle;
   
   @@ -36,29 +39,38 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +   * @param string $view_mode_name
   +   * @param string $bundle
   +   * @param string $entity_type_id
   

   Desc missing.

@effulgentsia, thanks for the reminder: #2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes

@jibran, c'mon, there's a difference between a patch-as-proposal and patch-needing-nitpicks...

Patch no longer applied. Straight reroll of #26.

Re #30, thanks, but that patch is now its own child issue in #2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes. Retitling this to a meta issue for clarity.

We discussed this issue yesterday at DevDays with catch, pwolanin, msonnabaum, xjm, berdir, dawehner, bejeebus, and me. Here's a summary of that discussion. (For the people who were there, please correct me if I got any of these notes wrong.)

tl;dr: Eventually reach a point where we stop using $request->attributes entirely, and instead have $request, $route_match, and $context_container as 3 entirely separate objects.

We wanted to approach the problem in 2 steps: 1st to discuss what we think makes the most architectural sense if we were to ignore pre-existing Symfony decisions, 2nd to discuss how to take existing Symfony decisions into account.

For the first part, msonnabaum, catch, and pwolanin felt pretty strongly that having route parameters (e.g., $node), and the route name and object, in $request->attributes does not make architectural sense, since from a conceptual standpoint, that information isn't just about the request itself, but the intersection of the request and a route. They see $request->attributes->get('node') as a regression from menu_get_object('node') in that the latter at least is clear about the menu system being the owner-of / entry-point-to that information.

They suggested that ideally we would do something more like what ZF2 does, and create a RouteMatch object, and have application code that needs access to that information look more like:

$route_match->getRouteName()
$route_match->getParameter('node')
$route_match->getRawParameter('node')

rather than

$request->attributes->get(RouteObjectInterface::ROUTE_NAME)
$request->attributes->get('node')
$request->attributes->get('_raw_variables')->get('node')

If there's agreement about this being logical in the abstract, let's consider what would be needed to implement this within Symfony constraints.

First of all, there is very little Symfony code that requires/expects route parameters to be in $request->attributes:

• Symfony\Component\HttpKernel\EventListener\RouterListener puts them there, but that's a single class, and we can override that if we want to.
• Symfony\Component\HttpKernel\Controller\ControllerResolver::doGetArguments() expects them there, but we can override that service as well.
• The biggest Symfony limitation is that RequestMatcherInterface::matchRequest() is defined as returning an array, so the CMF router, along with route enhancers all deal with an array that smushes raw parameters, upcasted parameters, and the matched route name and object, into a single place. However, per above, neither the CMF router nor the Drupal route enhancers or param converters are coupled to what happens to this array; instead of adding it to $request->attributes, we could take the result of $router->matchRequest() and unpack it into a $route_match object, without violating any routing expectations: yay for decoupled components!

However, that then leads to the question of how does various Drupal code get the appropriate $route_match object when it needs it:

• If we override ControllerResolver, then controllers can continue to just get their arguments via their method signatures.
• For access checkers, we can do the same in #2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes.
• For AccessManager, we can change the check*() methods to act on $route_match instead of $route, and the checkNamedRoute() method to create the needed $route_match object.
• We have a bunch of places where we pass $request to plugin/service/etc. methods. For example, LocalActionInterface::getOptions(), LanguageNegotiationUserAdmin::isAdminPath(), and many more. We'd need to decide for each of these whether to pass in only $request, or only $route_match, or both.
• And finally, there's both procedural (e.g., node_is_page()) and OOP (e.g., MenuTree::buildPageData()) code that gets the current request from the "global" stack. How would this code get at the corresponding $route_match object?

Assuming we do all that and completely remove route name, route object, and route parameters from $request->attributes, we'd then need to deal with the various other things currently in there, like _system_path, _authentication_provider, _theme_active, and others. We discussed the possibility of a $context_container object, with each of these being its own tagged service, similar to how we currently implement "cache contexts" (introduced in #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags).

Finally, we also discussed that as a temporary step (where temporary can be anywhere from very short lived to lasts until D9 to anything in between), we can keep the stuff in $request->attributes "under the hood", so long as all core modules exclusively use the $route_match and $context_container (or individual context service) objects as the API to get at them rather than ever interacting with $request->attributes directly.

Thoughts?

I'd like to point out that the Breadcrumb API entirely relies on request attributes.

Here are the notes from the discussion we had at Szeged:
https://docs.google.com/a/acquia.com/document/d/15gMoXoJmpXHzyR2hVatjJu-...

Setting active to avoid confusion for the moment. I think we wanted to create a new meta issue based on #32 and the discussion?

See https://drupal.org/node/2231127 for another example of problematic DX with request attributes.

Thank you for the writeup!

I think it is pretty clear that all of us agrees that accessing those attributes directly is a really BAD dx, even if you could have a better discoverability.

For the first part, msonnabaum, catch, and pwolanin felt pretty strongly that having route parameters (e.g., $node), and the route name and object, in $request->attributes does not make architectural sense, since from a conceptual standpoint, that information isn't just about the request itself, but the intersection of the request and a route. They see $request->attributes->get('node') as a regression from menu_get_object('node') in that the latter at least is clear about the menu system being the owner-of / entry-point-to that information.

Symfony really overuses the attributes all over the place. If we remove the usage of the attributes internally we though have potentially an issue with being uncoupled to all people who come to us from the the symfony side.
They might just expect things to be there, so the question is whether we want to come towards them and still allow what they are used to see.

If we decide to use the route result object internally we are more coupled than before.

We have a bunch of places where we pass $request to plugin/service/etc. methods. For example, LocalActionInterface::getOptions(), LanguageNegotiationUserAdmin::isAdminPath(), and many more. We'd need to decide for each of these whether to pass in only $request, or only $route_match, or both.

We should at least try to make it consistent to not require knowledge about several concepts at the same time.

And finally, there's both procedural (e.g., node_is_page()) and OOP (e.g., MenuTree::buildPageData()) code that gets the current request from the "global" stack. How would this code get at the corresponding $route_match object?

This is a good question! One simple approach could be certainly that we have something similar as the request stack but for route match objects, which will give you the same kind of information.

So, I'm finding the write-up a bit unclear - it's a bit stream-of-consciousness.

In general, I think the take-home pointe where that the way we put the upcast object directly into attributes is also a bit weird, and the current naming of _raw_variables isn't very clear, and having to know how to pull values out of the Request attributes is bad DX.

I'm not sure in the google doc what the discussion of MatchedRoute is about. My recollection of the discussion was that we wanted some clean interface for getting the raw, upcast, and other route parameters and any other useful data that's being carried around on the Request attributes. I guess MatchedRequest is supposed to be the class (or interface) that handles this?

Then we would use this as a facade so that we could more readily change or refactor the way values are stored on the Request, or so we could even move them elsewhere.

By default, there should be some factory or service to give this to you with a default to using the current request.

Tomorrow, I'll work on filing some child issues to help clarify #32.

So, I'm finding the write-up a bit unclear

Do you mean #32 or #34? The notes in #34 are pretty raw, but I tried to be clear in #32; can you elaborate on what needs further clarification after reading this comment?

Then we would use [MatchedRequest] as a facade

The facade approach failed to get consensus in #2103301: Add a helper class to introspect a given request. So based on our Szeged meeting, I thought to try a first class value object approach. Patch for that posted in #2238217: Introduce a RouteMatch class. Please provide feedback there.

By default, there should be some factory or service to give this to you

That's the part I'm still fuzzy on. I like the general idea of the "route match stack" suggestion from #36, but don't yet have a clear vision for how that will integrate everywhere. I'm hoping that could be a follow up to #2238217: Introduce a RouteMatch class if other than that TBD, people here generally like that direction.

Another (minor) child issue: #2238945: Remove the 'clean_urls' request attribute

Yet another child issue: #2238981: Change controllers to receive route parameters in their signature rather than via $request->attributes

Four more, one for each "internal" attribute being used within at least one core module:
- #2239001: Remove the '_menu_admin' request attribute
- #2239003: Remove the '_http_statuscode' request attribute
- #2239005: Remove the '_maintenance' request attribute
- #2239009: Remove public direct usage of the '_system_path' request attribute

There are other "internal" attributes that are only in use by /core/lib, and we may want to open issues for them, but the above set takes care of /core/modules, which I think is the higher priority set to take care of before beta.

Updated the issue summary to what I believe current consensus to be. Please correct me if I'm wrong about that. Per the new summary, I also split this issue's scope from #2256669: [meta] Finalize module-facing API of getting the matched route name and a matched route argument for a request: the two meta issues can be siblings rather than parent/child.

There are more that are available, but mostly not used like: _exception_statuscode

Per #2239009-12: Remove public direct usage of the '_system_path' request attribute, I closed that issue as a duplicate of #2238217: Introduce a RouteMatch class. That latter issue is a beta blocker. The remaining usages of internal attributes are few enough that I think they can be converted to proper APIs after beta with very little disruption, so removing the tag. @catch: please add it back if you disagree.

What is the plan for current_path() since this is called 39 times and is just a hidden call to $request->attributes->get('_system_path')?

Now that all the child issues are completed, is this meta issue closed?

I think the only thing that's left to do here is:

Once all usages of all these is strictly internal, document somewhere that that's our rule in general.

Also, is the change record referenced in #18 correct now, and do we need any others or have the child issues taken care of all the needed ones already?

The CR needs an update to explain how this can now be done.

Currently it has not much information ...

What's left here?

I think all of the points are addressed here. We still have _authentication_provider but this is not a public API at all.