Remove _system_path from $request->attributes

Note: Most of _request_path is already using current_path() which we have a replacement for in case there is routing out.

Update the IS with some more description.

First patch, only for core/lib/Drupal/Core/Form/FormSubmitter.php

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -137,12 +147,15 @@ public function redirectForm(FormStateInterface $form_state) {
       $request = $this->requestStack->getCurrentRequest();
-      // @todo Remove dependency on the internal _system_path attribute:
-      //   https://www.drupal.org/node/2293521.
-      $url = $this->urlGenerator->generateFromPath($request->attributes->get('_system_path'), array(
-        'query' => $request->query->all(),
-        'absolute' => TRUE,
-      ));
...
+      $url = $this->urlGenerator->generateFromRoute(
+        $this->currentRouteMatch->getRouteName(),
+        $this->currentRouteMatch->getParameters(),
+        array(
+          'query' => $request->query->all(),
+          'absolute' => TRUE,
+        )
+      );

Dump question ... isn't all you need $request->getPathInfo()? basically + http_build_query()? Then you could get rid of the currentRouteMatch dependency in total?

@dawehner yes and no.

We can just drop off the call to $request->attribute->get() with your suggested solution but the UrlGeneratorInterface definition:

interface UrlGeneratorInterface extends VersatileGeneratorInterface {
  /* @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 8.0.0. */
  public function generateFromPath($path = NULL, $options = array());
  /* @deprecated in Drupal 8.0.x-dev and will be removed before Drupal 8.0.0. */
  public function getPathFromRoute($name, $parameters = array());
  public function generateFromRoute($name, $parameters = array(), $options = array());
}

require to use the generateFromRoute method that need to use the route (other methods are deprecated), so we can't use the $request->getPathInfo() method to ensure that we don't need to rework it before drupal 8.0.0.

Let me know if I'm in the right direction or not.

PS: attached patch remove some changes in tests that are not required.

Let's see!

Patch re-rolled

Fixed a test that require a redirect also when form submission is executed programmatically.

Introduced changes for:

• core/modules/system/tests/modules/theme_test/src/EventSubscriber/ThemeTestSubscriber.php

Using the same approach to get current route and then compare it.

EDIT: sorry, my fault on interdiff extension :(

@dawehner individuate a "fix" for the test:

+++ b/core/modules/system/src/Tests/Form/ElementsTableSelectTest.php
@@ -217,6 +217,7 @@ private function formSubmitHelper($form, $edit) {
+    $form_state->disableRedirect();

This disable redirect for forms submitted programmatically. This is the solution I suppose to be more clean because reduce the amount of code in the FormResolver and just set the test that is execute programmatically (in a form without route) to skip redirect step (there are no pages, so no redirect are possible).

A second solution can be to add a condition to skip redirect if there is not route defined for the current request:

if {
  /* .... */
} elseif ($redirect === NULL && !is_null($this->currentRouteMatch->getRouteName())) {
  /* ... */
}

this allow to skip redirection for forms without routing.

Opinions?

Did you tried using <current> as route name?

@dawehner do you mean in tests?

Well, no, in actual used code.

For the particular example in FormBuilder I really wonder why we can't just use $request->getUri() given that its also an absolute URL, including
a query string.

Needs work to use <current> instead.

So here is a way simpler approach.

Using the approach you indicate we don't re-calculate the URI after the form submission and the redirect is not correctly generate.

Eg, after submitting the LanguageConfiguration switching from a lang A to a lang B we need to re-define the page URI based on default language, language prefix & c. Using current URI can generate issue like: user change the language prefix from XX to YY, current URI use XX as path prefix, after the POST the form set the redirect to XX but the language don't exist (it's changed to YY) and user receive error. Some other issues are related to use the current URI without regenerate it (eg we don't remove the query string parameters, ..).

I think using the route based navigation & redirect (if possible) is better then use the current URI (that can be used as failback).

Good point!

What so about using <current> like this?

@dawehner sound really better :)

I'm working on this part in the last days:

• core/lib/Drupal/Core/EventSubscriber/ExceptionLoggingSubscriber.php (We should log /d8/node instead of just "node")

and the only solution I found is a string manipulation from currentUri, but sound like a hack, and i don't like it :| I will update ASAP.

and the only solution I found is a string manipulation from currentUri, but sound like a hack, and i don't like it :| I will update ASAP.

Yeah I think the only proper way is to have an actual API to get the system path.

+++ b/core/modules/system/src/Tests/Form/ElementsTableSelectTest.php
@@ -217,6 +217,7 @@ private function formSubmitHelper($form, $edit) {
+    $form_state->disableRedirect();

Any reason for this? Can we add a comment here?

@jibran see https://www.drupal.org/node/2372507#comment-9373435 I will add a comment on code.

last patch seems good to me. Moving it to RTBC

#33 should be addressed

@alexpott @RavindraSingh @jibran added comment required on #33

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -33,15 +34,24 @@ class FormSubmitter implements FormSubmitterInterface {
+  public function __construct(RequestStack $request_stack, UrlGeneratorInterface $url_generator, RouteMatchInterface $current_route_match) {

140 character long in a single line. needs breakpoint.

Also, Formatted some whitelines.

1. +++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
   @@ -33,15 +34,25 @@ class FormSubmitter implements FormSubmitterInterface {
   -  public function __construct(RequestStack $request_stack, UrlGeneratorInterface $url_generator) {
   +  public function __construct(RequestStack $request_stack, UrlGeneratorInterface
   +      $url_generator, RouteMatchInterface $current_route_match) {
   

   Only limit comments to 80 characters, never actual PHP code.

2. +++ b/core/modules/system/src/Tests/Form/ElementsTableSelectTest.php
   @@ -215,15 +214,16 @@ private function formSubmitHelper($form, $edit) {
        $form['#token'] = FALSE;
   -
   ...
        $form_state->setFormObject(new StubForm($form_id, $form));
   -
        \Drupal::formBuilder()->prepareForm($form_id, $form, $form_state);
   -
        \Drupal::formBuilder()->processForm($form_id, $form, $form_state);
   -
        $errors = $form_state->getErrors();
   

   No need to touch these lines.

@tim.plunkett take a look to #38 I think it's ok.

+++ b/core/lib/Drupal/Core/Form/FormSubmitter.php
@@ -137,12 +147,15 @@ public function redirectForm(FormStateInterface $form_state) {
+
+      $url = $this->urlGenerator->generateFromRoute(
+        $this->currentRouteMatch->getRouteName(),
+        $this->currentRouteMatch->getRawParameters()->all(),
+        array(
+          'query' => $request->query->all(),
+          'absolute' => TRUE,
+        )
+      );

So why did you skipped the usage of <current> again? All what your code is doing is the exact same thing, but with just more effort.

Merged in the interdiff in 38 with the approach in 30.

... I think at this point it seems tricky to replace more of the _system_path examples.

Removed two more instances.

Some work to get rid of some more examples

This could be it.

+++ b/core/lib/Drupal/Core/EventSubscriber/CustomPageExceptionHtmlSubscriber.php
@@ -63,7 +63,7 @@ protected static function getPriority() {
-    $this->makeSubrequest($event, $path, Response::HTTP_FORBIDDEN);
+    $this->makeSubrequest($event, $event->getRequest()->getBasePath() . '/' . $path, Response::HTTP_FORBIDDEN);

This is not so pretty, and possibly easy to do incorrectly and not notice if you are not in a subpath?

Maybe we could add the basepath by default, we have the $event available in there, with an argument to disable that?

This is not so pretty, and possibly easy to do incorrectly and not notice if you are not in a subpath?

Adapted the code to accept both a relative URL and a path, if you think its worth.

Note: The bugfix was discussed with timplunkett yesterday

On top of it, I ensured that the unit test works and wrote some additional integration test

Here is an idea I had in mind .. have an object similar to CurrentRouteMatch which stores the path. Yes, its discouraged to use it, but there are examples where you simply can't get around it. Not sure whether I like this approach yet though.

@dawehner the idea you had in mind in #55 with CurrentPath seems very straight forward. Moves some of the internals out of the way and saves from having to know you need the current request object, or how to get it if you don't have it.

+1 to #55

@dawehner the idea you had in mind in #55 with CurrentPath seems very straight forward. Moves some of the internals out of the way and saves from having to know you need the current request object, or how to get it if you don't have it.

Well yeah, its straightforward, but whether its the way to go, I don't know. Anyone else has an oppinion? I guess I should just first try to push it to green.

#55 is what I've tried to propose in #2239009-15: Remove public direct usage of the '_system_path' request attribute, so definitely +1 from me.

@znerol
Great to give an ok from you! Just to be clear, I still think we should

Let's fix the failures quickly/

I like this new approach too, it's much better than any other ideas we've brainstormed.

Grepping for _system_path, the only remaining occurrence is in a comment in RouteProviderTest, let's kill that too.

Maybe we should retitle the issue to clarify that we're completely removing _system_path?

Alright

Fixed that instance and tried to update the IS

I like the overall direction. Mainly have some concerns about what "a path" now is exactly: Symfony-style (for full consistency), or "D7-style" (for legacy reasons)? IMHO that distinction either needs to go away (Symfony-style everywhere in this patch), or needs to be thoroughly explained and stressed on CurrentPath.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -87,24 +88,26 @@ public function on404(GetResponseForExceptionEvent $event) {
   +   *   The path/url to which to make a subrequest for this error message.
   

   s/url/Drupal root-relative URL/

   Since AFAICT absolute URLs aren't supported (which makes sense).

   Actually, even that is confusing. What this really is, is a path as defined in a *.routing.yml file, right? And optionally, a path as in a D7-style "path".

   The fact that I'm so confused by this means we should clarify this.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -87,24 +88,26 @@ public function on404(GetResponseForExceptionEvent $event) {
   +    if (!($url && $url[0] == '/')) {
   +      $url = $request->getBasePath() . '/' . $url;
   +    }
   

   I think a comment for this is desirable.

3. +++ b/core/lib/Drupal/Core/Path/CurrentPath.php
   @@ -0,0 +1,84 @@
   + *   path processing, there is though no way around dealing with paths.
   

   "though" sounds wrong to me here. "unfortunately" fits better.

4. +++ b/core/lib/Drupal/Core/Path/CurrentPath.php
   @@ -0,0 +1,84 @@
   +   * Internal cache of paths.
   

   Static, not internal?

5. +++ b/core/lib/Drupal/Core/Path/CurrentPath.php
   @@ -0,0 +1,84 @@
   +   * @param string $path
   +   *   The path.
   ...
   +  public function setPath($path, $request = NULL) {
   

   This stores a "D7-style path", not the path as defined in Symfony routes. Do we really want that?

6. +++ b/core/lib/Drupal/Core/RouteProcessor/RouteProcessorCurrent.php
   @@ -36,12 +36,18 @@ public function __construct(RouteMatchInterface $route_match) {
   +        // If we have no current route match available, point to the frontpage.
   +        $route->setPath('');
   

   I'd have expected this would have to be '/'?

   EDIT: ahh, this is a "D7-style path", because the corresponding test coverage does assert '/'. So… that makes this even more confusing, because we're actually working with a Symfony Route object, which always works with a leading slash.

7. +++ b/core/modules/system/src/Tests/Form/ElementsTableSelectTest.php
   @@ -217,6 +217,11 @@ private function formSubmitHelper($form, $edit) {
   +    // solution to skip redirect step (there are no pages, then the redirect
   

   s/skip redirect step/skip the redirect step/

8. +++ b/core/modules/system/src/Tests/RouteProcessor/RouteProcessorCurrentIntegrationTest.php
   @@ -96,6 +96,19 @@ public function testProcessOutbound() {
   +    $this->assertEqual('/', \Drupal::url('<current>'));
   

   Could use a comment saying that this verifies <current> is mapped to the frontpage when missing.

9. +++ b/core/modules/system/src/Tests/Routing/RouteProviderTest.php
   @@ -345,11 +355,11 @@ function testOutlinePathNoMatch() {
   +   * Ensures that a manual test current path overrides the request path.
   

   Confusing sentence.

10. +++ b/core/modules/system/tests/modules/theme_test/src/EventSubscriber/ThemeTestSubscriber.php
    @@ -24,6 +25,22 @@ class ThemeTestSubscriber implements EventSubscriberInterface {
    +  }
    +
     
       /**
    

    Extraneous newline added.

@wim
Thank you for the review!!

Actually, even that is confusing. What this really is, is a path as defined in a *.routing.yml file, right? And optionally, a path as in a D7-style "path".

Well, I blame berdir in #53. If you ask me, this is not a public API here, so requiring a URL, instead of a path is a good thing, IMHO.

"though" sounds wrong to me here. "unfortunately" fits better.

To be clear, I think path processing should at that point continue dealing with paths, because otherwise they would have to be renamed to URLProcessing for example which probably would be too much of an API change
without that much of a win.

Static, not internal?

Let's change it, but I don't care that much, to be honest.

This stores a "D7-style path", not the path as defined in Symfony routes. Do we really want that?

Well, some users append a "/" at the moment, some don't and would have to shave it off.

I'd have expected this would have to be '/'?

EDIT: ahh, this is a "D7-style path", because the corresponding test coverage does assert '/'. So… that makes this even more confusing, because we're actually working with a Symfony Route object, which always works with a leading slash.

Well, to be clear, it doesn't matter, because of the following code in the UrlGenerator:

        if ('' === $url) {
            $url = '/';
        }

Let's change it.

Confusing sentence.

Improved.

+++ b/core/lib/Drupal/Core/RouteProcessor/RouteProcessorCurrent.php
@@ -46,7 +46,7 @@ public function processOutbound($route_name, Route $route, array &$parameters) {
-        $route->setPath('');
+        $route->setPath('/');

So this was a bug? Missing test coverage then :(

From IRC:

15:29:28 WimLeers: dawehner: so I realize my comment was kind of rambling :P
15:29:34 WimLeers: which makes your answer also not very clear
15:29:42 WimLeers: But basically my biggest concern is "what is a path"
15:29:59 WimLeers: Do we keep it D7-style path? That's easier perhpaps, but inconsistent with the whole
15:30:00 dawehner: WimLeers: well a path is a relative drupal url without the "/"
15:30:10 WimLeers: But why not with a leading slash? Then it's all consistent
15:30:22 WimLeers:
CurrentRoute == the route in routing.yml
CurrentPath   == the path for that route in routing.yml
15:30:23 dawehner: ... we have path processing
15:30:33 dawehner: ... we have path aliases
15:30:36 dawehner: where do you want to stop
15:30:44 WimLeers: Right, the comparison I make is therefore invalid, you say?
15:30:55 WimLeers: that makes sense I think

So then my biggest remaining concern is that CurrentPath should excessively strongly stress that it's NOT a Symfony route's path, but the processed path, and potentially a path alias, i.e. the The entire string after your hostname + subdir, minus e.g. the language prefix..
I wish I had a more precise suggestion than that, but I don't. Perhaps effulgentsia does?

So this was a bug? Missing test coverage then :(

No Its not UrlGenerator($route('/')) == UrlGenerator($route('')); ... its a feature of the system

So then my biggest remaining concern is that CurrentPath should excessively strongly stress that it's NOT a Symfony route's path, but the processed path, and potentially a path alias, i.e. the The entire string after your hostname + subdir, minus e.g. the language prefix..
I wish I had a more precise suggestion than that, but I don't. Perhaps effulgentsia does?

At least having no slash here makes it clear that its not the incoming path, but whether and how to name it better I'm not sure.

Adapt the issue title.

Pff, now it's my fault :p

I was just thinking out loud on how to improve that. I'm not sure if making sub-requests is that much of an internal API?

Also, @WimLeers nicely shows why I suggested to make this easier, see #2368987-66: Move internal page caching to a module to avoid relying on config get on runtime, you can *not* just hardcode a leading / when working with a url string.

I was just thinking out loud on how to improve that. I'm not sure if making sub-requests is that much of an internal API?

Well, the API is the http kernel ... its just the makeSubrequest method which is a protected method of DefaultExceptionHtmlSubscriber.

1. +++ b/core/lib/Drupal/Core/Path/CurrentPath.php
   @@ -0,0 +1,84 @@
   +    $this->paths = new \SplObjectStorage();
   

   <3

2. +++ b/core/modules/system/src/Plugin/Condition/RequestPath.php
   @@ -57,6 +65,8 @@ class RequestPath extends ConditionPluginBase implements ContainerFactoryPluginI
   +   *   The curren path.
   

   current

3. +++ b/core/modules/system/src/Tests/RouteProcessor/RouteProcessorCurrentIntegrationTest.php
   @@ -96,6 +96,20 @@ public function testProcessOutbound() {
   +    // In case we have to routing, the current route should point to the front.
   

   This comment doesn't make sense

Other than that, looks RTBC to me - these are nits that could be fixed on commit.

Thank you for your quick review!

current

Mh, curren is not proper australian english?

Thanks
Please refer to this instructional video regarding Australian English https://www.youtube.com/watch?v=DHQRZXM-4xI (probably should say NSFW, some may find it offensive)
Note: Australians love self deprecating humour.

I was also reviewing this, I found no other nitpicks and already +1'd the direction in #61.
RTBC++

It would be great to get a +1 from crell, honestly.

Summoning him.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php
   @@ -130,4 +133,11 @@ protected function makeSubrequest(GetResponseForExceptionEvent $event, $path, $s
   +  /**
   +   * Wraps drupal_get_destination().
   +   */
   +  protected function drupalGetDestination() {
   +    return drupal_get_destination();
   +  }
   

   We haven't gotten rid of drupal_get_destination() yet? Oh bother... Is there a follow-up to do so? If not, let's file it before this is committed.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionLoggingSubscriber.php
   @@ -45,9 +45,7 @@ public function __construct(LoggerChannelFactoryInterface $logger) {
   -    // @todo Remove dependency on the internal _system_path attribute:
   -    //   https://www.drupal.org/node/2293523.
   -    $this->logger->get('access denied')->warning(String::checkPlain($request->attributes->get('_system_path')));
   +    $this->logger->get('access denied')->warning(String::checkPlain($request->getRequestUri()));
   

   This does change what the message will be, but I think it's a good change anyway. (Report the URI that the user requested, not the internal one.)

3. +++ b/core/lib/Drupal/Core/Path/CurrentPath.php
   @@ -0,0 +1,84 @@
   +class CurrentPath {
   

   I would actually suggest we rename this class to something like CurrentPathStack or similar (and then the variables that reference it, too). As is, it suggest that it's a static computed value rather than a service. I spent 10 minutes typing up a long screed below about how passing that to the constructor of various other services makes the services brittle and dependent on the request it was instantiated with before I remembered it was a lookup table, not a value. It's still ugly and wrong because it's stateful but we can at least name it such that people are less likely to confuse it with a value object.

4. +++ b/core/lib/Drupal/Core/Path/CurrentPath.php
   @@ -0,0 +1,84 @@
   +    if (!isset($this->paths[$request])) {
   +      $this->paths[$request] = trim($request->getPathInfo(), '/');
   +    }
   

   It may not be possible to resolve at this point, but this suggests that we are locking in the idea that sometimes paths have a leading /, sometimes not. If this is the last time we're futzing with _system_path (as it was the main place that non-/ paths came from), can we clean that up at the same time? Let's leave the / on. The patch is "only" 50 KB, and I suspect it's already touching all the places we'd need to modify anyway.

5. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -104,22 +115,7 @@ public function __construct(Connection $connection, RouteBuilderInterface $route
   +    $path = '/' . $this->currentPath->getPath($request);
   

   This is the kind of thing that we should clean up.

6. +++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
   @@ -99,8 +100,10 @@ class PathBasedBreadcrumbBuilder implements BreadcrumbBuilderInterface {
   -  public function __construct(RequestContext $context, AccessManagerInterface $access_manager, RequestMatcherInterface $router, InboundPathProcessorInterface $path_processor, ConfigFactoryInterface $config_factory, TitleResolverInterface $title_resolver, AccountInterface $current_user) {
   +  public function __construct(RequestContext $context, AccessManagerInterface $access_manager, RequestMatcherInterface $router, InboundPathProcessorInterface $path_processor, ConfigFactoryInterface $config_factory, TitleResolverInterface $title_resolver, AccountInterface $current_user, CurrentPath $current_path) {
   

   8 service dependencies is a strong code smell. So is the previous 7, though... Is there no way we can clean this up?

7. +++ b/core/modules/system/src/Tests/Plugin/Condition/RequestPathTest.php
   @@ -91,7 +101,7 @@ public function testConditions() {
   -    $request->attributes->set('_system_path', 'my/aliased/page');
   +    $this->currentPath->setPath('my/aliased/page', $request);
   

   As noted above, these should be using /-based paths now.

Thank you for your review crell!

We haven't gotten rid of drupal_get_destination() yet? Oh bother... Is there a follow-up to do so? If not, let's file it before this is committed.

Sure, here is a follow up
#2426805: Modernize drupal_get_destination()

It may not be possible to resolve at this point, but this suggests that we are locking in the idea that sometimes paths have a leading /, sometimes not. If this is the last time we're futzing with _system_path (as it was the main place that non-/ paths came from), can we clean that up at the same time? Let's leave the / on. The patch is "only" 50 KB, and I suspect it's already touching all the places we'd need to modify anyway.

You know, its 50/50, but I agree, let's change it. Many people requested that already.

8 service dependencies is a strong code smell. So is the previous 7, though... Is there no way we can clean this up?

Do you want to know my opinion? The problem is not the breadcrumb builder but rather the problem that the routing system (symfony routing + our additions)
is not written in a good reusable way, but rather we just put things on top of an existing design. All those services are needed, when you basically want
to rerun routing. I won't change things for now, I hope this is okay.

I would actually suggest we rename this class to something like CurrentPathStack or similar (and then the variables that reference it, too). As is, it suggest that it's a static computed value rather than a service. I spent 10 minutes typing up a long screed below about how passing that to the constructor of various other services makes the services brittle and dependent on the request it was instantiated with before I remembered it was a lookup table, not a value. It's still ugly and wrong because it's stateful but we can at least name it such that people are less likely to confuse it with a value object.

You know, this is at least consistent with RequestMatchStack and AccountProxy kinda. Its the same general problem.

Ha, broken core is broken.

Let's fix the remaining issues.

Crell made his review, let's remove the assignment field again.

back again

8 service dependencies is a strong code smell. So is the previous 7, though... Is there no way we can clean this up?

Seeing similar issues in #2090115: Don't install a module when its default configuration has unmet dependencies for config.installer - do we have a strong opinion on something of a proxy service?

RoutingProxy {
  public function getAccessControlHandler();
  public function getRouter();
  public function getInboundPathProcessor();
  public function getRequestContext();
  public function getCurrentPath();
}

Then services needing all five could just rely on the proxy service?

@larowlan
Let's discuss that, because there are potentially more to use.

I'm a bit confused by #76.4 (leading /) and the changes based on that. At least, we need a follow-up to fix the documentation in CurrentPathStack::getPath() because that still says it returns it without a leading /, and given that it is 50/50 when we need a leading / it, maybe there should be an easier way to get the path without leading /? IMHO, leading / was always a good way to differentiate between a url string (which is yet another thing, as commented above, it also includes the subfolder you're in) and the path.

#2430805: Fix CurrentPathStack::getPath() documentation that says it has no leading / (and make it easier to actually get that?)