Remove public direct usage of the '_system_path' request attribute

Opened two small issues: #2259315: Remove _system_path usage from shortcut.module and #2259317: Remove _system_path usage from system_test.module. Combined with #2183113: Update hook_help signature to use route_name instead of path, that gets us to few enough usages of _system_path remaining, that I don't think it needs to be considered a "critical API", and therefore, I don't think needs to block beta. Swapping tags accordingly.

From my understanding system_path belongs onto the the thing we call RequestContext from symfony but maybe simply an extended one from drupal.

Hm, that's an interesting idea. Request::getPathInfo() probably should stay as the path that was actually submitted by the user (aliased, language-prefixed, etc.), but perhaps RequestContext::getPathInfo() could be used to return the system path. We wouldn't need to extend RequestContext then, unless we wanted to have a separate method for retrieving the user-submitted path, but I don't see why we'd need that method if you could get at that via $request. Anyone else have thoughts on that approach? I guess it would mean that all code that does need system_path (which isn't and shouldn't be much code at all) would need to get injected with a RequestContext object instead of or in addition to $request.

Tagging WSCCI for the idea in #3/#4.

Ignoring the usages fixed by #2183113: Update hook_help signature to use route_name instead of path and #2259315: Remove _system_path usage from shortcut.module, here are all the places (excluding tests):

Calling current_path(), from either procedural code, or from OOP code that doesn't have an injected $request or $request_stack:

template_preprocess_pager()
tablesort_header()
BlockAccessController::checkAccess()
contact_mail()
LanguageBlock::build()
shortcut_preprocess_page()
shortcut_set_switch_submit()
system_page_build()
update_page_build()
UpdateManager::projectStorage()
UserLoginBlock::build()
views_views_pre_render()
template_preprocess_views_view_table()
template_preprocess_views_mini_pager()
template_preprocess_views_view_summary()
template_preprocess_views_view_summary_unformatted()
ExposedFormPluginBase::resetForm()
ViewUI::getStandardButtons()
ViewUI::renderPreview()

Calling current_path(), from OOP code that has an injected $request or $request_stack, so should probably be changed to use it:

TermForm::form()
OverviewTerms::buildForm()
RegisterForm::form()
ViewEditForm::submitDelayDestination()
ConfigHandler::buildForm()

Calling ->get('_system_path'), from OOP code that has an injected $request or $request_stack, so should not be changed to current_path():

CsrfAccessCheck::access()
ExceptionController::on403Html()
ExceptionController::on404Html()
PathListenerBase::extractPath()
FormBuilder::redirectForm()
RouteProvider::getRouteCollectionForRequest()
UrlMatcher::finalMatch()
MenuTree::buildPageData()
MaintenanceModeSubscriber::onKernelRequestMaintenance()
(Drupal\views\Plugin\views\argument_default\)Raw::getArgument()

The reason I broke up #6 by usages of the current_path() wrapper vs. direct access of _system_path, is that if we at some point remove _system_path from $request->attributes and put it on something like RequestContext::getPathInfo() instead (per #3/#4), then so long as we retain the current_path() wrapper, those usages don't break. However, anything accessing _system_path from an injected $request will need to be changed to accessing it from an injected something else (e.g., $request_context).

I'm not sure what I think of this yet, but it implements #3 as I understand it. It doesn't convert every usage of _system_path, but hopefully does enough to get the idea. What do you all think?

+++ b/core/modules/user/lib/Drupal/user/EventSubscriber/MaintenanceModeSubscriber.php
@@ -27,7 +28,9 @@ public function onKernelRequestMaintenance(GetResponseEvent $event) {
     $request = $event->getRequest();
...
-    $path = $request->attributes->get('_system_path');
+    $request_context = new RequestContext();
+    $request_context->fromRequest($request);
+    $path = trim($request_context->getPathInfo(), '/');

This, by the way, is the part I'm unsure of. For every current usage of _system_path from a place where we get an injected $request (e.g., a kernel event like this one), we would change 1 line of code to 3, as above. And one that hard-codes the class being instantiated. That seems a bit like a loss. However, it does make conceptual sense I think, in that any code that uses _system_path, what it's really wanting is "what is the canonical path as far as routing is concerned?", which is exactly what the domain of \Drupal\Core\Routing\RequestContext is.

Discussed this with Crell, znerol, pwolanin, and neclimdul at DrupalCon Austin. We concluded that getSystemPath() or some alternate name (e.g., getMatchedPath()?) can be added as a method to RouteMatch in #2238217: Introduce a RouteMatch class. In particular, Crell felt that we should not be using RequestContext within any module code in Drupal. Please reopen this issue if that requires more discussion.

Uh, I always thought that _system_path is routing input, while RouteMatch is the output.

We discussed this on that call last week, and @Crell pointed out that it depends on perspective. If you think of "routing" as only what happens in RouterListener, then yes, _system_path is input. However, if you think of "routing" as everything that happens from $kernel->handle() to the controller being invoked, then _system_path is intermediary information that could be considered part of the domain of route matching in a broad sense.

We want to encourage usage of route match but prevent usage of system path. Therefore... I propose that we'd rather introduce a new current_system_path service

Regardless of above perspective on what constitutes routing, I think splitting out something we intend to pseudo-deprecate away from a brand new API we're creating makes a lot of sense. Here's a patch for review. I'm not sure how to document that it's discouraged: elsewhere, we have @deprecated in Drupal 8.x, will be removed before Drupal 9.0., but does it make sense to state that something is @deprecated without instructions on what to replace it with, and I don't know what to replace the two use cases in the patch with. Any thoughts?

along with Drupal::currentSystemPath()

I'd rather not add things to Drupal:: that we're trying to discourage people from using. AFAIK, current_path() isn't going away in D8, so I think is fine to leave as the sole wrapper for this for procedural code.

Raising this to beta blocker to match what #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API was prior to me changing that in #2124749-46: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API on the assumption that this would be handled as part of #2238217: Introduce a RouteMatch class. However, I don't have a strong opinion on whether this should block beta or not: on the one hand, moving from path-based coding to route-based coding is a fundamental piece of D7 -> D8 module porting, and having a clear answer on how to handle edge case code that must use path could be seen as a key piece of that. On the other hand, if it's only edge case code, then why hold up a beta on it. I think there's a good chance we can get this in before beta in either case though.

And here's the patch.

It's better to have one way to get at something rather than two.

_current_path() will go away per #24. So, ignoring that:

With HEAD, we have three ways to get the current path:

• Within a function or method where $request (or an object from which we can get $request) is passed in: $request->attributes->get('_system_path').
• Within an OOP method where $request isn't passed in: get $request_stack constructor injected and then: $this->requestStack->getCurrentRequest()->attributes->get('_system_path').
• Within a procedural function where $request isn't passed in: current_path().

With this patch, that will change to four:

• Within an OOP method where $request (or an object from which we can get $request) is passed in: get $current_system_path constructor injected and then: $this->currentSystemPath->get($request).
• Within an OOP method where $request isn't passed in: get $current_system_path constructor injected and then: $this->currentSystemPath->get().
• Within a procedural function where $request isn't passed in: current_path().
• Within a procedural function where $request is passed in: \Drupal::service('current_system_path')->get($request).

The suggestion in #23 is to remove current_path() and change the above four to (first two are the same):

• Within an OOP method where $request (or an object from which we can get $request) is passed in: get $current_system_path constructor injected and then: $this->currentSystemPath->get($request).
• Within an OOP method where $request isn't passed in: get $current_system_path constructor injected and then: $this->currentSystemPath->get().
• Within a procedural function where $request isn't passed in: \Drupal::currentSystemPath()->get().
• Within a procedural function where $request is passed in: \Drupal::currentSystemPath()->get($request).

The benefit is that that is more unified. The drawback is currentSystemPath() becomes in autocomplete suggestion whenever you type \Drupal:: into your IDE, but this is a service that ideally no contrib module would ever use, and that over the course of 8.1, 8.2, etc., we'd find a way to remove all the remaining core usages as well. Of course, one could argue that current_path() as a procedural wrapper also has that same drawback: just in the global namespace, which depending on your point of view is either better or worse.

I'd also be OK with leaving it as is and trying to document better - this is a concept we should be worrying about less and less.

Yeah, along these lines, I took another look at the list at the bottom of #6, and think that we can just remove all but 2 or 3 non-internal usages, and instead of needing a service to support those 2 or 3, can just add a comment for them explaining that we're accessing something we shouldn't be in order to maintain a legacy feature?

Specifically, the two that I suspect we'll be stuck with are:
- Drupal\system\Plugin\Condition\RequestPath (not in the list in #6 due to being added after that)
- Drupal\views\Plugin\views\argument_default\Raw

I think both of those are flawed concepts (we should be implementing block visibility conditions and Views arguments from route names and named route parameters, or from breadcrumb or menu information, not from the system path), but I don't know that we'll be able to make that change in D8.

I think user module's MaintenanceModeSubscriber is an interesting use of _system_path given that it needs to happen pre-routing. I wonder if it could be implemented with a maintenance mode router though, that only contains the 5 or so hard-coded routes. Since that might be problematic, that's why I wrote "or 3" above.

As to the rest of the list, I think CsrfAccessCheck, FormSubmitter, and MenuTree can be refactored to use route name and params, ExceptionController can be refactored to use drupal_get_destination() (or a service equivalent if that's ever made), and PathListenerBase, RouteProvider, and UrlMatcher can be considered internal consumers of _system_path, so can continue accessing it from $request directly without problems.

Not sure where that leaves this issue then. Perhaps all that's needed before beta is to see if we agree on the above plan and file (but not necessarily complete) the corresponding conversion/documentation issues?

I'm going to open non-beta-blocking issues for #29, and then upload a patch here that just has @todos pointing to them. That's a completely different approach than #21, so setting to Active until I upload that.

Alrighty, how's this for a semi-cheating way of resolving this beta blocker?