Problem/Motivation

It would be great to provide URLs with UUIDs in their so you can link to them.

Proposed resolution

Add a new entity link format: {entity_type}/{uuid} - e.g. viewing /node/68d0a104-a5bf-466c-a429-f871d91f9580 is same as viewing node/3.
Allow URIs like entity:node/68d0a104-a5bf-466c-a429-f871d91f9580
Add a new uuid link template for entities:

$node->toUrl('uuid');

Remaining tasks

User interface changes

API changes

None

Files: 

Comments

chx’s picture

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

dawehner’s picture

Status: Active » Needs review
FileSize
2.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,050 pass(es), 32 fail(s), and 9 exception(s). View

This makes it at least possible to link to it.

Status: Needs review » Needs work

The last submitted patch, 2: 2353611-2.patch, failed testing.

dawehner’s picture

FileSize
3.12 KB

Just in case someone is already interested, here is a small module implementing this kind of functionality.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,181 pass(es). View

Just fixing hopefully all the failures for now.

dawehner’s picture

FileSize
5.86 KB

.

clemens.tolboom’s picture

Crell’s picture

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.

dawehner’s picture

Issue tags: +uuid-everywhere

Adding the tag.

clemens.tolboom’s picture

Status: Needs review » Needs work

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

Is #6 supposed to be a patch maybe?

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.88 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View

first a reroll, then tackling #10

Status: Needs review » Needs work

The last submitted patch, 11: link-entity-uuid-2353611.11.patch, failed testing.

larowlan’s picture

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

larowlan’s picture

Assigned: Unassigned » larowlan

working on this a bit

larowlan’s picture

Status: Needs work » Needs review
FileSize
8.93 KB
11.24 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

Status: Needs review » Needs work

The last submitted patch, 15: link-entity-uuid-2353611.15.patch, failed testing.

The last submitted patch, 11: link-entity-uuid-2353611.11.patch, failed testing.

larowlan’s picture

Assigned: larowlan » Unassigned

until tomorrow in case anyone else is inclined

The last submitted patch, 15: link-entity-uuid-2353611.15.patch, failed testing.

skwashd’s picture

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

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Task » Feature request
sime’s picture

Updating patch from #15.

Wim Leers’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: link-entity-uuid-2353611.22.patch, failed testing.

sime’s picture

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).
sime’s picture

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?

sime’s picture

Issue summary: View changes

Modified description to reflect the recent patches.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Anyone wants to discuss what / how we actually want to implement it?

larowlan’s picture

Yeah, will try and catch you tomorrow

larowlan’s picture

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

$entity->toUrl('uuid');
larowlan’s picture

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

  • Update to use inbound path processor instead
  • Update tests
  • Profile
larowlan’s picture

Assigned: Unassigned » larowlan

Working on this

larowlan’s picture

Status: Needs work » Needs review
FileSize
29.92 KB
15.83 KB

Interdiff is against #22

Let's see what fails here.

benjy’s picture

  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’s picture

Just a quick thought:

+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorEntityUuid.php
@@ -0,0 +1,58 @@
+    if (preg_match('/^\/([a-z_]+)\/(' . Uuid::VALID_PATTERN . ')$/i', $path, $matches)) {

So given that we have a static pattern we could improve the situation for all other routes, by registering one route per entity type together with the UUID pattern as requirement.

larowlan’s picture

@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?

dawehner’s picture

+1 from me.

Wim Leers’s picture

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}?

timmillwood’s picture

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.

larowlan’s picture

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

timmillwood’s picture

@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.

dawehner’s picture

Issue tags: +DrupalCampES

/{uuid}

I mean technically we decided against having such URLs, so we always at least some prefix, but generically I kinda dislike to totally have missing semantic information about what is contained behind those resources.

larowlan’s picture

working on this

larowlan’s picture

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

Status: Needs review » Needs work

The last submitted patch, 45: 2353611-uuid-link.45.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
23.27 KB

More work on failing tests

larowlan’s picture

FileSize
3.72 KB

Status: Needs review » Needs work

The last submitted patch, 47: 2353611-uuid-link.47.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
5.54 KB
25.71 KB

More work on failing tests

Status: Needs review » Needs work

The last submitted patch, 50: 2353611-uuid-link.50.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
726 bytes
25.89 KB

more fixes

dawehner’s picture

Nice, a green patch!

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    --- a/core/lib/Drupal/Core/Entity/EntityTypeManager.php
    +++ b/core/lib/Drupal/Core/Entity/EntityTypeManager.php
    
    +++ b/core/modules/user/src/Entity/UserRouteProvider.php
    @@ -2,21 +2,33 @@
    diff --git a/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php b/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
    
    diff --git a/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php b/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
    index c2325d4..6c96e79 100644
    
    index c2325d4..6c96e79 100644
    --- a/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
    
    --- a/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeManagerTest.php
    @@ -99,6 +99,14 @@ protected function setUpEntityTypeDefinitions($definitions = []) {
    
    @@ -99,6 +99,14 @@ protected function setUpEntityTypeDefinitions($definitions = []) {
           // Give the entity type a legitimate class to return.
           $entity_type->getClass()->willReturn($class);
     
    +      // Don't allow uuid key.
    +      $entity_type->hasKey('uuid')->willReturn(FALSE);
    +
    +      // Or route provider.
    +      if (!$entity_type->getMethodProphecies('getRouteProviderClasses')) {
    +        $entity_type->getRouteProviderClasses()->willReturn([]);
    +      }
    +
           $definitions[$key] = $entity_type->reveal();
         }
     
    

    Do you think we should expand the test coverage of the entity type manager here?

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

    Should we respect previously defined UUID templates?

  3. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
    @@ -23,12 +24,13 @@
    -class DefaultHtmlRouteProvider implements EntityRouteProviderInterface, EntityHandlerInterface {
    +class DefaultHtmlRouteProvider implements EntityUuidRouteProviderInterface, EntityHandlerInterface {
    
    +++ b/core/lib/Drupal/Core/Entity/Routing/EntityUuidRouteProviderInterface.php
    @@ -0,0 +1,10 @@
    +<?php
    +
    +namespace Drupal\Core\Entity\Routing;
    +
    +/**
    + * Defines an interface for route providers that support a UUID link template.
    + */
    +interface EntityUuidRouteProviderInterface extends EntityRouteProviderInterface {
    +
    +}
    

    Do we need that?

  4. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
    @@ -151,9 +154,13 @@ public function checkNodeAccess(array $tree) {
           $nids = $query->execute();
    -      foreach ($nids as $nid) {
    -        foreach ($node_links[$nid] as $key => $link) {
    -          $node_links[$nid][$key]->access = $access_result;
    +      if ($nids) {
    +        foreach ($nids as $nid) {
    +          if (isset($node_links[$nid])) {
    +            foreach ($node_links[$nid] as $key => $link) {
    +              $node_links[$nid][$key]->access = $access_result;
    +            }
    +          }
             }
           }
    

    We could prevent the if() by casting it to an array first. Not sure which you though you prefer.

  5. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
    @@ -2,6 +2,7 @@
    @@ -62,6 +63,12 @@ public function convert($value, $definition, $name, array $defaults) {
    
    @@ -62,6 +63,12 @@ public function convert($value, $definition, $name, array $defaults) {
         $entity_type_id = $this->getEntityTypeFromDefaults($definition, $name, $defaults);
         if ($storage = $this->entityManager->getStorage($entity_type_id)) {
           $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);
    +        }
    +      }
    

    Can we document on the convert method that the value might also be a uuid?

  6. +++ b/core/modules/menu_ui/menu_ui.module
    @@ -201,8 +201,12 @@ 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', 'internal:/node/' . $node->id())
    +        ->condition('link.uri', 'internal:/node/' . $node->uuid());
    +      $query->condition($group)
    
    @@ -212,8 +216,12 @@ 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', 'internal:/node/' . $node->id())
    +        ->condition('link.uri', 'internal:/node/' . $node->uuid());
    +      $query->condition($group)
    

    Don't we miss the case of entity:/node/$uuid?

larowlan’s picture

  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
acbramley’s picture

Just confirming this applies and works on 8.1.0 as well

larowlan’s picture

jibran’s picture

+++ 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?

dawehner’s picture

Just some continues review ...

  1. +++ b/core/lib/Drupal/Core/Entity/Routing/EntityUuidRouteProviderInterface.php
    @@ -0,0 +1,10 @@
    + */
    +interface EntityUuidRouteProviderInterface extends EntityRouteProviderInterface {
    

    I still haven't got the point of this interface to be honest ...

  2. +++ b/core/modules/node/src/Entity/NodeRouteProvider.php
    @@ -2,21 +2,33 @@
    +
    +    $route = (new Route("/node/{node}"))
    +      ->addDefaults([
    ...
    +      ->setRequirement('node', '^' . Uuid::VALID_PATTERN . '$')
    +      ->setRequirement('_entity_access', 'node.view');
    +    $route_collection->add('entity.node.uuid', $route);
    +
    

    I'm wondering what happens for page manager for example now that the path pattern is there now twice. Do they to check for the requirement to just override one of the specific routes or would they override both routes?

Wim Leers’s picture

  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/

dawehner’s picture

+ $has_uuid_route = isset($route_providers['html']) && in_array(EntityUuidRouteProviderInterface::class, class_implements($route_providers['html']), TRUE);

The reason why I don't understand it is that I think people should be still able to normally register routes using yml files.

larowlan’s picture

#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?

dawehner’s picture

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

Mh I see where you are coming from, it is still a bit weird

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityTypeManager.php
@@ -83,6 +84,12 @@ public function processDefinition(&$definition, $plugin_id) {
+    $has_uuid_route = isset($route_providers['html']) && in_array(EntityUuidRouteProviderInterface::class, class_implements($route_providers['html']), TRUE);

Yeah I'm not 100% sure about this one. This kinda makes the assumption we would just provide HTML routes with the 'html' key. There is no such assumption somewhere in the system so far.

larowlan’s picture

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.

dawehner’s picture

#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

I like this idea. Less magic!

Wim Leers’s picture

#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.

larowlan’s picture

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
dawehner’s picture

+++ 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}}");

What about using a UUID link template, in case there is one?

jibran’s picture

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?

dawehner’s picture

Can we add comment or use single quotes instead?

I agree, let's go with single quotes instead ...

larowlan’s picture

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

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

larowlan’s picture

FileSize
1.24 KB
36.07 KB

Fixes #69

Wim Leers’s picture

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.

larowlan’s picture

FileSize
1.15 KB
36.41 KB

#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

Wim Leers’s picture

  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.

larowlan’s picture

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.

larowlan’s picture

FileSize
12.12 KB
44.16 KB

Done

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

That was my only remaining concern. Thanks! :)

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

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

larowlan’s picture

@tstoeckler can you clarify what you mean there?

larowlan’s picture

+++ 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

tstoeckler’s picture

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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
848 bytes
44.15 KB

Nice one @tstoeckler - fixed

Status: Needs review » Needs work

The last submitted patch, 83: 2353611-uuid-link.83.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
885 bytes
44.24 KB
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

@tstoeckler++

dawehner’s picture

Nice!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
44.34 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

The feedback from @alexpott got addressed

alexpott’s picture

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;
   }
dawehner’s picture

Merge looks perfect!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.2.0 release notes

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

  • alexpott committed 85bc69d on 8.2.x
    Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers,...
bzrudi71’s picture

Status: Fixed » Needs work

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/

alexpott’s picture

Status: Needs work » Fixed

  • alexpott committed 895c204 on 8.2.x
    Revert "Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers...
alexpott’s picture

Status: Fixed » Needs work

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

bzrudi71’s picture

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 ;)

daffie’s picture

Status: Needs review » Needs work

The last submitted patch, 100: 2353611-100.patch, failed testing.

daffie’s picture

Second try to fix the PostgreSQL test bug.

Status: Needs review » Needs work

The last submitted patch, 102: 2353611-102.patch, failed testing.

The last submitted patch, 91: 2353611-91.patch, failed testing.

alexpott’s picture

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.

alexpott’s picture

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.

alexpott’s picture

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.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
    @@ -150,10 +178,25 @@ public function checkNodeAccess(array $tree) {
    +      if (!empty($nids)) {
    +        $nodes = $this->entityTypeManager->getStorage('node')->loadMultiple($nids);
    

    You would be able to skip using this load multiple by using a groupby in the query. Not sure though whether its worth to do so.

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -325,7 +326,7 @@ public static function fromUri($uri, $options = []) {
    +   *   parse_url(). Note that {entity_id} can be both a UUID and a serial ID.
    

    Note: config entities don't have a serial ID, should we somehow point to that as well in the documentation?

  3. +++ b/core/modules/menu_ui/src/Tests/MenuNodeTest.php
    @@ -338,4 +339,38 @@ function testMultilingualMenuNodeFormWidget() {
    +    /* @var \Drupal\node\NodeTypeInterface $type */
    +    $type = NodeType::load('page');
    +    // Enable the main menu for this node type..
    +    $menu_name = 'main';
    +    $type->setThirdPartySetting('menu_ui', 'available_menus', [$menu_name]);
    +    $type->save();
    +    // Test links using node/{uuid}.
    +    $node6 = $this->drupalCreateNode(array('type' => 'page'));
    +    $uuid_link = $this->addMenuLink('', '/node/' . $node6->uuid(), $menu_name);
    +    $this->verifyMenuLink($uuid_link, $node6);
    +    $this->drupalGet($node6->url('edit-form'));
    +    $this->assertFieldByName('menu[title]', $uuid_link->label());
    +    $this->drupalPostForm(NULL, [], t('Save'));
    +    \Drupal::entityManager()->getStorage('menu_link_content')->resetCache([$uuid_link->id()]);
    +    /** @var \Drupal\menu_link_content\MenuLinkContentInterface $uuid_link */
    +    $uuid_link = MenuLinkContent::load($uuid_link->id());
    +    $this->assertEqual($uuid_link->getUrlObject(), Url::fromUri('internal:/node/' . $node6->uuid()));
    +    // Test with entity:node/{uuid}.
    +    $node7 = $this->drupalCreateNode(array('type' => 'page'));
    +    $uuid_link = $this->addMenuLink('', 'entity:node/' . $node7->uuid(), $menu_name);
    +    $this->verifyMenuLink($uuid_link, $node7);
    +    $this->drupalGet($node7->url('edit-form'));
    +    $this->assertFieldByName('menu[title]', $uuid_link->label());
    +    $this->drupalPostForm(NULL, [], t('Save'));
    +    \Drupal::entityManager()->getStorage('menu_link_content')->resetCache([$uuid_link->id()]);
    +    /** @var \Drupal\menu_link_content\MenuLinkContentInterface $uuid_link */
    +    $uuid_link = MenuLinkContent::load($uuid_link->id());
    +    $this->assertEqual($uuid_link->getUrlObject(), Url::fromUri('entity:node/' . $node7->uuid()));
    

    Some new lines here would improve the readability quite a bit IMHO

  4. +++ b/core/modules/taxonomy/taxonomy.routing.yml
    @@ -76,6 +76,16 @@ entity.taxonomy_vocabulary.overview_form:
     
    +entity.taxonomy_term.uuid:
    +  path: '/taxonomy/term/{taxonomy_term}'
    +  defaults:
    +    _entity_view: 'taxonomy_term.full'
    +    _title: 'Taxonomy term'
    +    _title_callback: '\Drupal\taxonomy\Controller\TaxonomyController::termTitle'
    +  requirements:
    +    _entity_access: 'taxonomy_term.view'
    +    taxonomy_term: '[0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12}'
    +
    

    What happens if views takes part in here? Don't we override this URL as well, but it doesn't work any longer?

    I think we have to expand \Drupal\views\Plugin\views\argument_validator\Entity::validateArgument to also accept UUIDs.

  5. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
    @@ -274,7 +296,7 @@ public function testCheckNodeAccess() {
    -      4 => MenuLinkMock::create(array('id' => 'node.4', 'route_name' => 'entity.node.canonical', 'title' => 'qux', 'parent' => 'node.3', 'route_parameters' => array('node' => 4))),
    +      4 => MenuLinkMock::create(array('id' => 'node.4', 'route_name' => 'entity.node.uuid', 'title' => 'qux', 'parent' => 'node.3', 'route_parameters' => array('node' => '7910d537-04be-450a-8200-b53f49ea6b30'))),
    

    Is it just me that it is a bit weird to remove an existing testcase instead of just adding a new one?

  6. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
    @@ -289,12 +311,32 @@ public function testCheckNodeAccess() {
    +
         $query = $this->getMock('Drupal\Core\Entity\Query\QueryInterface');
    +    $condition = $this->getMock(ConditionInterface::class);
         $query->expects($this->at(0))
    +      ->method('orConditionGroup')
    +      ->willReturn($condition);
    +    $condition->expects($this->at(0))
    +      ->method('condition')
    +      ->with('nid', [1, 2, 3])
    +      ->willReturn($condition);
    +    $condition->expects($this->at(1))
           ->method('condition')
    -      ->with('nid', array(1, 2, 3, 4));
    +      ->with('uuid', [3 => '7910d537-04be-450a-8200-b53f49ea6b30'])
    +      ->willReturn($condition);
         $query->expects($this->at(1))
           ->method('condition')
    +      ->with($condition);
    +    $query->expects($this->at(2))
    +      ->method('condition')
    

    This is just perfect test code ... at(N) everywhere, what else do you need

alexpott’s picture

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.

alexpott’s picture

@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.

dawehner’s picture

@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.

We made that clear on IRC. This was me being snarky about the existing test, not about the added code.

xjm’s picture

Issue tags: -8.2.0 release notes

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

  • alexpott committed 85bc69d on 8.3.x
    Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers,...
  • alexpott committed 895c204 on 8.3.x
    Revert "Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers...

  • alexpott committed 85bc69d on 8.3.x
    Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers,...
  • alexpott committed 895c204 on 8.3.x
    Revert "Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers...
larowlan’s picture

Working on #108

dawehner’s picture

+++ b/core/modules/views/src/Plugin/views/argument_validator/Entity.php
@@ -168,8 +169,28 @@ public function validateArgument($argument) {
+      foreach ($uuids as $uuid) {
+        if ($entities = $storage->loadByProperties(['uuid' => $uuid])) {

What about using an entity query + a multiload?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

larowlan’s picture

\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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

At least for me this looks great!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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?

larowlan’s picture

Status: Needs work » Needs review
FileSize
555 bytes
58.18 KB

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.

larowlan’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
webchick’s picture

Version: 8.3.x-dev » 8.2.x-dev

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

alexpott’s picture

Status: Needs review » Needs work
+++ 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.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.58 KB
58.23 KB

Something like this...

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

alexpott’s picture

Let's not change the Uuid component unnecessarily here.

Status: Needs review » Needs work

The last submitted patch, 127: 2353611-3-127.patch, failed testing.

alexpott’s picture

larowlan’s picture

Changes look good to me, thanks @alexpott

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Seems like everyone is happy.

effulgentsia’s picture

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?

effulgentsia’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: -beta target

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.

dawehner’s picture

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

larowlan’s picture

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?

larowlan’s picture

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?

alexpott’s picture

@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.

Wim Leers’s picture

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?

Wim Leers’s picture

Issue tags: +API-First Initiative

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: Menu link content entities that point to nodes are not deployable.

larowlan’s picture

Issue tags: +Default content
acbramley’s picture

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}

Wim Leers’s picture

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.

Crell’s picture

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.

larowlan’s picture

Yeah, I think we're stuck with a redirect

Wim Leers’s picture

  • #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.

larowlan’s picture

interest √
time ?

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

acbramley’s picture

FileSize
57.91 KB

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

Wim Leers’s picture

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

acbramley’s picture

FileSize
593 bytes

Sorry, here you go

acbramley’s picture

FileSize
57.91 KB
823 bytes

Another update hook collision :(

jibran’s picture

Status: Needs work » Needs review
Wim Leers’s picture

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

Status: Needs review » Needs work

The last submitted patch, 152: 2353611-150.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
60.2 KB
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().

Status: Needs review » Needs work

The last submitted patch, 154: 2353611-154.patch, failed testing.

Wim Leers’s picture

FileSize
60.11 KB
623 bytes

#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.

Wim Leers’s picture

FileSize
60.78 KB
711 bytes

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.

Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

FileSize
62.92 KB
8.67 KB

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.

Status: Needs review » Needs work

The last submitted patch, 159: 2353611-158.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
561 bytes
62.93 KB

Oops, I forgot a hunk.

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

Status: Needs review » Needs work

The last submitted patch, 161: 2353611-161.patch, failed testing.

  • alexpott committed 85bc69d on 8.4.x
    Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers,...
  • alexpott committed 895c204 on 8.4.x
    Revert "Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers...

  • alexpott committed 85bc69d on 8.4.x
    Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers,...
  • alexpott committed 895c204 on 8.4.x
    Revert "Issue #2353611 by larowlan, dawehner, sime, alexpott, Wim Leers...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

acbramley’s picture

@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...

Wim Leers’s picture

@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.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -315,6 +315,9 @@ protected function urlRouteParameters($rel) {
    +    if ($rel === 'uuid') {
    

    This would not be flexible enough for jsonapi. I guess we would need to load the route and check somehow whether something needs a uuid. Not sure how to detect that though.

  2. +++ b/core/lib/Drupal/Core/Entity/Routing/DefaultHtmlRouteProvider.php
    @@ -235,6 +244,31 @@ protected function getCanonicalRoute(EntityTypeInterface $entity_type) {
    +        ->setRequirement('_access', 'TRUE')
    

    I'm wondering whether we should copy over the access checking of the normal entity somehow

Wim Leers’s picture

#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.

DamienMcKenna’s picture

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

dawehner’s picture

#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},

Not sure how you think this should work for the original problem this issue tried to solve: Linking to /node/$uuid should be possible. Therefore it has to be the same param converter. In the usecase of JSONAPI it would work to have two separate converters, but there is still no easy way to link to it.

Wim Leers’s picture

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.

dawehner’s picture

We could add a entity.

Note: We do that already. When we though use a different parameter name, like node_uuid, stuff like local tasks won't work anymore.

Wim Leers’s picture

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.

gambry’s picture

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.

axel.rutz’s picture

#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.

Berdir’s picture

#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.

gambry’s picture

@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!]]

Grimreaper’s picture

Hello,

Here is a reroll the patch from comment #161 against branch 8.3.x. I was not able to make an interdiff because patch from comment #161 did not apply anymore.

If I understood the comments it is this patch that needs to pass. If someone can confirm, thanks.

I have changed arrays into short array syntax and tried to update some code in the tests that

I had a problem updating come code in the tests because of properties that changed. I have commented using // the parts I was not able to port.

I have not tested the patch yet.

Grimreaper’s picture

After applying the patch, if I try to go to node/[UUID], I am redirected to node/[NID]. That is ok.

But the UUID does not seems to be stored in case of a link field, it is still the ID.

And I have the following error when displaying a taxonomy term field value:

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).
Drupal\Core\Routing\UrlGenerator->getInternalPathFromRoute('entity.taxonomy_term.canonical', Object, Array, Array) (Line: 291)
Drupal\Core\Routing\UrlGenerator->generateFromRoute('entity.taxonomy_term.canonical', Array, Array, 1) (Line: 105)
Drupal\Core\Render\MetadataBubblingUrlGenerator->generateFromRoute('entity.taxonomy_term.canonical', Array, Array, 1) (Line: 759)
Drupal\Core\Url->toString(1) (Line: 167)
Drupal\Core\Utility\LinkGenerator->generate('tag', Object) (Line: 94)
Drupal\Core\Render\Element\Link::preRenderLink(Array)
call_user_func(Array, Array) (Line: 376)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 76)
__TwigTemplate_a6591983c7b2818f783039d045606bc65df838e04dad4892ffdd0917b7d12d79->doDisplay(Array, Array) (Line: 379)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 347)
Twig_Template->display(Array) (Line: 358)
Twig_Template->render(Array) (Line: 64)
twig_render_template('core/themes/bartik/templates/field--node--field-tags.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('field', Array) (Line: 435)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 448)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 490)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 114)
__TwigTemplate_a624ba251fc4b3c6b5feacfb6a9c332a974950caa272a7ee83a047a42ffdc8b3->doDisplay(Array, Array) (Line: 379)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 347)
Twig_Template->display(Array) (Line: 358)
Twig_Template->render(Array) (Line: 64)
twig_render_template('core/themes/bartik/templates/node.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('node', Array) (Line: 435)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 149)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 656)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Wim Leers’s picture

#175: security through obscurity is never a good idea. If your client requirement is like that, then I suggest instead implementing Entity Access to disallow access. And if a 403 is already divulging information, then map 403s to 404s. Done.
Or… what @Berdir described in #177.

#178: the "duplicate content" I wrote was not from an SEO POV, but from a Drupal POV: we simply shouldn't have two routes serving the exact same content. This has consequences beyond SEO. An example: a block is made visible only for /node/5… but then if you access it via /node/<UUID for node 5>, then you don't see the block. Clearly dangerous.

#180: I already explained that failure at the bottom of #159: this is a long-standing bug in Views.

michaellander’s picture

So the JSONAPI module currently requires the bundle to request a resource(i.e. /jsonapi/node/article/), and I'm curious if that should be considered here. For example, If I'm moving some link field data between two sites and would like to lookup the related entity from the link, it would be great to have all the information needed for the subsequent request. This probably opens a whole other can of worms...