Add a toUriString method to Url class and add a route: scheme

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -248,13 +249,25 @@ public static function fromUri($uri, $options = array()) {
   +    // Extract query parameters and fragment and merge them into $options.
   ...
   +      $uri_query = [];
   

   "query parameters" vs. $uri_query — that's confusing. Let's be consistent.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -248,13 +249,25 @@ public static function fromUri($uri, $options = array()) {
   +    $options += ['fragment' => $uri_parts['fragment']];
   

   In HEAD, $options = [] by default. This causes $options to never be empty. I don't think we want that?

   Besides, we only set $options['query'] conditionally.

   So either set both $options['fragment'] and $options['query'] only when necessary, or set both always.

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -271,8 +284,10 @@ public static function fromUri($uri, $options = array()) {
   +   *   Parts from an URI of the form entity:{entity_type}/{entity_id}.
   
   @@ -280,38 +295,60 @@ public static function fromUri($uri, $options = array()) {
   +   *   Parts from an URI of the form user-path:{path}.
   

   Should mention that it expects an array of the shape returned by parse_url().

4. +++ b/core/lib/Drupal/Core/Url.php
   @@ -280,38 +295,60 @@ public static function fromUri($uri, $options = array()) {
   -    $uri_reference = explode(':', $uri, 2)[1];
   ...
   -      ->getUrlIfValidWithoutAccessCheck($uri_reference) ?: static::fromUri('base:' . $uri_reference);
   ...
   +      ->getUrlIfValidWithoutAccessCheck($uri_parts['path']) ?: static::fromUri('base:' . $uri_parts['path'], $options);
   

   This is a subtle change; we used to call PathValidator with an URI reference, which included querystring/fragment, now it doesn't anymore.

   I'm pretty sure that this is fine, but I just wanted to make sure this is what we want.

5. +++ b/core/lib/Drupal/Core/Url.php
   @@ -280,38 +295,60 @@ public static function fromUri($uri, $options = array()) {
   +   *   Parts from an URI of the form route:{path}. Where path is the route name
   +   *   optionally followed by a ";" followed by query parameters in key=value
   +   *   format with & separators.
   

   Hrm, the param docs here are much longer than for the other protected static helpers. It also explicitly mentions query parameters plus explains them. Why? And it's strange that it omits fragment; does that mean that route: URIs don't support fragments?

   Oh, I see, it's non-standard, it uses the semi-colon to indicate the query string instead of the question mark. Why?

   EDIT: oh, the docs are just wrong; this is for route parameters, not query parameters! Then it makes sense. But the docs definitely need to be fixed then :)

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

   Awesome test coverage! :) :)

7. +++ b/core/tests/Drupal/Tests/Core/UrlTest.php
   @@ -508,6 +520,55 @@ public function testInvalidEntityUriParameter() {
   +   * Tests the toUriString() method with entity: URI.
   ...
   +   * Tests the toUriString() method with user-path: URI.
   ...
   +   * Tests the toUriString() method with route: URI.
   

   Nit: s/URI/URIs/

Finally: do we call it "query parameters", "query arguments" or "querystring"? Let's go with whatever D8 uses elsewhere, and otherwise use the URI RFC's terminology.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -280,38 +295,60 @@ public static function fromUri($uri, $options = array()) {
   +   *   Parts from an URI of the form route:{path}. Where path is the route name
   

   This documentation is confusing ... what about using route:{route_name};{route_parameters} or something? {path} is certainly confusing in that domain.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -358,6 +395,25 @@ protected function setUnrouted() {
      /**
   +   * @return string
   +   *   A URI representation of the Url object data.
   +   */
   +  public function toUriString() {
   

   Given that this is a public method it should describe what it returns.

3. +++ b/core/lib/Drupal/Core/Url.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
   @@ -489,6 +489,18 @@ public function testEntityUris() {
   
   @@ -489,6 +489,18 @@ public function testEntityUris() {
        $url = Url::fromUri('entity:test_entity/1');
        $this->assertSame('entity.test_entity.canonical', $url->getRouteName());
        $this->assertEquals(['test_entity' => '1'], $url->getRouteParameters());
   +    // Ensure the options are parsed out.
   +    $url = Url::fromUri('entity:test_entity/2?page=1&foo=bar#bottom');
   +    $this->assertSame('entity.test_entity.canonical', $url->getRouteName());
   +    $this->assertEquals(['test_entity' => '2'], $url->getRouteParameters());
   +    $this->assertEquals($url->getOption('query'), ['page' => '1', 'foo' => 'bar']);
   +    $this->assertSame($url->getOption('fragment'), 'bottom');
   +    // Ensure the options are parsed out and merged with $options.
   +    $url = Url::fromUri('entity:test_entity/2?page=1&foo=bar#bottom', ['fragment' => 'top', 'query' => ['foo' => 'yes', 'focus' => 'no']]);
   +    $this->assertSame('entity.test_entity.canonical', $url->getRouteName());
   +    $this->assertEquals(['test_entity' => '2'], $url->getRouteParameters());
   +    $this->assertEquals($url->getOption('query'), ['page' => '1', 'foo' => 'yes', 'focus' => 'no']);
   +    $this->assertSame($url->getOption('fragment'), 'top');
      }
   

   ... mh, so previously we had just one single entry, in the test, can't we use a data provider here?

4. +++ b/core/tests/Drupal/Tests/Core/UrlTest.php
   @@ -508,6 +520,55 @@ public function testInvalidEntityUriParameter() {
   +   *
   +   * @covers ::toUriString
   +   */
   +  public function testToUriStringForEntity() {
   +    $url = Url::fromUri('entity:test_entity/1');
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1');
   +    $url = Url::fromUri('entity:test_entity/1', ['fragment' => 'top', 'query' => ['page' => '2']]);
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1?page=2#top');
   +    $url = Url::fromUri('entity:test_entity/1?page=2#top');
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1?page=2#top');
   +  }
   +
   ...
   +   * Tests the toUriString() method with user-path: URI.
   +   *
   +   * @covers ::toUriString
   +   */
   +  public function testToUriStringForUserPath() {
   +    $url = Url::fromRoute('entity.test_entity.canonical', ['test_entity' => '1']);
   +    $this->pathValidator->expects($this->any())
   +      ->method('getUrlIfValidWithoutAccessCheck')
   +      ->with('test-entity/1')
   +      ->willReturn($url);
   +    $url = Url::fromUri('user-path:test-entity/1');
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1');
   +    $url = Url::fromUri('user-path:test-entity/1', ['fragment' => 'top']);
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1#top');
   +    $url = Url::fromUri('user-path:test-entity/1', ['fragment' => 'top', 'query' => ['page' => '2']]);
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1?page=2#top');
   +    $url = Url::fromUri('user-path:test-entity/1?page=2#top');
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1?page=2#top');
   +  }
   +
   +  /**
   +   * Tests the toUriString() method with route: URI.
   +   *
   +   * @covers ::toUriString
   +   */
   +  public function testToUriStringForRoute() {
   +    $url = Url::fromUri('route:entity.test_entity.canonical;test_entity=1');
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1');
   +    $url = Url::fromUri('route:entity.test_entity.canonical;test_entity=1', ['fragment' => 'top', 'query' => ['page' => '2']]);
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1?page=2#top');
   +    $url = Url::fromUri('route:entity.test_entity.canonical;test_entity=1?page=2#top');
   +    $this->assertSame($url->toUriString(), 'route:entity.test_entity.canonical;test_entity=1?page=2#top');
   +  }
   +
   

   Data providers would be nice here as well.

This patch is really nice <3

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -240,21 +241,36 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +      $url = static::fromEntityUri($uri_parts, $uri_options, $uri);
   ...
   +      $url = static::fromUserPathUri($uri_parts, $uri_options);
   ...
   +      $url = static::fromRouteUri($uri_parts, $uri_options, $uri);
   

   +1. Now that the various schemes are unified, it makes sense to do this, avoiding the extra parse_url() call.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -280,38 +299,63 @@ public static function fromUri($uri, $options = array()) {
   +   *   Parts from an URI of the form user-path:{path}  as from parse_url().
   ...
   +   *   Parts from an URI of the form route:{path}  as from parse_url(). Where
   

   Nit: extra space.

+++ b/core/lib/Drupal/Core/Url.php
@@ -271,8 +288,11 @@ public static function fromUri($uri, $options = array()) {
-   * @param string $uri
-   *   An URI of the form entity:{entity_type}/{entity_id}.
+   * @param array $uri_parts
+   *   Parts from an URI of the form entity:{entity_type}/{entity_id} as from
+   *   parse_url().
+   * @param array $options
+   *   An array of options, see static::fromUri() for details.

@@ -280,38 +300,63 @@ public static function fromUri($uri, $options = array()) {
+  protected static function fromEntityUri(array $uri_parts, $options, $uri) {

One minor point to get picked up in the next re-roll...

1) $options should be type-hinted as array like $uri_parts is

2) we deleted the docs for $uri but it's still a parameter?

Beside of that change it looks great!

Found another small missing test coverage, great that this didn't slept through.

This time with an actual sane rebase.

Good catch!

So I was a bit concerned earlier on IRC about us introducing Yet Another Freaking Scheme™ here, and the DX impact on that. Basically, the rationale was that there are cases when you are intentionally resolving a link to a route, actually have all of the info you need already, and so re-resolving from some other scheme to this is wasted work. Also, from a DX POV you're mostly going to call Url::fromRoute() or use a Link field if you want to capture user input, same as Menu Link and Shortcut do, which handles all this whack-a-mole stuff scheme stuff for you. Fair enough.

Code-wise this is all pretty straight-forward, and nice catch on the additional test coverage.

Committed and pushed to 8.0.x, so we can continue to make progress on the _l()/_url() removal issues. w00t!

I think there's one actual bug in the code plus 3 nitpicks that aren't worth a follow-up unless the first is going to require a follow-up:

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -240,21 +241,37 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +    if (!empty($uri_parts['fragment'])) {
   +      $uri_options += ['fragment' => $uri_parts['fragment']];
   +    }
   +    unset($uri_parts['fragment']);
   

   Why is the unset() happening outside of the if-statement? That doesn't seem to make any sense?

   Before I file a follow-up for this tiny thing, I'd like confirmation from someone else that this is a problem.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -280,38 +302,68 @@ public static function fromUri($uri, $options = array()) {
   +  protected static function fromEntityUri(array $uri_parts, $options, $uri) {
   

   Nit: This $options didn't get typehinted.

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -358,6 +410,32 @@ protected function setUnrouted() {
   +   * Return a URI string that represents tha data in the Url object.
   

   Nit: Returns.

4. +++ b/core/lib/Drupal/Core/Url.php
   @@ -358,6 +410,32 @@ protected function setUnrouted() {
   +   * constructed using an entity: or user-path: scheme.  A user-path: URI
   +   * that does not match a Drupal route with be returned here with the base:
   +   * scheme, and external URLs will be returned in their original form.
   

   Nit: Double space after period, 80 col formatting a bit off.

Hadn't even considered that. Thanks for the follow-up!

The summary says:

This is needed to support Views token processing of paths, among other things

However, I have no idea where this requirement comes from. Views is doing token processing of paths more or less as it always has in #2409209: Replace all _url() calls beside the one in _l(), using just Views' internal API. I can see a case for refactoring that as it's scary, but I don't see why this new scheme is somehow necessary to support Views?

Furthermore, in general, tokenized Views paths will be just that, paths, entered by the user. So it would make sense to store them with user-path: or whatever we end up calling it.

`views.view.files.yml` is a system defined view with a token in it (one of the alter paths). That was the view started us down this road.

@mpdonadio, is that in scope for #2404603: Add proper support for Url objects in FieldPluginBase::renderAsLink(), so we can remove EntityInterface::getSystemPath()? Didn't see it in the patch there. I guess there's an attempt to make a distinction between what's usually user input (and user-alterable), but defined in default config, and what's actually entered by the user?