Make it possible to link to an entity by UUID

Absolutely lovely idea, I do not even know why that's not the default :)

Cool ... that would invalidate #2304849: Stop excluding local ID and revision fields from HAL output too.

The proposal in the summary sounds great: A 301 is the right way to do this.

However, it looks like the patch is not actually setting that up yet. IMO we should put the uuid version at a different URI than the auto-inc version. In fact, it doesn't even need to be in REST. Any entity with a canonical URI can also get a /uuid/{type}/{uuid} URI that does nothing but send a redirect.

Checking #6 and remarks from #8 this needs work.

Is #6 supposed to be a patch maybe?

first a reroll, then tackling #10

this died horribly because OutboundRouteProcessorInterface changed, and invalid yml, guess that explains #6 being a txt file :)

working on this a bit

Wow Drupal\menu_ui\Tests\MenuTest is a big one, 7 mins per run locally.

Added new integration tests to it covering the new behaviour, made sure that menu links pointing to nodes by UUID are picked up by the node edit form alters in menu_ui module.

There is no redirect because the path processor lets you use serial ID (nid) or UUID. Redirect would be the job of global redirect module (we don't redirect node/2 to /some/pretty/path in core).

Had to do some refactoring in the menu tree manipulator because sometimes the node parameter is a uuid.

until tomorrow in case anyone else is inclined

It is great to see this functionality being incorporated into D8. This is something the D7 contrib module already does.

Updating patch from #15.

The last patch is broken.

FWIW, I've been working on this against the 8.0.x branch (so this might not a be a problem on the 8.1.x branch) and I'm having some issues getting past a route exception:

Symfony\Component\HttpKernel\Exception\MethodNotAllowedHttpException: No route found for "GET /node/cf4048f3-51b3-4a9a-8918-969929c175d8": Method Not Allowed (Allow: DELETE, PATCH) in Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest() (line 164 of /var/www/site/vendor/symfony/http-kernel/EventListener/RouterListener.php).

This patch is against 8.0.x. It's basically #15 plus some hacks to make it work, and I removed the tests. I'm not setting this to issue "needs review" but maybe the patch is useful - the comments below may not even apply to 8.1.x.

So, firstly the route expects an integer so using node/{uuid} is rejected before it gets to RouteProcessorEntityUUID.php. See ->setRequirement('node', '\d+') in NodeRouteProvider.php

Also in the patch in #15, the /node/{uuid} has no logic for redirecting to /node/{id} - I think this would be mandatory. I worked around this with a redirect in the NodeViewController. I don't know the best fix for this.

I'm not sure what the menu-related changes in #15 patch do, I left them in... perhaps they are only relevant if we don't do a redirect?

Modified description to reflect the recent patches.

Yeah, will try and catch you tomorrow

Another option here is to add a new link template to go with the 301 redirect.

$entity->toUrl('uuid');

Discussed with @dawehner - I'm going to do the following

• Update to use inbound path processor instead
• Update tests
• Profile

Working on this

Interdiff is against #22

Let's see what fails here.

1. +++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorEntityUuid.php
   @@ -0,0 +1,58 @@
   +        if ($entity_type->hasLinkTemplate('canonical') && $entities = $storage->loadByProperties(['uuid' => $uuid])) {
   

   Not sure but might be worth injecting EntityRepository for loadEntityByUuid here

2. +++ b/core/modules/menu_ui/menu_ui.module
   +++ b/core/modules/menu_ui/menu_ui.module
   @@ -201,8 +201,12 @@ function menu_ui_get_menu_link_defaults(NodeInterface $node) {
   
   @@ -201,8 +201,12 @@ function menu_ui_get_menu_link_defaults(NodeInterface $node) {
   +      $query = \Drupal::entityQuery('menu_link_content');
   

   Does this need an upgrade path?

@dawehner so we'd add that to \Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider for any entity-type with a canonical link template?

Makes sense to me.

Would it be worth adding a uuid link-template to go with it, that'd make it easier to use - instead of internal:/{entity_type}/{uuid} we could use $entity->toUrl('uuid');

Sound about right?

Would it be worth adding a uuid link-template to go with it

+1

internal:/{entity_type}/{uuid}

Why not entity:{entity type}/{entity uuid}?

We need a uuid index, this is something we have added as part of Multiversion.

I am proposing this for Entity module (#2690745: Create an index of UUIDs) and core (#2690747: [PLAN] Create an index of UUIDs), essentially it's just a big key_value store keyed by UUID and stores entity type id, entity id, and potentially other things.

Then, as we do in Multiversion and other modules that depend on it, we query the index with just the UUID and we can load the entity from the entity type manager.

@timmillwood whilst I agree a uuid index is useful, I don't think we need it here, we have the entity_type information.

@larowlan - Right, this issue introduces incoming paths matching /{entity_type_id}/{uuid}, but wouldn't it be cool to introduce Incoming paths matching /{uuid}. In that case we would need a UUID index.

In Relaxed module we use paths with the uuid but not the entity type, therefore we make use of the uuid index in Multiversion.

working on this

current patch, some failing tests at this stage, will continue on tuesday

More work on failing tests

More work on failing tests

more fixes

1. Yep
2. Yep
3. Yeah, in case contrib has an entity type with a uuid and a canonical display, but no UUID route (e.g. taxonomy in core is a case of this too). Even if they have a route provider we can't be sure that the route provider provides a UUID route - this interface allows us to do that - see the hunk in EntityManager - so yeah makes 1 more important (more tests)
4. Yep
5. Yep
6. Yep

Just confirming this applies and works on 8.1.0 as well

Fixes #53

+++ b/core/lib/Drupal/Core/Entity/EntityTypeManager.php
@@ -83,6 +84,12 @@ public function processDefinition(&$definition, $plugin_id) {
+      $definition->setLinkTemplate('uuid', "/{$plugin_id}/{{$plugin_id}}");

Do we need an update hook to clear the cache for this?

1. +++ b/core/lib/Drupal/Core/Entity/EntityTypeManager.php
   @@ -83,6 +84,12 @@ public function processDefinition(&$definition, $plugin_id) {
   +    // Add the UUID link template if applicable.
   +    $route_providers = $definition->getRouteProviderClasses();
   +    $has_uuid_route = isset($route_providers['html']) && in_array(EntityUuidRouteProviderInterface::class, class_implements($route_providers['html']), TRUE);
   +    if (!$definition->hasLinkTemplate('uuid') && $definition->hasKey('uuid') && $definition->hasViewBuilderClass() && $has_uuid_route) {
   +      $definition->setLinkTemplate('uuid', "/{$plugin_id}/{{$plugin_id}}");
   +    }
   
   +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -23,12 +24,13 @@
   + * - uuid
   

   This makes so much sense!

2. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -83,6 +85,12 @@ public function getRoutes(EntityTypeInterface $entity_type) {
   +    // This goes before canonical so that the UUID pattern can be tested
   +    // before non-integer entity IDs.
   

   *can* or *must*?

3. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -230,6 +238,34 @@ protected function getCanonicalRoute(EntityTypeInterface $entity_type) {
   +    if ($entity_type->getKey('uuid') && $entity_type->hasViewBuilderClass()) {
   

   Is this correct? What about file entities? They don't have a view builder, but can be referenced by UUID.

4. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -230,6 +238,34 @@ protected function getCanonicalRoute(EntityTypeInterface $entity_type) {
   +          '_title_callback' => '\Drupal\Core\Entity\Controller\EntityController::title',
   

   do we want this one or EntityViewController::buildTitle()? (In-place editing needs that one. I didn't even realize we also had this one.)

5. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -135,7 +135,10 @@ public function checkNodeAccess(array $tree) {
   +        ->condition('nid', $nids, 'IN')
   +        ->condition('uuid', $nids, 'IN');
   
   @@ -150,10 +153,13 @@ public function checkNodeAccess(array $tree) {
   +      $nids = (array) $query->execute();
   

   The $nids variable name no longer makes sense with this.

   Also, we need to have updated test coverage for this.

6. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -57,11 +58,19 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +        if ($entities = $storage->loadByProperties(['uuid' => $value])) {
   

   Why not use \Drupal\Core\Entity\EntityRepository::loadEntityByUuid()?

7. +++ b/core/lib/Drupal/Core/Url.php
   @@ -336,7 +337,11 @@ public static function fromUri($uri, $options = []) {
   +      return new static("entity.$entity_type_id.uuid", [$entity_type_id => $entity_id], $options);
   

   This looks confusing.

8. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -201,8 +201,13 @@ function menu_ui_get_menu_link_defaults(NodeInterface $node) {
   -      $query = \Drupal::entityQuery('menu_link_content')
   -        ->condition('link.uri', 'node/' . $node->id())
   +      $query = \Drupal::entityQuery('menu_link_content');
   +      $group = $query->orConditionGroup()
   +        ->condition('link.uri', 'entity:node/' . $node->id())
   +        ->condition('link.uri', 'entity:node/' . $node->uuid())
   +        ->condition('link.uri', 'internal:/node/' . $node->id())
   +        ->condition('link.uri', 'internal:/node/' . $node->uuid());
   +      $query->condition($group)
   

   This looks like a straight bugfix? Nice!

   Do we have updated test coverage for this?

9. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
   @@ -99,6 +100,14 @@ protected function setUpEntityTypeDefinitions($definitions = []) {
   +        // Don't allow uuid key.
   
   @@ -300,6 +309,10 @@ public function testGetRouteProviders() {
   +    // Don't allow uuid key.
   
   @@ -311,6 +324,27 @@ public function testGetRouteProviders() {
   +    // Allow uuid key.
   

   Nit: s/uuid/UUID/

#60 they can, but they also have to add the uuid template to the entity-type annotation.

I can do taxonomy term in this patch to prove it works if that helps?

Addressing comments:

1. #57- will add
2. #58.1, #63 - So how about I remove the 'automagic uuid link template' feature and just add it to all the entity annotations that have a uuid route - manual or from a route provider? Then that hunk/interface can go, as can all changes to EntityManager/EntityManagerTest
3. #59.2 - will fix
4. #59.3 - this is the same logic for canonical paths, File entity will need to do it's own thing
5. #59.4 - again this mimics the existing canonical path
6. #59.5 will fix
7. #59.6 can do - but would mean another dependency - thoughts?
8. #59.7 any suggestions on how to address?
9. #59.8 I think I added some, will confirm
10. #59.9 will fix

So need some feedback from @WimLeers on #59.6 and 7 and from @dawehner on removing the magic UUID link template.

#64:

• #59.6: I think this is a fine dependency, because it provides the exact functionality you need. No need to duplicate the same logic.
• #59.7: Current:
  

    if (Uuid::isValid($entity_id)) {
      // UUID instead of entity ID.
      return new static("entity.$entity_type_id.uuid", [$entity_type_id => $entity_id], $options);
    }
   
    return new static("entity.$entity_type_id.canonical", [$entity_type_id => $entity_id], $options);
  

  What I think is less confusing:

    $route_name = Uuid::isValid($entity_id) ? "entity.$entity_type_id.uuid" : "entity.$entity_type_id.canonical";
    return new static($route_name, [$entity_type_id => $entity_id], $options);
  

  I think this is less confusing because it clearly shows that the route parameters are the same in both cases, it's just a different route name.

Addressing items from #64

1. #57- fixed
2. #58.1, #63 - removed magic
3. #59.2 - fixed
4. #59.3 - nothing to do
5. #59.4 - nothing to do
6. #59.5 - fixed
7. #59.6 - this changed the constructor and there were several classes extending from it that were calling the parent constructor including some in contrib (search api and workbench moderation) so changing this would be an API break, so reverted my changes
8. #59.7 - fixed
9. #59.8 - added
10. #59.9 - that hunk removed as part of magic rollback

Other then this small feedback I think it is ready. Can you please also fix all the needs tag?

+++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
@@ -230,6 +238,34 @@ protected function getCanonicalRoute(EntityTypeInterface $entity_type) {
+      $route = new Route("/{$entity_type_id}/{{$entity_type_id}}");
...
+          '_entity_view' => "{$entity_type_id}.full",
...
+        ->setRequirement('_entity_access', "{$entity_type_id}.view")

These double quotes are making it hard to understand. Can we add comment or use single quotes instead?

#68 - @dawehner not sure what you mean there
##6 - will fix

Draft change notice is https://www.drupal.org/node/2725969

Fixes #69

Note this would also help make linking in formatted text fields better, because the content would actually be syncable across sites. Hence this is also related to #2292159: EditorLinkDialog should validate URLs, and autocomplete like the Link widget.

1. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
   @@ -23,6 +24,7 @@
   + * - uuid
   

   This is not listed at https://www.iana.org/assignments/link-relations/link-relations.xhtml, is that a problem?

2. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   --- a/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   

   Nit: The docblock for this class says:

   * Parameter converter for upcasting entity IDs to full objects.
   

   Needs to be updated.

3. +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -221,6 +222,14 @@ function addCustomMenu() {
   +    // Make this menu available for node-edit forms.
   +    /* @var \Drupal\node\NodeTypeInterface $type */
   +    $type = NodeType::load('article');
   +    $node_menus = $type->getThirdPartySetting('menu_ui', 'available_menus', array('main'));
   +    $node_menus[] = $menu_name;
   +    $type->setThirdPartySetting('menu_ui', 'available_menus', $node_menus);
   +    $type->save();
   
   @@ -466,6 +475,29 @@ function doMenuTests() {
   +    $this->drupalGet($node6->url('edit-form'));
   +    $this->assertFieldByName('menu[title]', $uuid_link->label());
   +    $this->drupalPostForm(NULL, [], t('Save'));
   ...
   +    $this->drupalGet($node7->url('edit-form'));
   +    $this->assertFieldByName('menu[title]', $uuid_link->label());
   +    $this->drupalPostForm(NULL, [], t('Save'));
   

   This causes this to be testing the Node UI, rather than the Menu UI. I think it's only necessary for the assertions I mentioned. And do we really need those here?

4. +++ b/core/modules/system/system.install
   @@ -1652,3 +1652,19 @@ function system_update_8014() {
   + * The simple presence of this update function clears cached field definitions.
   

   I don't think this comment is accurate? Looks like a c/p thing.

EDIT: forgot to say: I was very tempted to RTBC this. It's mostly because of the test that I didn't. This looks 99% ready.

#73.1 - I thought so too, but then I looked up all of our existing link-templates and found we're already using some that don't exist - e.g. delete-form. In the existing templates, I only see duplicate and alternate as possibles but I don't think that would lead to the best DX.
In my opinion $entity->toUrl('duplicate'); isn't as obvious as $entity->toUrl('uuid'); - thoughts?
#73.2 - fixed
#73.3 - yeah I changed the logic around that form alter, so we need to test that we can also load the default value if it references a node by UUID
#73.4 - fixed

1. If delete-form also doesn't exist, then this is fine. I agree duplicate would be a meaningless and even confusing relation name.
2. Yay
3. Ok, but is this the right place to test that?
4. Yay

So only point 3 needs further attention.

re #75.3 will move to MenuNodeTest from MenuTest and push the ::verifyMenuLink and ::addMenuLink methods up into MenuWebTestBase, making that the new base-class for MenuNodeTest.

Done

That was my only remaining concern. Thanks! :)

As far as I can tell, the latest patch does not actually use the URI link template in the route provider.

@tstoeckler can you clarify what you mean there?

+++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
@@ -83,6 +85,12 @@ public function getRoutes(EntityTypeInterface $entity_type) {
+    if ($uuid_route = $this->getUuidRoute($entity_type)) {
+      $collection->add("entity.{$entity_type_id}.uuid", $uuid_route);
+    }

@@ -230,6 +238,34 @@ protected function getCanonicalRoute(EntityTypeInterface $entity_type) {
+  protected function getUuidRoute(EntityTypeInterface $entity_type) {

@tstoeckler - this is where it is used

Sorry for being so terse. What I meant was this:

In \Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider::getUuidRoute():

This

+    if ($entity_type->getKey('uuid') && $entity_type->hasViewBuilderClass()) {
+      $entity_type_id = $entity_type->id();
+      $route = new Route('/' . $entity_type_id . '/{' . $entity_type_id . '}');

should be

+    if ($entity_type->hasLinkTemplate('uuid') && $entity_type->getKey('uuid') && $entity_type->hasViewBuilderClass()) {
+      $entity_type_id = $entity_type->id();
+      $route = new Route($entity_type->getLinkTemplate('uuid'));

I.e. we should check whether the entity type specified a UUID link template and then use it to fetch the path for the route.

Hope that makes it clearer what I'm thinking.

Nice one @tstoeckler - fixed

@tstoeckler++

1. Looks like #82 could do with a test - no? Also don't we need to check for the case when there is no uuid link template as shown by @tstoeckler

2. diff --git a/core/modules/menu_ui/src/Tests/MenuTest.php b/core/modules/menu_ui/src/Tests/MenuTest.php
   index ecd28ca..016cca7 100644
   --- a/core/modules/menu_ui/src/Tests/MenuTest.php
   +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -8,7 +8,6 @@
    use Drupal\Core\Menu\MenuLinkInterface;
    use Drupal\Core\Url;
    use Drupal\menu_link_content\Entity\MenuLinkContent;
   -use Drupal\node\Entity\NodeType;
    use Drupal\system\Entity\Menu;
    use Drupal\node\Entity\Node;
    
   

   Unused use.

3. +++ b/core/lib/Drupal/Core/Url.php
   @@ -336,10 +337,15 @@ public static function fromUri($uri, $options = []) {
   -      throw new \InvalidArgumentException("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.");
   +      throw new \InvalidArgumentException("The entity URI '$uri' is invalid. You must specify the entity id in the URL. e.g., entity:node/1 or entity:node/d44a0040-2844-4cca-b0b5-20c6c96c4d8c for loading the canonical path to node entity with id 1.");
   

   Hmmm well the eg uuid doesn't really work does it.

4. +++ b/core/modules/system/system.install
   @@ -1652,3 +1652,19 @@ function system_update_8014() {
   +function system_update_8015() {
   

   Should be system_update_8200() since we need to leave a gap in case of an emergency update function. At last that is what @catch made a recent views patch do.

First, rewinding head to replay your work on top of it...
Applying: Init commit
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/Entity/Entity.php
Falling back to patching base and 3-way merge...
Auto-merging core/lib/Drupal/Core/Entity/Entity.php

git diff 8.2.x HEAD core/lib/Drupal/Core/Entity/Entity.php

diff --git a/core/lib/Drupal/Core/Entity/Entity.php b/core/lib/Drupal/Core/Entity/Entity.php
index e89ce31..403945d 100644
--- a/core/lib/Drupal/Core/Entity/Entity.php
+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -309,6 +309,9 @@ protected function urlRouteParameters($rel) {
     if ($rel === 'revision' && $this instanceof RevisionableInterface) {
       $uri_route_parameters[$this->getEntityTypeId() . '_revision'] = $this->getRevisionId();
     }
+    if ($rel === 'uuid') {
+      $uri_route_parameters[$this->getEntityTypeId()] = $this->uuid();
+    }

     return $uri_route_parameters;
   }

Committed 85bc69d and pushed to 8.2.x. Thanks!

I'm sorry but since commit MenuNodeTest fails on PostgreSQL for 8.2.x. Please see https://dispatcher.drupalci.org/job/php5.5_postgres9.1/

Let's fix this in #2762413: Drupal\menu_ui\Tests\MenuNodeTest is failing on automatic testing for PostgreSQL rather than re-opening this issue.

After discussing with @catch decided to revert- since this introduces a postgres regression and increases the likelihood of adding more.

Thanks @alexpott! Just found a minute to ran this locally and here is the PostgreSQL log:

ERROR:  invalid input syntax for integer: "d0baf2a6-48eb-4e09-919e-32bc4a69f55c" at character 391
STATEMENT:  SELECT base_table.vid AS vid, base_table.nid AS nid
	FROM 
	simpletest666842node base_table
	LEFT JOIN simpletest666842node_field_data node_field_data ON node_field_data.nid = base_table.nid
	LEFT JOIN simpletest666842node node ON node.nid = base_table.nid
	INNER JOIN simpletest666842node_field_data node_field_data_2 ON node_field_data_2.nid = base_table.nid
	WHERE ( (node_field_data.nid IN  ('d0baf2a6-48eb-4e09-919e-32bc4a69f55c')) or (LOWER(node.uuid) IN (LOWER('d0baf2a6-48eb-4e09-919e-32bc4a69f55c'))) )AND (node_field_data_2.status = '1')

PostgreSQL is a bit more strict and the IN nid condition seems obsolete ;)

Trying to fix the PostgreSQL test bug.

Second try to fix the PostgreSQL test bug.

Discussed with @catch, @xjm, @effulgentsia, @cottser. We felt that this issue is a beta target because it has been committed once before and is only help up because postgres fails and it is an important feature for some of the API first work.

The issue summary could do with an update to explain the issue, the benefits and the solution.

This should fix the problem - also I'm not convinced the original patch actually worked because of how \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkNodeAccess() was adding the access result to the tree.

Here's an integration test for the \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkNodeAccess(). I'm really sure why this exists - it is only used in \Drupal\Core\Menu\MenuParentFormSelector and it's not part of \Drupal\system\Plugin\Block\SystemMenuBlock::build() which creates the menus people see on the page.

Discussed a bit with @dawehner - so \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkNodeAccess() exists for performance reasons (as it documents in its doc block). The performance fix is to not load nodes so the fix in #107 loses that benefit for UUIDs. We have three options here:

1. To only make menu links benefit from this when they are linked by nid
2. Make \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators::checkNodeAccess() handle both
3. Add a new manipulator to \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators for UUIDs.

The current approach I've chosen is 2 but that's because I started from #107 - maybe it is not the best option.

@dawehner's review in #108 still needs to be done. To be honest I don't think #108.6 is particularly fair. The existing test is full of $this->at() already and in general I'm not a fan of mocking complex queries.

(Removing release notes tag; we can retag for whichever release notes whenever the change is committed.)

Working on #108

108.1 was handled in 109
108.2 -fixed comment
108.3 -fixed
108.4 -fixed and added a new test
108.5 -added back

\Drupal\Core\Entity\EntityStorageBase::loadByProperties uses an IN query by default, so we can pass the array of UUIDs and use loadMultiple instead of looping.

1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -57,11 +58,19 @@ public function __construct(EntityManagerInterface $entity_manager) {
          $entity = $storage->load($value);
   +      // If there is no entity loadable by ID, try to load by UUID.
   +      if (!$entity && Uuid::isValid($value)) {
   +        if ($entities = $storage->loadByProperties(['uuid' => $value])) {
   +          $entity = reset($entities);
   +        }
   +      }
   

   We could use the ctype_digit() || is_int() trick to potentially save a query here. In fact we could add these checks to Uuid::isValid() to make it cheaper.

2. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -201,8 +201,13 @@ function menu_ui_get_menu_link_defaults(NodeInterface $node) {
   -      $query = \Drupal::entityQuery('menu_link_content')
   -        ->condition('link.uri', 'node/' . $node->id())
   +      $query = \Drupal::entityQuery('menu_link_content');
   +      $group = $query->orConditionGroup()
   +        ->condition('link.uri', 'entity:node/' . $node->id())
   +        ->condition('link.uri', 'entity:node/' . $node->uuid())
   +        ->condition('link.uri', 'internal:/node/' . $node->id())
   +        ->condition('link.uri', 'internal:/node/' . $node->uuid());
   +      $query->condition($group)
   
   @@ -212,8 +217,13 @@ function menu_ui_get_menu_link_defaults(NodeInterface $node) {
   -      $query = \Drupal::entityQuery('menu_link_content')
   +      $query = \Drupal::entityQuery('menu_link_content');
   +      $group = $query->orConditionGroup()
            ->condition('link.uri', 'entity:node/' . $node->id())
   +        ->condition('link.uri', 'entity:node/' . $node->uuid())
   +        ->condition('link.uri', 'internal:/node/' . $node->id())
   +        ->condition('link.uri', 'internal:/node/' . $node->uuid());
   +      $query->condition($group)
   

   Is this fixing a bug?

1 fixed
2 this is from HEAD

/**
 * Helper function to create or update a menu link for a node.
 *
 * @param \Drupal\node\NodeInterface $node
 *   Node entity.
 * @param array $values
 *   Values for the menu link.
 */
function _menu_ui_node_save(NodeInterface $node, array $values) {
  /** @var \Drupal\menu_link_content\MenuLinkContentInterface $entity */
  if (!empty($values['entity_id'])) {
    $entity = MenuLinkContent::load($values['entity_id']);
    if ($entity->isTranslatable()) {
      if (!$entity->hasTranslation($node->language()->getId())) {
        $entity = $entity->addTranslation($node->language()->getId(), $entity->toArray());
      }
      else {
        $entity = $entity->getTranslation($node->language()->getId());
      }
    }
  }
  else {
    // Create a new menu_link_content entity.
    $entity = MenuLinkContent::create(array(
      'link' => ['uri' => 'entity:node/' . $node->id()],
      'langcode' => $node->language()->getId(),
    ));
    $entity->enabled->value = 1;
  }
  $entity->title->value = trim($values['title']);
  $entity->description->value = trim($values['description']);
  $entity->menu_name->value = $values['menu_name'];
  $entity->parent->value = $values['parent'];
  $entity->weight->value = isset($values['weight']) ? $values['weight'] : 0;
  $entity->save();
}

The URI is created as 'link' => ['uri' => 'entity:node/' . $node->id()],

New tests are added here.

Undoing the bot, since this is still a beta target for 8.2.x.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -57,11 +58,19 @@ public function __construct(EntityManagerInterface $entity_manager) {
       $entity = $storage->load($value);
+      // If there is no entity loadable by ID, try to load by UUID.
+      if (!$entity && Uuid::isValid($value)) {
+        if ($entities = $storage->loadByProperties(['uuid' => $value])) {
+          $entity = reset($entities);
+        }
+      }

So now Uuid::isValid() is cheaper we can do only one query here.

Something like this...

Also I think we need to cast to string for ctype_digit because it's weird when passed something else.

Let's not change the Uuid component unnecessarily here.

Ah I see... this loads all entities. So it also loads the string IDs of config entities too.

Changes look good to me, thanks @alexpott

Seems like everyone is happy.

Leaving at RTBC, but a question before I'm comfortable with committing this:

With this patch, when you're on a node/{uuid} URL, then you do not get the local task tabs, such as Edit, Delete, and Revisions. Is it ok for that to be the situation in an 8.2.0 release? The IS doesn't say much about what the real use case for UUID-based URLs is, but since the patch is allowing people to add them to their menus, I'm guessing there are some sites that will want to do so and then be confused as to how to access the revision history for that node as well as all the other local tasks provided by core and contrib?

Potentially, integrating local tasks onto the UUID-based URLs could happen in follow-ups, but with the current architecture of this patch (UUID as separate routes rather than as aliases), I'm not yet clear on whether such follow-ups could be successfully implemented in patch releases, or if they'd need to be targeted for 8.3 only, and even then, how much of a pain that would end up being.

Any thoughts on this? Will we need those local tasks at some point, and if we will, then do we still think the architecture of UUID as a separate route is desirable?

Sorry folks. I discussed this with @alexpott, who committed this originally in #93 and tagged it as a beta target in #105, and we decided that despite that original commit, #132 is enough of a concern to keep this out of 8.2.x and finish it for 8.3.x instead. Thank you to everyone who worked hard on this, it's a great feature, and I look forward to it landing in 8.3 whenever it's ready.

My preference would to be to go back to the approach in #34 which was a processor.

We switched to actual routes based on @dawehner's review in #36 where we decided that the UUID regex meant a static pattern would mean there was no performance impact on a real route.

The other option is we add a local-task alter/deriver that duplicates any tasks that live on entity.{type}.canonical to also appear on entity.{type}.uuid routes - thoughts?

Also, is there a reason why this was bunted to 8.3 without a chance/window to rectify whereas other issues that have beta target tag were given a chance to address concerns?

@larowlan we bunted to 8.3 because the issue is a significant and rather than do a rush fix or re-architecture we decided to take our time. There is only 1 beta target left and once that is done we plan to release the next beta and after that there shouldn't be anymore new features or tasks going in to the beta.

One thing we could do is to provide a redirect to the other routes.

IMO this makes most sense.

Otherwise you also can end up with the exact same UI, but with two different URLs. This also means that e.g. conditionally placed blocks would need to be placed at two distinct URLs. Which has ecosystem consequences.

A simple redirect fixes that. Or am I missing something?

This also has consequences for REST. Any entity that is referenced by a entity:ENTITYTYPE/UUID URI can also be resolved by REST clients. Also see #2577923: MenuLinkContent entities pointing to nodes are not deployable: LinkItem should have a "target_uuid" computed property.

Just my 2c but I have to agree with #135, using a processor/redirect makes more sense to me rather than duplicating effort making (for example) node/{uuid} the same as node/{nid}

Let's get this issue going again, so we can put this behind us.

Here's a recap.

1. #132 is a valid concern and offers very convincing reasons to change direction.
2. This patch originally used an inbound path processor (see #34). The intent is for us to have /node/[UUID] links in menus, which can be deployed across machines (unlike /node/[NID]). The inbound path processor then rewrote this to /node/[NID], which means the regular route+controller is used, and path-based matching like that used by blocks still works as expected. Finally, what's stored is UUID menu links, but they're rendered as "normal" menu links: creating a link to /node/[UUID] is still rendered as a link to /node/[NID]. The flaw: you can access /node/[UUID] and that would just work: as far as Drupal is concerned, you're at /node/[NID]. In other words: this violates the duplicate content rule: two distinct URLs serve identical content.
3. Then this patch shifted towards using actual routes (#36 through #129). The flaw: because each entity now has a UUID route too, local tasks, block placement, etc all don't work as expected anymore. Each entity now effectively has two URLs. This is even worse.
4. #134 proposed to solve this via redirects, and I agreed in #138. The UUID URL would redirect to the non-UUID URL. It has the potential to deliver the best of both prior approaches: A) storing links to /node/[UUID], but rendering as /node/[NID], B) those who access /node/[UUID] directly are redirected to /node/[NID] and C) hence block placement etc still work as expected.

I first wanted to agree with larowlan in #135, and the patch in #34 even still applies cleanly (!!!), but I did discover this one big flaw listed above. Hence the redirect approach seems the best candidate.

To clarify #142: You're suggesting:

1) When generating outbound URLs, we still convert node/abc123 to just node/4, so we never OUTPUT uuid-based URLs.

2) If someone sends a uuid-based request anyway, we 301 redirect to node/4 (or its alias).

If so, that sounds like a good plan to me.

Yeah, I think we're stuck with a redirect

• #143: yep, exactly!
• #144: glad to see you're on board!

@larowlan, do you have the time+interest to reroll this? I would provide reviews.

interest √
time ?

this time of year lots of stuff going on outside, will see what I can do

Rerolling #129 to latest 8.2.x as the system update hooks conflicted.

One thing to note from the difference in #129 to #34 that may be of interest: I use a node/uuid path for the site front page so that I can have a homepage node to control the content, with #129 blocks and things that are set to display on work fine but not with #34

#147: can you please provide an interdiff? https://www.drupal.org/documentation/git/interdiff

Sorry, here you go

Another update hook collision :(

Reuploading the patch in #150, because it was never tested by testbot.

00:00:46.346 PHP Fatal error:  Cannot redeclare system_update_8203() (previously declared in /var/www/html/core/modules/system/system.install:1789) in /var/www/html/core/modules/system/system.install on line 1803

Renaming system_update_8203() to system_update_8300().

#2842480: system_update_8203() is named incorrectly just landed, so now #154 must be done again, now moving from system_update_8300 to system_update_8301.

Doing that first, then addressing the failure.

The failure in #154 is because #2585899: HtmlResponseAttachmentsProcessor::processHtmlHeadLink produces invalid HTTP Link header landed since the last green test run, and it added a new test that we now also need to update.

So, in #142, I tried to get this back on the rails.

In #143, Crell summarized it as follows:

To clarify #142: You're suggesting:

1) When generating outbound URLs, we still convert node/abc123 to just node/4, so we never OUTPUT uuid-based URLs.

2) If someone sends a uuid-based request anyway, we 301 redirect to node/4 (or its alias).

If so, that sounds like a good plan to me.

But as the fail in #154 and the fix in #157 are proving, we are outputting a UUID-based URL: in the Link headers. Of course, that UUID-based URL would respond with a redirect.

… except that the patch I've been rerolling to get to green is the WRONG PATCH, because @acbramley rerolled #129 in #147, rather than implementing what we discussed & agreed in #142 (me) + #143 (Crell) + #144 (larowlan).

Gaahhh!!!! I should've looked at the patches since #142 more closely before spending any time on it :(

So, I'm adding a \Drupal\Core\Entity\Controller\EntityViewController::redirectUUidToCanonical method, and am making the necessary corresponding rout e modifications to make this happen. That redirectUUidToCanonical() method is modeled after \Drupal\comment\Controller\CommentController::redirectNode().

It seems this is unfortunately not yet passing because Views is apparently still incorrectly overriding the entity.taxonomy_term.canonical route with its own controller. Or at least, that's what I conclude when I do the debugging of the exception that I'm getting, which looks like this:

Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("entity") to generate a URL for route "entity.taxonomy_term.canonical". in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 180 of core/lib/Drupal/Core/Routing/UrlGenerator.php).

I'll let somebody else take it from here.

Oops, I forgot a hunk.

This will still fail for the reasons I outlined in my previous comment.

@Wim Leers sorry, what did I do wrong?? #147 was a straight reroll of the previous patch to fix the update hook issue. The reason I didn't reroll the older patch with the newly agreed approach is because I had issues with it which are explained in #147...

@acbramley: no need to apologize, this is a super super super confusing issue! I didn't mean to blame you at all, sorry if that's how it came across. #142 basically said we need a hybrid of the different approaches we had had up until that point.

#159 then got that hybrid going.

#168.1: Perhaps we should not expand Drupal/Core/ParamConverter/EntityConverter's capabilities, but instead add a new parameter converter. So that we have {node} and {node_uuid}, instead of just {node} that accepts both ID and UUID. Then it'd be trivial to also make this work for JSON API and other UUID-based link templates.

@Wim Leers: Is that a decision the Entity subsystem maintainers should weigh in on?

Therefore it has to be the same param converter.

Why?

We could add a entity.<entity type ID.uuid route in \Drupal\Core\Entity\Routing\DefaultHtmlRouteProvider, because it's already $rel == 'uuid' anyway.

When we though use a different parameter name, like node_uuid, stuff like local tasks won't work anymore.

But that's exactly what we want. We want /node/<UUID> to redirect to /node/<ID>. See #142.

I came across this issue while a client requires content URLs not to be "guess-able". So disallowing the use of /node/<ID> as ID is incremental and users can just decrease/increase its value. And the easiest solution seems to be using /node/<UUID>.

But if I integrate the solution described from #142.4, so with the redirect, this won't work anymore.

I understand this issue original value is to be able to deploy links to content between environments without breaking them, but I can see obfuscating content paths as a good value too.

From #142.2:

This patch originally used an inbound path processor (see #34). The intent is for us to have /node/[UUID] links in menus, which can be deployed across machines (unlike /node/[NID]). The inbound path processor then rewrote this to /node/[NID], which means the regular route+controller is used, and path-based matching like that used by blocks still works as expected. Finally, what's stored is UUID menu links, but they're rendered as "normal" menu links: creating a link to /node/[UUID] is still rendered as a link to /node/[NID]. The flaw: you can access /node/[UUID] and that would just work: as far as Drupal is concerned, you're at /node/[NID]. In other words: this violates the duplicate content rule: two distinct URLs serve identical content.

The flaw exists anyway if you use multiple path aliases or whatever shows the same content. The fix for it is simply using the canonical URL meta tag which AFAIK is already on node and comments by default.

Ignore my comment if you don't think obfuscating the path is an additional value of using UUID instead of ID on paths, but generally I don't see why #142.2 was a bad approach.

#175: So you say the canonical-url metatag makes the redirect obsolete?
I'd say good news, as that redirect never felt really good imho.

#175: Even if it would not be a redirect it wouldn't make Drupal actually *use* that route or prevent anymore from using node/ID. That is not the goal here, redirect or not.

IMHO, if your goal is to prevent access to node/ID, then this is not the right approach to do that. All you need for that are aliases (You could even make aliases the contain the UUID, that's trivial. Or anything else.) and a early request subscriber that denies access if the original URL before the alias resolver is node/ID.

@Berdir , yeah I probably didn't make myself totally clear.
I found this issue while wondering 'Can I resolve an entity using its UUID?' and the work done so far solves the problem perfectly up to when it switched to redirect /node/<UUID> to /node/<NID>.

Redirecting UUID path --> NID path will make impossible to use just plain /node/<UUID> URLs.
So 'Can I resolve an entity using its UUID?' is answered by 'Kind of. Visiting /node/UUID will redirect to /node/NID and that will resolve your entity'.

My real question was if we do need to redirect. #142.2 refers to a "In other words: this violates the duplicate content rule: two distinct URLs serve identical content." flaw which is a problem taken care by the canonical metatag already existing on nodes.
Also @larowlan on #135 suggested to go back to original - without redirect - solution in #34.

[[P.S.: I personally solved my issue extending EntityConverter as per suggested by patch in here, but I haven't though about aliases. Thanks for the hint!]]