Cannot use UUID as entity ID

After talking to @rnsrk abou this in Burgas, spent some more time on this.

Fixed the issue mentioned in #9. Also added some inline comments about the changes and finally added some test coverage. I pushed those to individual commits in case there are any objections to any of those.

Note that I thought a bit about how to avoid all this array madness (i.e. array_values(array_unique(array_filter(...))) and friends) and I think the solution would be to make DefaultTableMapping provide an addFields() method, which would internally check for duplicate fields. Will open a follow-up about that, so we can avoid that discussion here (even though it does make the situation (inherently) slightly worse).

Not exactly why this was tagged "Needs change record" or what the change record should be. I mean we could literally write a change notice with "Content entities may now use the same field for the ID and UUID keys" if people think that's useful. (I don't, in particular, but it also doesn't hurt, so I wouldn't mind it.)

Not sure why the testbot doesn't run for the updated commits, but I opened 📌 Replace DefaultTableMapping::setFieldNames() with ::add*() and ::remove*() Needs work in the meantime.

...ahh maybe because the MR is targeting 10.1.x. @bradjones1 do you have access to change the target branch? Or should we create a new MR? Not sure...

MR target changed.

Needed a rebase and now GitLab CI is running.

Added a basic CR. Tests green now.

Awesome, thanks. You dropped my commits from the MR, though, I'm guessing that was accidental? 🤔

That is so strange, I think something weird happened with changing the merge target. I thought it was strange I didn't need to do a rebase --onto... and clearly that would be necessary. I've fixed this. Thanks for catching it.

Great, thanks! 👍

Docs will need changing.

For example, on EntityTypeInterface::getKeys()

   *   - id: The name of the property that contains the primary ID of the
   *     entity. Every entity object passed to the Field API must have this
   *     property and its value must be numeric.

That documentation is actually already incorrect prior to this issue, so opened 📌 Update EntityTypeInterface::getKeys() docs for string IDs Needs review about that. We are not changing anything about being able to use strings in the first place, this is just about using UUIDs, in particular, and to be more precise to be able to use the same field as UUID key and ID key.

There a recommended way for testing this one?

Beyond the test that's in the MR? AFAIK this isn't test-able in the UI since you can't create a custom entity type in core via the UI.

1 test triggered 2 PHP warnings:
1) /builds/issue/drupal-3310170/core/modules/mysql/src/Driver/Database/mysql/Schema.php:218
Array to string conversion
Triggered by:
* Drupal\FunctionalTests\Entity\EntityUuidIdTest::testUi
  /builds/issue/drupal-3310170/core/tests/Drupal/FunctionalTests/Entity/EntityUuidIdTest.php:45
2) /builds/issue/drupal-3310170/core/modules/mysql/src/Driver/Database/mysql/Schema.php:218
Undefined array key "Array:normal"
Triggered by:
* Drupal\FunctionalTests\Entity\EntityUuidIdTest::testUi
  /builds/issue/drupal-3310170/core/tests/Drupal/FunctionalTests/Entity/EntityUuidIdTest.php:45
ERRORS!

In that case I ran the test-only feature to show the test coverage.

Cleaned up the issue summary some, copied the CR to the release notes as seems like something that probably should be well announced.

Code review wise everything seems to have proper typehints and return hints. The comments added to DefaultTableMapping about why to get unique is a nice touch.

Going to go out on a limb and say this one is good.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed that, thanks!

A simple solution to the problem, I don't foresee any side effects but also a little bit wary of touching DefaultTableMapping; as this also has a change record I only committed this to 11.1.x and 10.4.x and higher for now so it will be available in the next minor release.

longwave → closed merge request !2766

Looks like after these changes on 11.x, PHPStan is failing the build: https://git.drupalcode.org/project/drupal/-/jobs/3330912.

------ ---------------------------------------------------------------------------- 
  Line   core/modules/language/tests/src/Traits/LanguageTestTrait.php (in            
         context of class Drupal\FunctionalTests\Entity\EntityUuidIdTest)            
 ------ ---------------------------------------------------------------------------- 
  65     Method                                                                      
         Drupal\FunctionalTests\Entity\EntityUuidIdTest::disableBundleTranslation()  
         has no return type specified.                                               
 ------ ---------------------------------------------------------------------------- 

I regenerated baseline.

Thanks for the followup, not sure what happened there as PHPStan didn't complain locally on commit.

Committed and pushed b5c5a2a3ef8 to 11.x and 07eea76d204 to 11.1.x. Thanks!

godotislate → closed merge request !10157

See y'all in ✨ Make it possible to link to an entity by UUID Active next? 🤓

@wim: The point of this issue is that the UUID can be the ID, at the same time. That means these entities do not need to link by UUID because it's literally the same thing as linking by ID.

This got it in just before 📌 Convert entity type discovery to PHP attributes Needs review , so the new EntityTestUuidId entity type is still using annotation instead of attributes, should we open a follow-up issue or do a quick fix here?

@berdir at some point we will deprecate annotations so can probably mop it up then?

Automatically closed - issue fixed for 2 weeks with no activity.

I just successfully created an "uuid only entity" with just using "entity_keys = { "id" = "uuid", ...". That's awesome and many thanks to everybody who made this possible. I will use this from now on many upcoming contrib and custom entities.

But when there is already content and other code handling with the serial ID? On some other uuid discussions I read many comments of people loving the old serial user and node IDs. For users I created the module "u3id" to handle a mixed usage. So I see a need of mixed usage and transformation of existing content etc. How to start in contrib/custom and how at core entities?

This is great! But how do you use it?

As I understand this change, content entities ("nodes") may now use UUID's instead of integers, so I was hoping to see a new option, like "Use UUID's in stead of classic node ID's" under /admin/structure/types/add, when creating a new content type ...

Do you need to create a custom module to use this new feature, and if yes, is there a documentation page?

This is an option for new entity types, it is not something that can be enabled for existing entity types such as nodes. It's a storage/base field decision, not something that can be configured. Existing core entity types such as nodes do not and will never* use this.

It's an interesting option for some use cases, but has its drawbacks, database indexes for v4 UUIDs perform much worse than integers for example.

* never say never, but I don't see how that's possible to migrate, all kinds of things reference nodes by ID and there are still things that do not support string entity IDs in core, such as comments.

Thanks for clarifying @berdir. I wonder if a regular old school nid-based node could reference content with a UUID-based entity type, or perhaps as @c-logemann mentions, he created "... the module "u3id" to handle a mixed usage.", and this is needed?

If the basics of setting up a basic entity type using UUID as identifier hasn't already been created, it would be fantastic if a basic documentation page was made, maybe also covering the subject of mixing nid's and UUID's?

I think you still mix up entity types, content types and bundles a bit. Which is understandable, because the terminology is confusing.

This allows to set both the id and the uuid to the same field for an entity type. That's all. In practice, all the change really does is fix a problem that the storage got confused when the same field ended up multiple times in automatically created indexes in the table.

See the committed example test entity type: https://git.drupalcode.org/project/drupal/-/commit/9b1a9796b42d6bd581b5c..., I also mentioned it in the change record a bit.

There is no mixing of nid and uuid, because a custom entity type doesn't have a nid (because only nodes do) or any other integer id, both id() and uuid() return the UUID. They can be referenced by and reference other entity types because entity reference fields understand when the target entity type has a string ID.

Ah yes, I do have a hard time differentiating between them, thank you for your patience :)

Maybe it would have been easier if I had shared my vision: I was hoping to be able to create content, like we can with nodes now, but on top of NID's use UUID's as well (or solely UUID's) specifically to be able to manage most content as configuration, allowing some (or all) content to use UUID's as identifier. The point would be to not use the standard auto generated incrementing integer as unique identifier, as we do now with NID's. But maybe that's not possible ...

All nodes also have a UUID, always had since Drupal 8.0. You can rely on that.

The default_content project (including recipe default content in core which is the same format and implementation) uses the UUID as primary identifier for dependency resolution. Both REST and JSON:API modules use the UUID of entities the primary external identifier.

In both cases because the UUID is reliably unique and can be explicitly set, while it is not possible to rely on using a specific serial ID as that may already be in use in a given system.

That's awesome! I didn't realize that, thanks for making me aware, and of modules using UUID as identifier, I'll take a closer look at them. I am very grateful for your detailed answers, it helped me a lot. ... and now, I won't be adding more noise to this issue :)