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

Good catch, yeah that got forgotten after the review of crell.

I have made changes in the documentation after deducing that we just need to return path without mentioning that / will be trimmed.Have i deduced correct ?.Please review.
Thanks

Changing the status.

Made changes according to comment 5.Please review.
Thanks.

Hm. I was asked to comment here. The current patch looks fine as far as it goes.

I'll follow additional patches... I am all for more clear documentation on what is being returned, of course! Sounds like this is Needs Work for #7?

Agreed with Berdir and jhodgdon. Standardize on leading / everywhere, fix this doc, and possibly improve the doc for which path we're talking about.

I am suggesting:

/**
   * Returns the path of the current request.
   *
   * @param \Symfony\Component\HttpFoundation\Request $request
   *   (optional) The request.
   *
   * @return string
   *   Returns the non URI decoded request path, with leading slash,
   *   without queries or the base URL.
   */

getPath() calls getPathInfo() that has:

    /**
     * Returns the path being requested relative to the executed script.
     *
     * The path info always starts with a /.
     *
     * Suppose this request is instantiated from /mysite on localhost:
     *
     *  * 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'
     *
     * @return string The raw path (i.e. not urldecoded)
     *
     * @api
     */

Two more levels in getRequestUri() has:

    /**
     * Returns the requested URI (path and query string).
     *
     * @return string The raw URI (i.e. not URI decoded)
     *
     * @api
     */

Thanks for the new patch... but it does not address the questions in #7. I am also not sure it is accurate... or at least I think using the term "URI decoded" is misleading (it sounds to me as though it's about urlencode/urldecode, and that is not related to what these functions do).

Here is a patch which fixes documentation on trailing slash, and brings over urldecoded notes from Request::getPathInfo().

OK, I'm pretty confused now.

I was attempting to review the patch here to see if it was accurate... I decided that it doesn't merit a Novice tag until we sort out what is happening. Sorry mglaman -- you followed the instructions (thanks!) but I am not sure the patch is the correct documentation.

So.

CurrentPathStack::getPath(), if you look at the code, is returning the result of a \Symfony\Component\HttpFoundation\Request::getPathInfo() call, which says "always starts with \".

However, what it actually does is to return the result of a call to its preparePathInfo() method. This method returns "\" if it can't figure out the path, and otherwise it strips getBaseUrl() off the request URL and returns that.

The base URL comes from the prepareBaseUrl() method. It looks as though this method always strips off the last / so that getPath() should always start with a / no matter what.

So I'm very confused about this issue. What makes you think that CurrentPathStack::getPath() has the wrong documentation? I must have missed something...

Adding a related issue

I'm seeing different behaviour if a node has a path alias. When the path alias is used, the returned path has a leading slash. I'm not 100% sure, but since \Drupal\Core\Url::fromUserInput requires a leading slash this gets pretty confusing.

Here's two patches - one with the docs fix, and the other that throws a \LogicException. Since this method itself doesn't I'm not 100% sure that the path alias is the cause, so I'd like to see if this fails any tests.

Huh, not running tests on CS fails is new. That's too bad, it adds a roadblock to running tests like this.

\Drupal\Core\Path\CurrentPathStack::setPath() doesn't ensure that the leading slash is set, unlike \Symfony\Component\HttpFoundation\Request::getPathInfo. That means that for routes, \Drupal\Core\Routing\RouteProvider::getRouteCollectionForRequest ends up setting paths with the leading slash. The easiest way to see this is to set a breakpoint with strpos($path, '/') === 0; at the uncached setPath call in getRouteCollectionForRequest. Clear caches from the UI, and you'll see them being set.

My initial thought was to treat the docs as correct, but stripping the leading slash in setPath() hoses the site.

Here's a patch that validates incoming paths in setPath. If all the tests pass I think we can call this a docs bug. I don't think this is a hard BC break because there's no interface defined, but it's certainly one worth calling out.

My initial thought was to treat the docs as correct, but stripping the leading slash in setPath() hoses the site.

Do you mind elaborating a bit more on that? I could imagine that throwing an exception here is problematic too. Maybe triggering an error / having an assert would be a better approach to not cause 500s on production, which leads to really problematic behaviours on reverse proxies.

Here's the exception thrown when clearing caches with the attached patch that instead removes any leading slashes. I'm guessing this will cause a ton of test fails.

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Symfony\Component\HttpKernel\Exception\NotFoundHttpException</em>: No route found for &quot;POST /admin/config/development/performance&quot; (from &quot;http://d8.local/admin/config/development/performance&quot;) in <em class="placeholder">Symfony\Component\HttpKernel\EventListener\RouterListener-&gt;onKernelRequest()</em> (line <em class="placeholder">139</em> of <em class="placeholder">vendor/symfony/http-kernel/EventListener/RouterListener.php</em>). <pre class="backtrace">Drupal\Core\Routing\AccessAwareRouter-&gt;matchRequest(Object) (Line: 115)
Symfony\Component\HttpKernel\EventListener\RouterListener-&gt;onKernelRequest(Object, &#039;kernel.request&#039;, Object)
call_user_func(Array, Object, &#039;kernel.request&#039;, Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher-&gt;dispatch(&#039;kernel.request&#039;, Object) (Line: 127)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache-&gt;pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel-&gt;handle(Object, 1, 1) (Line: 665)
Drupal\Core\DrupalKernel-&gt;handle(Object) (Line: 19)
</pre>

Back to review for the patch in #26.

Seems there were some errors in #28 from what I can tell. Least the tests aren't green.