Create standard logic to handle a entity: URI scheme

From dawehner, this should be trivial.

We will just resolve
entity://{entity_type}/{id}
to
Url::fromRoute('entity.{$entity_type}.canonical', [$entity_type => $id]);

Just want to point out that that won't work for config entities then, they don't have a canonical route.

Maybe the link template should at least optionally be supported as well? entity://node_type/article/edit-form ?

Seems fair IMHO.

Supporting entity://view/frontpage like suggested in #2407505-15: [meta] Finalize the menu links (and other user-entered paths) system might be tricky since that's not a link template, e.g. neither canonical nor edit-form.

A possible answer is to support only content entity types with this entity:// scheme since they usually have a canonical template, and then it will be as trivial as #3.

#6: did you see #4/#5? They discussed exactly that.

#8: Of course I saw that, my comment is a direct reply to them :) @Berdir asks if we should introduce link template support in the scheme and I say that it won't do any good since we still can't support the view/frontpage case.

We can't support view/name anyway, given that we would need to support both display ID and arguments.

Fixing nitpicks. Fixing tests.

I think we also want to validate the entity URI? That'd be the next step here.

#12+#13: gotcha.

Looks sane to me, thanks!

+++ b/core/lib/Drupal/Core/Url.php
@@ -285,7 +285,12 @@ public static function fromEntityUri($uri) {
+        throw new \InvalidArgumentException(String::format('The entity specified by "@uri" does not exist. You must specify the uri to an existing entity.', ['@uri' => $uri]));

+++ b/core/tests/Drupal/Tests/Core/UrlTest.php
@@ -448,12 +463,29 @@ public function accessProvider() {
+   * @expectedExceptionMessage The entity specified by "entity://test_entity/1" does not exist. You must specify the uri to an existing entity.

s/uri/URI/

OTOH, are there use cases where we may want to link to an entity that doesn't yet exist?

Good question. I'm inclined to say "no", because how could you then possibly already know the entity ID to use?

+++ b/core/lib/Drupal/Core/Url.php
@@ -267,6 +267,37 @@ public static function createFromRequest(Request $request) {
+      list($entity_type, $id) = explode('/', $entity_path);

What if $entity_type is null or wrong? We should also add a check for that and can we rename it to $entity_type_id? Same in the @param description.

+++ b/core/lib/Drupal/Core/Url.php
@@ -267,6 +267,38 @@ public static function createFromRequest(Request $request) {
+      if (\Drupal::entityManager()->getStorage($entity_type_id)->load($entity_id)) {
+        return new Url("entity.{$entity_type_id}.canonical", [$entity_type_id => $entity_id]);
+      }
+      else {
+        throw new \InvalidArgumentException(String::format('The entity specified by "@uri" does not exist. You must specify the URI to an existing entity.', ['@uri' => $uri]));<blockquote></blockquote>
+      }

Is loading the entity a performance concern at all? Maybe not given that we have the static cache and will probably need to do it anyway for access checking. So, if we're ok with loading the entity, how about instead of this, we just return $entity->urlInfo('canonical')?

A possible answer is to support only content entity types with this entity:// scheme since they usually have a canonical template

If we call urlInfo(), then we support any entity type that implements that properly. Looks like the 'view' entity type doesn't implement that, but perhaps we can open a separate issue for it to do so?

We can't support view/name anyway, given that we would need to support both display ID and arguments.

Could punt to a separate issue, but URIs support query strings, so we could extend the entity:// syntax for Views (and other config entities that require arguments, like Panel Pages) to something like entity://view/frontpage?display_id=page_1&arguments=... or whatever else makes sense.

Maybe the link template should at least optionally be supported as well? entity://node_type/article/edit-form ?

+1. Either in this issue or in a follow-up would be fine with me.

+++ b/core/lib/Drupal/Core/Url.php
@@ -267,6 +267,38 @@ public static function createFromRequest(Request $request) {
+  public static function fromEntityUri($uri) {

What about making this protected, and having fromUri() call it if the scheme (which it already parses) is 'entity'? I could see us adding more internal schemes (as well as refactoring our spread-out logic of the 'base' scheme), and that way we don't need to expose each scheme as a separate public method to the outside world.

AFAIK this blocks a critical and is thus critical.

I think we should just handle the canonical route for content entities for a start - anything else seems problematic. I think we should resolve others from the path.

+++ b/core/lib/Drupal/Core/Url.php
@@ -267,6 +267,38 @@ public static function createFromRequest(Request $request) {
+      if (\Drupal::entityManager()->getStorage($entity_type_id)->load($entity_id)) {
+        return new Url("entity.{$entity_type_id}.canonical", [$entity_type_id => $entity_id]);

Not to be difficult, but this hints of code smell.
Why does the Url object know this implementation detail? It seems like that is beyond its responsibilities.
The Url object is supposed to be a value object for detailing with Urls, are we sure we want it loading entities?

Moves the logic into fromUri and updates documentation.
Removes the loading, I think that is too much reach for a value object.
If we really want to validate the URL, we have processors for that right? And do we really want that? We don't stop someone from doing Url::fromRoute('entity.node.canonical', ['node' => 12]) if node/12 doesn't exist - how is this any different?
Similarly, what if someone stores this in a link field (using whatever widget we come up with) and then removes the node - do we want that to break that whole page from rendering? I wouldn't think so - I would think we'd want them to end up with a broken link - not a broken page.
Either way, if folks think loading and validating is what we want, I won't stand in the way.

Either way, if folks think loading and validating is what we want, I won't stand in the way

provided we do it in a processor ;)

   public static function fromUri($uri, $options = array()) {
-    if (!parse_url($uri, PHP_URL_SCHEME)) {
+    if (!($scheme = parse_url($uri, PHP_URL_SCHEME))) {
       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.', ['@uri' => $uri]));
     }
+    if ($scheme == 'entity') {
+      $entity_path = substr($uri, 9);

^ Please use parse_url() properly instead of, erm, this thing.

+      list($entity_type_id, $entity_id) = explode('/', $entity_path);

^ That needs a check on the number of exploded parts.

I really like this apporoch.

+++ b/core/lib/Drupal/Core/Url.php
@@ -225,9 +231,16 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
+      $entity_path = substr($uri, 9);

Can we use strlen('entity://') instead of 9?

What if I want to sore entity://node/1/edit?

This is fine to use
+ $entity_path = substr($uri, 9);
entity:// = 9 chars, so the hardcoded 9 would work here.
I have also added t() for multilingual site security purpose.

throw new \InvalidArgumentException(String::format(t('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.'), ['@uri' => $uri]));

Formatted and cleand the patch.

+++ b/core/lib/Drupal/Core/Url.php
@@ -225,13 +231,19 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   public static function fromUri($uri, $options = array()) {
-    if (!parse_url($uri, PHP_URL_SCHEME)) {
-      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.', ['@uri' => $uri]));
+    if (!($scheme = parse_url($uri, PHP_URL_SCHEME))) {
+      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.', ['@uri' => $uri]));
+    }
+    if ($scheme == 'entity') {
+      $entity_path = substr($uri, 9);
+      list($entity_type_id, $entity_id) = explode('/', $entity_path);
+      return new static("entity.$entity_type_id.canonical", [
+        $entity_type_id => $entity_id,
+      ]);
     }
 
     $url = new static($uri, array(), $options);
     $url->setUnrouted();
-
     return $url;
   }

I really think we should ensure that the canonical link template exists, and otherwise throw a exception ... note: Url->toString() will throw one as well, but it will be simple less easy to understand.

Just a little refactoring and addressing comments in #35 and partially in #26.

@Damien Tournoud (#26): I am not sure I like this handling of parse_url and check for entity as well, but can you suggest an alternative?

Fixing that embarrassing failure. :)

I don't think this is an acceptable solution, but hopefully, this will get it moving.

@almaudoh: I reviewed the patches earlier and see that there was an fromEntityUri method which was subsequently removed. I think, however, it is not exactly the same. The problem with the earlier method was that it actually checked for the entity itself which could be considered a bit too much. Here, we are just checking if the canonical link template exists.

Either way, I think it is still a good idea to move it to a different method. The relatively simple fromUri method suddenly started doing too much. For the record, I still don't like the condition to check for $scheme == 'entity' in there but I understand we probably can't handle entity:// the same way we are handling base:// (as the former is just a shorthand for writing the proper route, whereas the latter actually points to a resource).

Of course, I don't have enough experience to call the final shots on this, which is why we need reviews all around. :)

A slightly different and better approach.

Fixing the failure, and also moving the test method near other tests.

Adding the test as per #49.3. Other responses:

1. I think it seems that this is more like an example. In any case, I don't think an exception message should be treated like documentation.

2. I kind of agree. I am wondering if it makes sense to make the new fromEntityUri() method as public and remove the functionality from fromUri() completely. That way, each of these methods will do only one thing and it will be clear on what it does. Right now it seems that fromUri() method might be doing a lot more than makes sense.

4. Not really. The next line adds on to the $map array. We can, of course, combine both but I think it is better to leave it in separate lines.

This looks good. I was just trying to avoid another call to parse_url but of course, code clarity is better than micro-optimization, especially since this is not in any critical path as far as I can tell. :)

I see your points about fromUri() and fromEntityUri(). I did think of adding a method that would resolve the scheme and call the appropriate protected method (which is what fromUri() does). My main problem was that fromUri already did something else (handling unrouted URL's) and I did not want to change that. I think what we can do now is put in a new method handle unrouted URL's. That would be just a simple refactor and fromUri() would then be clean. Thoughts?

The other thing with the route validation is that it only validates the entity type. It doesn't validate the actual entity.

And I don't think it should, Url::fromRoute('entity.node.canonical', ['node' => 12]) doesn't check if node 12 exists, so this shouldn't either. Loading and validating entities is not the realm of a Url object.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -225,17 +232,51 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +    if ($scheme == 'entity') {
   

   Any reason not to use === here?

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -225,17 +232,51 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +      return self::fromEntityUri($uri);
   

   Should we use static instead of self? Someone might sub-class.

+++ b/core/lib/Drupal/Core/Url.php
@@ -225,17 +232,51 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
+    $routes = \Drupal::service('router.route_provider')
+      ->getRoutesByNames([$canonical_route]);
+    if (empty($routes)) {
+      throw new RouteNotFoundException(String::format('Could not find the canonical route "@canonical_route" for entity URL "@uri".',
+        ['@uri' => $uri, '@canonical_route' => $canonical_route]));
+    }

Was consideration given to using the ::toString() method (which does this exact check and throws the same Exception)?

$url = new $static($canonical_route, [$entity_type_id => $entity_id]);
// Validate the route exists, ::toString() will throw a RouteNotFoundException if the route does not exist.
$url->toString();
return $url

Those three lines remove the call to ->getRoutesByName and therefore prevent the Url object from having to know the workings of another object (the route provider), instead deferring it to the UrlGenerator; which it already knows about and is clearly in the same domain (Urls).

+++ b/core/lib/Drupal/Core/Url.php
@@ -225,17 +232,51 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
+    $entity_id = trim($uri_parts['path'], '/');

What if someone passes entity://node/1/edit?
$entity_id would end up being 1/edit
Note that the $url->toString() approach from my last comment should also validate that because the Route enforces a format on the entity ID slug.

Fixed #55, implemented #56 (though that required a bit of mocking, which means we're not actually testing what we claim to be testing…).

#58: indeed, you'll end up with something like this:
[Mon Jan 26 08:32:59 2015] [error] [client 127.0.0.1] Uncaught PHP Exception Symfony\\Component\\Routing\\Exception\\InvalidParameterException: "Parameter "node" for route "/node/{node}" must match "[^/]++" ("100/edit" given) to generate a corresponding URL." at /Users/wim.leers/Work/drupal-tres/core/vendor/symfony/routing/Symfony/Component/Routing/Generator/UrlGenerator.php line 167

What I think is concerning is that entity://node/such-epic-fail happily renders to /node/such-epic-fail. I can't believe this is intentional?

-   *   A new Url object for an unrouted (non-Drupal) URL.
+   *   A new Url object for an unrouted (non-Drupal) URL or a routed entity URI.

This indicates one DX regression: previously, Url::fromUri() was for unrouted URLs. You'd always use Url::fromRoute() for routed URLs. That's no longer the case. I don't see a better way, but it's worth pointing out.

+++ b/core/lib/Drupal/Core/Url.php
@@ -225,17 +232,51 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
+    $canonical_route = "entity.$entity_type_id.canonical";
+    $routes = \Drupal::service('router.route_provider')
+      ->getRoutesByNames([$canonical_route]);

MH I would have checked for the canonical link template instead. Is there are reason to not do that?

... Yes, my comment still applies to the latest patch.

@dawehner the thing with using link templates is you have to load the entity first and we're trying to avoid that. The Url value object should not know about entities.

This is not TRUE. I was thinking about this:

Fix typo.

To what extent do we want to unit test? maybe \Drupal::entityManager() should be injected or at least allowed to be set?

Is toString() the best way to validate a route exists? At the least, feels like that could be worth caching, or does the UrlGenerator at least have static caching?

I don't think Url objects should know about EntityManager objects.

$url->toString() will validate the route name and the parameter.

Not going to block progress because I realize this is critical to the menu link sprint, but hoping we can avoid coupling Url to entity manager.

Can we also have this store entity UUIDs instead of IDs?

@larowlan: I think the patch in #59 did not use EntityManager but called the toString() method. Is that what you mean?

@Dave Reid: Do you mean in the entity URI? Are you saying we should not use id's here but expect it to be an UUID? Our URI's would then be entity://{entity_type}/{uuid}.
I suppose it makes sense to use uuid's as this path may be exported as config. The question still remains if we should support id's as well.

@hussainweb

+++ b/core/lib/Drupal/Core/Url.php
@@ -236,6 +248,42 @@ public static function fromUri($uri, $options = array()) {
+    $entity_type = \Drupal::entityManager()->getDefinition($entity_type_id);
+    if (!$entity_type->hasLinkTemplate('canonical')) {
+      throw new \InvalidArgumentException(String::format('The entity type "@entity_type_id" does not have a "canonical" link template.', ['@entity_type_id' => $entity_type_id]));
+    }

This is still in the latest patch - that's the coupling I'm referring to

@larowlan: True. But I was referring to this in #59:

+++ b/core/lib/Drupal/Core/Url.php
@@ -236,6 +248,31 @@ public static function fromUri($uri, $options = array()) {
+    $url = new static("entity.$entity_type_id.canonical", [$entity_type_id => $entity_id]);
+    // Validate the route exists, ::toString() will throw a
+    // RouteNotFoundException if the route does not exist.
+    $url->toString();
+    return $url;
+  }

Is this the implementation to which you were referring?

Personally, I don't like the above too much. Without the comment, it is not at all clear why we are calling toString(). Even worse, if the behavior of toString() changes later on, e.g., to check if the entity actually exists, this would indirectly affect this (I think it is okay for an entity:// style URI to refer to an entity that does not (yet) exist).

On the other hand, I think coupling it with EntityManager is a bit too much out of it's job description. Between RouteProvider and EntityManager, I would pick RouteProvider as it makes some sense to the Url class at least.

On the other hand, I think coupling it with EntityManager is a bit too much out of it's job description. Between RouteProvider and EntityManager, I would pick RouteProvider as it makes some sense to the Url class at least.

Url is already coupled to the UrlGenerator, could we just use that instead? We have #1965074: Add cache wrapper to the UrlGenerator for performance sake.

Can we also have this store entity UUIDs instead of IDs?

+1

Otherwise menu links can potentially break when deployed.

Regarding UUID support ... this is a dedicated issue we should discuss ... #2353611: Make it possible to link to an entity by UUID ... there are various approach you go use.

This is still in the latest patch - that's the coupling I'm referring to

Well, I don't think that adding a coupling to the entity manager is any more coupling that adding support for entities in general.

In general I'm curious why

+  protected static function fromEntityUri($uri) {

is protected ... if people are explicit, I would allow them to be so.

Otherwise menu links can potentially break when deployed.

Entity reference fields store IDs, not uuids, so this is a general problem with content entities still.

If I create a node/1, how do I know what the uuid is to link to without checking in the database? entity://node/1 can be figured out from node/1 easily enough.

@dawehner: Regarding fromEntityUri, it was discussed in #50 and #19 and #22 before that. It was not very conclusive except for keeping fromUri() as an entry point that can work with various types of URI's.

I still think it is a good idea to make this public (we need to add checks) and have fromUri() call this. Further, we can refactor rest of the functionality of fromUri() into another method. That way, fromUri() is purely a facade to handle different types of URI and if you know which one you need for sure, you can directly use the specific method.

@catch

Entity reference fields store IDs, not uuids, so this is a general problem with content entities still

For defautl values ER stores target_uuid please see EntityReferenceFieldItemList::processDefaultValue() and EntityReferenceFieldItemList::defaultValuesFormSubmit()

#77: probably because that is stored in config, which requires you to use UUIDs. But the instances of a field are content, which has no such mandate.

re-roll of #64 for conflict in UrlTest.php

I agree with larowlan about the coupling - I think we should remove all the explicit validation in the new method and just let it fail at the time you try to call toString() I don't see what's gained by doing the work twice, other than potentially babysitting broken code.

If user input needs to be validated, this is not the right pace.

larolwan notes in IRC "we don't validate Url::fromRoute('entity.node.canonical', ['node' => 12]) to see if 12 exists"

So, the extra validation is inconsistent.

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -15,6 +15,7 @@
   +use Symfony\Component\Routing\Exception\RouteNotFoundException;
   

   Not used

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -236,6 +248,33 @@ public static function fromUri($uri, $options = array()) {
   +   * @throws \Symfony\Component\Routing\Exception\RouteNotFoundException
   +   *   Thrown when the entity's canonical route does not exist.
   

   Not anymore

3. +++ b/core/tests/Drupal/Tests/Core/UrlTest.php
   @@ -9,13 +9,16 @@
   +use Symfony\Component\Routing\Exception\RouteNotFoundException;
   
   @@ -51,6 +54,13 @@ class UrlTest extends UnitTestCase {
   +   * The route provider.
   +   *
   +   * @var \Drupal\Tests\Core\Routing\TestRouterInterface|\PHPUnit_Framework_MockObject_MockObject
   +   */
   +  protected $routeProvider;
   +
   +  /**
   
   @@ -90,8 +100,11 @@ protected function setUp() {
   +    $this->routeProvider = $this->getMock('Symfony\Cmf\Component\Routing\RouteProviderInterface');
   +
   ...
   +    $this->container->set('router.route_provider', $this->routeProvider);
   

   Not used either

Only documentation changes and deletes, as per #81 - this is RTBC

NW for IS update.

+++ b/core/lib/Drupal/Core/Url.php
@@ -236,6 +247,31 @@ public static function fromUri($uri, $options = array()) {
+    $entity_type_id = $uri_parts['host'];

Shouldn't we be validating $entity_type_id?

Yay, RTBC! Patch looks good.

I do still have one remaining concern: confusing DX.

Url::fromUri() still says this:

  /**
   * Creates a new Url object for a URI that does not have a Drupal route.
   *
   * This method is for generating URLs for URIs that do not have Drupal
   * routes, both external URLs and unrouted local URIs like
   * base://robots.txt. For URLs that have Drupal routes (that is, most pages
   * generated by Drupal), use Url::fromRoute().

I think that's misleading to say the least. We added a paragraph after the last quoted one above for entity URIs, but that makes it pretty buried. So, on the surface, it seems one must only ever use Url::fromUri() when dealing with "external to Drupal" links. That's in fact specifically how this was designed: always use ::fromRoute(), and in exceptional cases, i.e. when linking to "external to Drupal" stuff, use ::fromUri(). With this change, that's no longer true, and hence this is now very confusing.

That's also what I was getting at in #59. I think we should rephrase it to something clearer. Attached patch includes my suggestions.

And three nitpicks (all fixed in this reroll):

1. +++ b/core/lib/Drupal/Core/Url.php
   @@ -196,9 +196,15 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
   +   * For resolving URLs to an entity, you may use the
   

   It's not resolving, but generating.

2. +++ b/core/lib/Drupal/Core/Url.php
   @@ -236,6 +247,30 @@ public static function fromUri($uri, $options = array()) {
   +      throw new \InvalidArgumentException(String::format('The entity URI "@uri" is invalid. You must specify the entity id in the URL. e.g., entity://node/1 for loading the canonical path to node entity with id 1.',
   +        ['@uri' => $uri]));
   

   Weird code formatting.

3. +++ b/core/tests/Drupal/Tests/Core/UrlTest.php
   @@ -90,6 +93,7 @@ protected function setUp() {
        $this->router = $this->getMock('Drupal\Tests\Core\Routing\TestRouterInterface');
   +
        $this->container = new ContainerBuilder();
   

   Unnecessary whitespace change.

And one more thing: do we want entity:// or entity:? I.e. entity:node/1 or entity://node/1? We're so used to seeing ://, but I'm not sure we need/want it here. Quoting pwolanin at #2412509-16: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme:

Discussing directly with effulgentsia - maybe user-path:// would be a clearer option? We also discussed using user-path: (// is technically incorrect here and doesn't work as expected with parse_url() see http://en.wikipedia.org/wiki/URI_scheme) but that seems like a follow-up discussion.

I would have expected that PathValidator would understand entity://... but well, even people disagreed that we need support for that,
we could at least still add support for it later.

So, in person discussion with Wim, xjm, dawehner, effulgentsia, mpdonadio, others - let's use entity:

working on the patch.

It would be great explain in the issue summary why we went with that decision.

It's explained at #2407505-39: [meta] Finalize the menu links (and other user-entered paths) system.

Its fine for me.

I "blind-reviewed" this on IRC since Drupal.org was down at the time. Questions/comments were:

- The docs mention base:// but I read in some other issue we're moving to base: ... however, this is fine since it's documenting how HEAD currently works and the issue that renames it will rename these docs as well.
- Lee raised some concerns about the coupling of Url to EntityManager, however this no longer happens in the current patch.
- The format is entity:node/1 but there are lots of known problems of linking to serial IDs versus UUIDs. But I see xjm has cross-linked the issue that would make it possible to link to entity:node/{uuid} instead. Cool.

Otherwise, this has docs, tests, and looks like what we talked about. Great work everyone! Onward! :)

Committed and pushed to 8.0.x. Thanks!