Add support for user-path: scheme to Url class

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -232,17 +241,24 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +      throw new \InvalidArgumentException(String::format('The URI "@uri" is invalid. You must use a valid URI scheme. Use base:// for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controlled by Drupal. Use user-path: for user input without a scheme.', ['@uri' => $uri]));
   

   Should we also document entity: ?

2. +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -115,8 +115,8 @@ function testMenu() {
   diff --git a/core/modules/system/system.routing.yml b/core/modules/system/system.routing.yml
   
   diff --git a/core/modules/system/system.routing.yml b/core/modules/system/system.routing.yml
   index 2bb9e62..f5e0e99 100644
   
   index 2bb9e62..f5e0e99 100644
   --- a/core/modules/system/system.routing.yml
   
   --- a/core/modules/system/system.routing.yml
   +++ b/core/modules/system/system.routing.yml
   
   +++ b/core/modules/system/system.routing.yml
   +++ b/core/modules/system/system.routing.yml
   @@ -383,7 +383,6 @@ system.theme_settings_theme:
   
   @@ -383,7 +383,6 @@ system.theme_settings_theme:
      options:
        _only_fragment: TRUE
    
   -
    '<current>':
      path: '<current>'
   

   out of scope for real

3. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestRedirectForm.php
   @@ -54,8 +54,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        // The destination is a random URL, so we can't use routed URLs.
            $form_state->setRedirectUrl(Url::fromUri('base://' . $form_state->getValue('destination')));
   

   ... yeah its important that we don't accidentally generate a routing URL.

4. +++ b/core/modules/views_ui/src/ViewListBuilder.php
   @@ -263,7 +263,9 @@ protected function getDisplayPaths(EntityInterface $view) {
   +          //   https://www.drupal.org/node/2364157 is available.
   

   Let's point to https://www.drupal.org/node/2409209

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -232,17 +241,24 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +    $uri_parts = parse_url($uri) + ['scheme' => NULL, 'path' => ''];
   +    if (empty($uri_parts['scheme'])) {
   

   Why bother adding NULL for scheme if we're going to check empty() anyway?

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -232,17 +241,24 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +      $url = \Drupal::pathValidator()
   

   The Drupal::service() calls for access manager, url generator, unrouted_url_assembler all have protected wrapper methods, can we add one for pathValidator too?

   In fact, using Drupal:: implies to me that these new changes don't have unit test coverage.

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -232,17 +241,24 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +      $url = \Drupal::pathValidator()
   +        ->getUrlIfValidWithoutAccessCheck($uri_parts['path']) ?: Url::fromUri('base://' . $uri_parts['path']);
   
   @@ -586,7 +602,11 @@ public function getInternalPath() {
   +      return $this->accessManager()
   +        ->checkNamedRoute($this->getRouteName(), $this->getRouteParameters(), $account);
   

   Why split these onto two lines? it's not THAT long...

4. +++ b/core/tests/Drupal/Tests/Core/Form/ConfirmFormHelperTest.php
   @@ -95,8 +96,20 @@ public function testCancelLinkRouteWithUrl() {
   +    $container_builder = new ContainerBuilder();
   +    $container_builder->set('path.validator', $path_validator);
   +    \Drupal::setContainer($container_builder);
   

   Ah, so we do have adjustments for this, but I would much rather avoid using ContainerBuilder and mock the helper.

Un-postponed this since https://www.drupal.org/node/2416763 landed in core.

Rerolling for base://.

Updated patch. The provided interdiff shows the changes from HEAD's Url::fromUri().

effulgentsia and I pair-reviewed this, we found the following problems, and fixed them all:

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -232,20 +241,33 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +        ->getUrlIfValidWithoutAccessCheck($uri_parts['path']) ?: Url::fromUri('base:' . $uri_parts['path']);
   

   This omits the querystring and fragment, if any.

2. +++ b/core/modules/dblog/src/Controller/DbLogController.php
   @@ -255,11 +255,11 @@ public function eventDetails($event_id) {
   +          $this->l($dblog->location, $dblog->location ? Url::fromUri('user-path:' . $dblog->location) : Url::fromRoute('<none>')),
   ...
   +          $this->l($dblog->referer, $dblog->referer ? Url::fromUri('user-path:' . $dblog->location) : Url::fromRoute('<none>')),
   

   Both are already absolute URLs, they don't need to be prefixed with a scheme.

   The second should be referer.

3. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestRedirectForm.php
   @@ -54,8 +54,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        // The destination is a random URL, so we can't use routed URLs.
            $form_state->setRedirectUrl(Url::fromUri('base:' . $form_state->getValue('destination')));
   

   The other 'destination' querystring occurrences use user-path:, so this one should too.

4. +++ b/core/modules/views_ui/src/ViewListBuilder.php
   @@ -263,7 +263,9 @@ protected function getDisplayPaths(EntityInterface $view) {
   +          // @todo Use the method on the executable when
   +          //   https://www.drupal.org/node/2417333 is available.
   

   Adding a @todo to the issue itself doesn't make any sense? :) Removed it.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -232,20 +241,29 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +      throw new \InvalidArgumentException(String::format('The URI "@uri" is invalid. You must use a valid URI scheme. Use base:// for items like a static file that needs the base path.. Use user-path: for user input without a scheme. Use entity: for referencing the canonical route of a content entity.', ['@uri' => $uri]));
   

   it should be base: and not two ".."

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -567,7 +606,10 @@ public function getInternalPath() {
       */
      public function access(AccountInterface $account = NULL) {
   -    return $this->accessManager()->checkNamedRoute($this->getRouteName(), $this->getRouteParameters(), $account);
   +    if ($this->isRouted()) {
   +      return $this->accessManager()->checkNamedRoute($this->getRouteName(), $this->getRouteParameters(), $account);
   +    }
   +    return TRUE;
      }
    
   

   Can we please add a test coverage for this particular bugfix?

YesCT also provided some additional in-person comment-only nitpicks, which have also been fixed in this reroll.

This reroll adds one test case and other than that only touches comments.

+++ b/core/lib/Drupal/Core/Url.php
index c055ab4..458e5ac 100644
--- a/core/tests/Drupal/Tests/Core/UrlTest.php

--- a/core/tests/Drupal/Tests/Core/UrlTest.php
+++ b/core/tests/Drupal/Tests/Core/UrlTest.php

+++ b/core/tests/Drupal/Tests/Core/UrlTest.php
+++ b/core/tests/Drupal/Tests/Core/UrlTest.php
@@ -432,9 +432,16 @@ public function testGetOptions($urls) {

@@ -432,9 +432,16 @@ public function testGetOptions($urls) {
    */
   public function testAccess($access) {
     $account = $this->getMock('Drupal\Core\Session\AccountInterface');
+
+    // Assert that routed URLs are access-checked by the access manager.
     $url = new TestUrl('entity.node.canonical', ['node' => 3]);
     $url->setAccessManager($this->getMockAccessManager($access, $account));
     $this->assertEquals($access, $url->access($account));
+
+    // Assert that unrouted URLs always have access.
+    $url = TestUrl::fromUri('base:kittens');
+    $this->assertTrue($url->access($account));
+
   }
 

It would be nice to have a testAccessForUnroutedUrls

Done!

Thank you!

There is an addition to the draft change record.

Don't see anything untoward here. Looks like it's cleaning up quite a few places where we were erroneously using base:// in the past. Onward!

Committed and pushed to 8.0.x. Thanks!

Did I stutter? ;)