Add a Url value object

1. +++ b/core/lib/Drupal/Core/Urlfactory.php
   @@ -0,0 +1,61 @@
   +      return new Url($result['_route'], $result['_raw_variables']->all());
   ...
   +
   ...
   +      // it from the container?
   ...
   +      // @todo Inject the url generator here instead of letting Url retrieve
   

   At least we could use $this->create

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,149 @@
   +    // @todo Refactor UrlGenerator to accept $this instead of three arguments?
   +    $this->urlGenerator()->generateFromRoute($this->routeName, $this->routeParameters, $this->routeOptions);
   

   We would need to extend the interface of urlGenerator, as this is something coming from symfony. Not sure whether it is worth to move even farther away fro the original interface.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,149 @@
   +    // @todo Refactor UrlGenerator to accept $this instead of three arguments?
   

   I wouldn't bother. It's hidden away from the user, so who cares?

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,149 @@
   +  public function toString() {
   

   This should either be __toString(), or a more semantically meaningful method. (Or, a more semantically meaningful method and then __toString() just sub-calls to that.)

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,149 @@
   +    if (!$this->urlGenerator) {
   +      $this->urlGenerator = \Drupal::urlGenerator();
   +    }
   +    return $this->urlGenerator;
   

   Inject this. The constructor in this setup is essentially private, so it's fine to inject it along with the data.

4. +++ b/core/lib/Drupal/Core/Urlfactory.php
   @@ -0,0 +1,61 @@
   +      // @todo Inject the url generator here instead of letting Url retrieve
   +      // it from the container?
   +      return new Url($result['_route'], $result['_raw_variables']->all());
   

   As above: Yes. :-)

5. +++ b/core/lib/Drupal/Core/Urlfactory.php
   @@ -0,0 +1,61 @@
   +    catch (\Exception $e) {
   +      // @todo Throw a helpful exception or something.
   +    }
   

   This should either catch nothing and let exceptions bubble, or catch everything and wrap it in a local exception, then throw that. I don't have a strong preference at the moment but I'm sure Mark does. ;-)

6. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
   @@ -547,8 +547,9 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
   -      if ($result = static::findRouteNameParameters($this->link_path)) {
   -        list($this->route_name, $this->route_parameters) = $result;
   +      if ($url = \Drupal::service('url_factory')->createFromPath($this->link_path)) {
   +        $this->route_name = $url->getRouteName();
   +        $this->route_parameters = $url->getRouteParameters();
   

   Shouldn't this be injected?

I don't find Url very intuitive as the class name (I almost like Tim's suggestion of UrlBag better) Something like UrlData?.

Part of the potential for confusion is that this is NOT the output of the UrlGenerator, for example.

in terms of DX when do I use a UrlFactory vs a UrlGenerator?

@pwolanin why not RouteData or RouteInfo?

What's the naming convention that defines that acronyms are treated as normal words and changed to "Url" instead of "URL"? (seriously asking, not being sarcastic)

I believe we settled on Url instead of URL, as that's somewhat more common.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -145,4 +144,19 @@ public function toArray() {
   +   */
   +  public function __sleep() {
   +    unset($this->urlGenerator);
   +    return array_keys(get_object_vars($this));
   +  }
   

   Why not serialize/unserialize? (I vaguely recall there being some reason DependencySerialization doesn't use that, but not why.)

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -127,7 +126,7 @@ public function getRouteOptions() {
   -  public function toString() {
   +  public function getPath() {
   

   Problem: What comes back is a URI. It could, depending on the options, be an absolute URI starting with http. Thus "getPath" is incorrect.

I almost like Tim's suggestion of UrlBag better

I was joking.

What's the naming convention that defines that acronyms are treated as normal words

https://drupal.org/node/608152#naming, number 3. We violate that in hundreds of places, but better to not add more. Also, see http://en.wikipedia.org/wiki/XMLHttpRequest :)

Let's link to that issue then, because otherwise people will ask why we're using PHP 4-era methods.

getString? getUri? render()? getUriString()?

getPath() is inaccurate, and toString() is too easily confused with __toString(). Otherwise I'm not wedded to a specific method name.

toString() is exactly what it is. That's what it should be called. There's no room for confusion…because it doesn't have a double underscore.

Every other name proposed makes the method's intent less clear.

Don't want to derail the issue further, but a static createFromRequest(Request $request) would be quite handy next to createFromPath($path).
(Spent an hour yesterday figuring out how to do that - and LinkGenerator::getActive() is a weird place for it)

Eh, call it toString(). Better to have something than to go nowhere with this issue.

Comments on IRC but figured I should share it here.

Seem like its a bad idea to make the assumption that URL's are only internal and defined by routes. If we're going to do something like this it seems like we should define a interface for a URL's and then describe how to internal and external url's implement that. Some interfaces may end up only working with internal URLs but that's on them to enforce.

Coming from #2141329: Replace getAdminPath() with getAdminRouteInfo() in Field UI.

In that issue, there was this piece of code:

'@url' => url($path)

which then got changed to this piece of code:

'@url' => $this->url($route['route_name'], $route['route_parameters'], $route['options'])

Which hurts my soul, as well as my eyeballs.

I'm told this issue could change that to:

'@url' => $this->url($route),

(This is not clear to me from the patch.)

If that's the case, however, I think the DX improvement here is significant enough to call this at least a beta target.

There has been quite a bit of discussion since #8 but I'm tagging this for reroll so we can have something to work from at least.

Not sure why this didn't go to the testbot (or maybe it did but the result didn't report back?)
Might be related to #2130059: Issue status change not consistently saved in node revisions or #2166935: Testbot not running. Should we just resubmit the same patch?

Status should trigger it hopefully.

Technically, '<front>' is the route name, and '/' is the link path. But we allow people to enter that, and need to handle it.

Previously, UrlMatcher caught all exceptions to prevent this from breaking, I think this special casing is better.

Also returning in toString() :)

I considered moving the exception handling into the static factory method, but @msonnabaum and @Crell counseled against it.

Also cleaned up the unit test.

For later: https://privatepaste.com/bf23059ff4

Okay, switched to using $router->match(), which means we don't have to create a new Request, or mess with _system_path.
I also removed the urlGenerator from the constructor, that would prevent people from making new Url objects very easily...

The change to UrlMatcher is copied from RouteProvider, and was pointed out by @Crell. That allows $router->match() to work.

I'm not sure why this change to menu_item_route_access() wasn't needed already...
The exact same pattern is in PathBasedBreadcrumbBuilder::getRequestForPath()

I also added some setters that I know we need, and added Url support to $form_state['redirect_route'] and did one sample conversion.

Well, there is an explicit check for that exception NOT being caught, so I'm not sure which way to go here.

Let's go this way.

Doing \Drupal::l($text, $url->getRouteName(), $url->getRouteParameters(), $url->getRouteOptions()); constantly is not fun, so let's add a code path to support \Drupal::l($text, $url)

I think this is as far as the patch should go, let's get it in!

Doh!

1) I'm not really sure *what* we want to do here, I assume we'd want to remove the alternate approach, but I think we should make this available right away.

2) We talked about this more in IRC and came up with UrlHelper. I renamed the alias in FormBuilder for now, and added a todo for renaming Url, if we want. I don't think this is in scope, that change would be big, and an extra API change.

This also hopefully addresses #22 (yched) and #24 (neclimdul).

While writing ExternalUrl, I decided to move the class down a namespace, and to rename getRouteOptions to getOptions, to match the named part of toArray() as well as making it applicable for ExternalUrl.

If no one likes the ExternalUrl approach, let's just push it to a follow-up.

Needed a reroll after #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit. No changes.

#22 / createFromRequest() : awesome :-)

The current logic around $this->active in LinkGenerator::setRequest() could probably be refactored to build on the new createFromRequest() rather than duplicating it ?

#49, that's actually dead code. I opened #2184649: Remove LinkGenerator::getActive() and LinkGenerator::setRequest() after talking to Wim.

#2184653: Rename Drupal\Component\Utility\Url to UrlHelper actually fixed the broken test, so I've included it for now.

Per @amateescu's suggestions in IRC, I folded the ExternalUrl class into Url, and moved it back out of the Routing namespace.

Changed our usage of Exceptions per an IRC discussion.

Also removed the UrlHelper changes, that can just happen in the other issue.

Really great work!

1. +++ b/core/lib/Drupal.php
   @@ -517,7 +518,13 @@ public static function linkGenerator() {
      public static function l($text, $route_name, array $parameters = array(), array $options = array()) {
   -    return static::$container->get('link_generator')->generate($text, $route_name, $parameters, $options);
   +    $link_generator = static::linkGenerator();
   +    if ($route_name instanceof Url) {
   +      return $link_generator->generateFromUrl($text, $route_name);
   +    }
   +    else {
   +      return $link_generator->generate($text, $route_name, $parameters, $options);
   +    }
      }
   

   I would have guessed that we could just do this functionality inside the link generator. Note: The providerBasedGenerator we use, does the same for SymfonyRoute objects, so it is kind of similar.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,352 @@
   +   * @throws \Symfony\Component\Routing\Exception\ResourceNotFoundException
   +   */
   

   Let's explain under which circumstances this exception is thrown.

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,352 @@
   +    // Special case the front page route.
   +    if ($path == '<front>') {
   +      $route_name = $path;
   +      $route_parameters = array();
   +    }
   +    else {
   +      // Look up the route name and parameters used for the given path.
   +      $result = \Drupal::service('router')->match('/' . $path);
   +      $route_name = $result[RouteObjectInterface::ROUTE_NAME];
   +      $route_parameters = $result['_raw_variables']->all();
   +    }
   

   It is sad that we actually need to make a workaround. I would have guessed that PathProcessorFront::processOutbound could have solved it for us automatically.

4. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,352 @@
   +    return static::createFromPath($path);
   ...
   +  public static function createFromRequest(Request $request) {
   

   Can't we use \Drupal::service('router')->matchRequest directly?

5. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,352 @@
   +  protected function setExternal() {
   

   I would have expected this to be public.

6. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,352 @@
   +  public function toString() {
   

   Have we considered using "toPath" instead?

7. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,352 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function __sleep() {
   +    unset($this->urlGenerator);
   +    return array_keys(get_object_vars($this));
   +  }
   

   Can't we just extend DependencySerialization ?

8. +++ b/core/lib/Drupal/Core/Utility/LinkGenerator.php
   @@ -99,80 +99,88 @@ public function getActive() {
   +  public function generate($text, $route_name, array $parameters = array(), array $options = array()) {
   +    $url = new Url($route_name, $parameters, $options);
   +    $url->setUrlGenerator($this->urlGenerator);
   

   It seems more clean to do it the other way around. Use generate as the basic method (given its similarity to the url generator) and use the generateFromUrl the special case.

9. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
   @@ -124,9 +126,12 @@ public static function preCreate(EntityStorageControllerInterface $storage_contr
    
   -    if ($route_info = \Drupal::service('router.matcher.final_matcher')->findRouteNameParameters($this->path->value)) {
   -      $this->setRouteName($route_info[0]);
   -      $this->setRouteParams($route_info[1]);
   +    try {
   +      $url = Url::createFromPath($this->path->value);
   +      $this->setRouteName($url->getRouteName());
   +      $this->setRouteParams($url->getRouteParameters());
   +    }
   +    catch (ResourceNotFoundException $e) {
        }
      }
   

   Is there a reason we previously did not had to catch the exception?

#54
1) I wanted to have typehinting and a dedicated method on LinkGenerator, while still letting \Drupal::l() serve both.

2) Done

3) We're moving this workaround out of MenuLink::preSave, which is a good thing. I don't really want to mess with path processing as part of this patch...

4) Good idea!

5) This is handled internally (no pun intended), and no one should be able to change the externality of the URL.

6) Yes, see #4, #8, #12, #16, and finally #18.

7) Yes

8) I disagree, mostly because I think the changes to hook_link_alter are an improvement.

9) Not sure, I'm going to remove it and see what fails. I think that the refactoring I've done since should obsolete this.

#55

1) Fixed
2) Fixed
3) Fixed
4) Fixed

5)
This should work (not actually using this test because its just testing UrlGenerator)

$url = \Drupal\Core\Url::createFromPath('http://drupal.org');
$url->setOption('fragment', 'foobar');
$this->assertSame('http://drupal.org#foobar', $url->toString());

6) I don't really care either way, and I know in other issues @damiankloip has explicitly asked for using methods. Take it up with him I guess :)

If we tried to write this test from the UI, it would fail. The API should fail as well. So let's fix the bug, and make the paths used by the test exist.

A few small comments, none of them terminal:

1. +++ b/core/lib/Drupal.php
   @@ -517,7 +518,13 @@ public static function linkGenerator() {
      public static function l($text, $route_name, array $parameters = array(), array $options = array()) {
   -    return static::$container->get('link_generator')->generate($text, $route_name, $parameters, $options);
   +    $link_generator = static::linkGenerator();
   +    if ($route_name instanceof Url) {
   +      return $link_generator->generateFromUrl($text, $route_name);
   +    }
   +    else {
   +      return $link_generator->generate($text, $route_name, $parameters, $options);
   +    }
   

   A parameter that could be a string or could be a full object that results in 2 different actions feels wrong to me. (I don't know if that was discussed above, I've not kept a close eye on the last several patches.) I don't want to hold up the entire patch on it, but if we can sort that out somehow that would be preferred.

2. +++ b/core/lib/Drupal/Core/Routing/NullGenerator.php
   @@ -12,7 +12,7 @@
   - * No-op implementation of a Url Generator, needed for backward compatibility.
   + * No-op implementation of a UrlGenerator, needed for backward compatibility.
   

   Scope creep! (Kidding!)

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,343 @@
   +   * @throws \Symfony\Component\Routing\Exception\ResourceNotFoundException
   

   This is a "foreign" exception (ie, from a different package). We ought to catch it, wrap it with one in our namespace, and rethrow it.

#60.1 and #60.2:
I removed a lot of changes I introduced while writing the patch. This has seen a lot of refactoring, and many of them turned out to be unnecessary.

#60.3, after discussion in IRC, I added \Drupal\Core\Routing\MatchingRouteNotFoundException.

I don't know how to better manage the scope of this patch. If we had just added the class and a test, it wouldn't have all of the methods it has now, because I didn't know we needed them.

I can rip out all of the actual usage of this if necessary... And put that in a follow-up.

Any particular reason why Drupal\Core\Url doesn't implement __toString() to complement the toString() method?

I don't think we should make __toString call the url generator, which I assume is what you suggest. If anything, it would be

public function __toString() {
  return var_export($this->toArray(), TRUE);
}

And that's not what you're suggesting.

Anyway, I removed all of the entity conversions. That needs to be its own issue, we just need this object available ASAP. No changes, no interdiff.

+++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
@@ -30,36 +30,16 @@ public function finalMatch(RouteCollection $collection, Request $request) {
+    if ($request->attributes->has('_system_path')) {
+      // _system_path never has leading or trailing slashes.
+      $path = '/' . $request->attributes->get('_system_path');
     }
-    catch (\Exception $e) {
-      return array();
+    else {
+      // getPathInfo() always has leading slash, and might or might not have a
+      // trailing slash.
+      $path = rtrim($request->getPathInfo(), '/');
     }
+    return $this->match($path);

This change is needed here, but is really a part of #2185831: Split up ParamConverterManager and stop throwing NotFoundHttpException.

admin/structure/taxonomy/manage/tags/fields is not an actual path anymore...

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -8,7 +8,8 @@
+use Drupal\Component\Utility\Url as UrlHelper;

Great idea, seriously!

This looks really nice now.

This is blocked by #2185831: Split up ParamConverterManager and stop throwing NotFoundHttpException, because it also makes the same change to UrlMatcher, but includes test coverage for it. I'll reroll after that goes in.

Leaving postponed, here's the reroll.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,386 @@
   +      try {
   +        $result = \Drupal::service('router')->match('/' . $path);
   +      }
   

   We've got the generator wrapped in a utility method. If we have to live with a global service call out for the router, too, it should also be wrapped (with a corresponding setter as well).

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,386 @@
   +  public function getOption($name) {
   +    if (!isset($this->options[$name])) {
   +      return NULL;
   +    }
   +
   +    return $this->options[$name];
   +  }
   

   The anal-retentive jerk in me would ask that be shortened to a ternary, because it's so easy to do. Not a commit blocker, though, just me being an anal-retentive jerk. :-)

Reroll! Should be quick re-RTBC.
Can someone help with the change record?

The anal-retentive jerk in me would ask that be shortened to a ternary, because it's so easy to do. Not a commit blocker, though, just me being an anal-retentive jerk. :-)

Note: a ternary might cost more and an URL object might be used in a lot of places.

I crossposted with Crell's review, let me address that:

1) These are static factory methods. There cannot be a setter, and it's not worth having a protected static function to wrap them.

2) Ternaries copy the values needlessly, we don't want to do that if we can help it.

Added a draft change record: https://drupal.org/node/2189619

Thanks for the change notice. Couple of minor issues.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -933,13 +934,17 @@ public function redirectForm($form_state) {
   +      // @todo Remove once all redirects are converted to Url.
   

   followup issue please.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -0,0 +1,386 @@
   +    // What was passed in as the route name is actually the path.
   +    $this->path = $this->routeName;
   ...
   +    // Set empty route name and parameters.
   +    $this->routeName = '';
   +    $this->routeParameters = array();
   

   Why we need this?

82.1) Opened #2189661: Replace $form_state['redirect_route'] with setRedirect()
82.2) If you call Url::createFromPath('http://drupal.org'), it will determine that it is an external URL, and this is how we handle it. The code you referred to is in the protected function setExternal. You cannot reach this code path via the constructor directly.

Thanks for the explanation. RTBC++

+++ b/core/lib/Drupal/Core/Url.php
@@ -0,0 +1,386 @@
+  /**
+   * Returns the route parameters.
+   *
+   * @return array
+   */
+  public function getRouteParameters() {
+    return $this->routeParameters;
+  }
+
+  /**
+   * Sets the route parameters.
+   *
+   * @param array $parameters
+   *   The array of parameters.
+   *
+   * @return $this
+   */
+  public function setRouteParameters($parameters) {
+    if ($this->isExternal()) {
+      throw new \Exception('External URLs do not have route parameters.');
+    }
+    $this->routeParameters = $parameters;
+    return $this;
+  }
+
+  /**
+   * Sets a specific route parameter.
+   *
+   * @param string $key
+   *   The key of the route parameter.
+   * @param mixed $value
+   *   The route parameter.
+   *
+   * @return $this
+   */
+  public function setRouteParameter($key, $value) {
+    if ($this->isExternal()) {
+      throw new \Exception('External URLs do not have route parameters.');
+    }
+    $this->routeParameters[$key] = $value;
+    return $this;
+  }
+

Should there be a getRouteParameter() method to complement setRouteParameter()?

There is no use case for needing the value for a single route parameter. You either need the whole set to pass along to something else, or you don't need them at all.

I purposefully left that out.

#2184649: Remove LinkGenerator::getActive() and LinkGenerator::setRequest() removed code right next to where we were adding it. Rerolled, no changes.

87: url-2153891-87.patch queued for re-testing.

Bot fluke, fixing status.

A beta blocker is blockered on this.

Committed 65c808b and pushed to 8.x. Thanks!

Let's publish the change record!

Not both.

And wow, that d.o bug is nasty.

shouldnt the change record be published now? or there are other issues required for that?

Finally opened the follow-up I mentioned in #66 #2215961: Entity::urlInfo() should return \Drupal\Core\Url