Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
For quite a while, I'd been wondering what the exact purpose was of id_field: uuid
in config/install/jsonapi.resource_info.yml
. Then I ran into \Drupal\jsonapi\ParamConverter\EntityConverterField
and it all made sense.
My concerns:
- Its config schema mysteriously labels it
API field
, whatever that means - Entities are standardized on
id
anduuid
. So what other field could you possibly have, or want to have? I see this was raised in #2745719: [FEATURE] Allow using alternate fields as ID too, but never addressed. - Finally, the default configuration (
uuid
) gets in the way for those entity types that don't have UUIDs. Because even thoughEntityInterface
contains auuid()
method, it's not at all guaranteed sadly:
* @return string|null * The UUID of the entity, or NULL if the entity does not have one.
This means that the default configuration doesn't allow you to access those entities at all.
- If we continue to need the
_entity_load_key
route option, it should become_jsonapi_entity_load_key
: all route options and similar things should be prefixed with the module name. - This being configurable makes it too easy to write contrib modules and JS code that breaks when site A configured it to
id
, but site B configures it touuid
. - JSON API the spec (http://jsonapi.org/) is highly opinionated. JSON API the Drupal 8 module should therefore also be. The fewer knows to play with to configure things, the more portable any code built to talk to Drupal 8 JSON API servers will be. And hence the more productive and plug-and-play the ecosystem will be.
- Therefore I propose to remove this setting altogether, and always use
uuid
when an entity supports UUIDs, and automatically useid
otherwise.
Comment | File | Size | Author |
---|---|---|---|
#35 | id_uuid_followup-2831134-35.patch | 1.48 KB | Wim Leers |
| |||
#30 | id_uuid-2831134-25.patch | 34.6 KB | Wim Leers |
|
Comments
Comment #2
Wim LeersComment #3
Wim LeersThis can be deleted if we implement point 7, but if we don't do that, we should improve this label.
This is how I propose to implement point in the IS.
Comment #4
dawehnerIt would be kinda nice to point to a place which explains that UUIDs are optional
Comment #5
Wim Leers+1.
@see \Drupal\Core\Entity\EntityInterface::uuid()
is the only place that mentions it. There's no explanation anywhere. Once such an explanation is added, it'd be referenced at\Drupal\Core\Entity\EntityInterface::uuid()
, methinks.Comment #6
e0ipsoAll in favor of this new approach. To be honest, I think that an API consumer should not care about the form of ID they are getting. They have to get IDs from an entry point (collections).
One concerning point is POST and PUT. We should be able to accept as input the same thing we're spitting as output. That means that we need to make sure that the denormalizers are also updated in this patch.
My final thought is about BC. Do you think that if an entity gets a
uuid
key in the future, that will break BC for the API? Routes for those entities will change, relationships to those entities will change, …Some comments about the current patch:
We should drop this config property since we will not be using it anymore.
Shouldn't we keep this to avoid involuntary upcasting in non-JSONAPI routes?
Comment #7
e0ipsoComment #8
Wim LeersThe only content entity that does not have a UUID currently, is
\Drupal\aggregator\Entity\Item
(thanks @Berdir for pointing that out!). And that's really an oversight: it has aguid
field, which is kind of like a UUID, but RSS-specific. There's an issue to fix that: #2149851: Remove todo about GUID field on the 'aggregator_item' entity and add UUID field.I think it's fair to assume in practice that every content entity has a UUID. So I think it's definitely fair to consider an entity type that does not have UUIDs will not get it in the future. If that changes, it's a BC break for the entity type, and hence it's unavoidably also a BC break for the JSON API output for that entity type. But, everything indicates this is a super super rare occasion. I don't think JSON API can possibly protect against this.
Can you elaborate on this? I don't quite understand what you mean.
Comment #9
Wim LeersAddressed all feedback.
Comment #11
e0ipsoThis patch is only centered on loading entities in a
GET
. Right now you canPOST
an article that relates to a author via the entity id (if configured). We'll need to enforce that the relationship is using UUID and not Entity ID, when UUID is available.Comment #12
Wim LeersComment #14
Wim LeersI think this is running into the bug described at #2833850: IDs not converted to UUIDs for POST/PATCH to relationship endpoint?
Comment #15
Wim Leers#2833850: IDs not converted to UUIDs for POST/PATCH to relationship endpoint landed, retesting.
Comment #17
Wim LeersSo to make this pass tests, I need to update
\Drupal\Tests\jsonapi\Kernel\Normalizer\DocumentRootNormalizerTest()
. Which also requires updatingResourceConfig::getIdKey()
, to ensure it always returns'uuid'
.This unfortunately also causes not this to be returned:
but:
Because now also config entities must return UUIDs, not IDs, and so for the config entity
node.type.article
, whose contents look like this:and for which the entity type definition (
\Drupal\node\Entity\NodeType
) includes this:… this means we're returning its UUID and not its ID (which was stored in the
type
key).How do you propose we deal with this, e0ipso?
Comment #18
Wim LeersForgot the patch with my changes that caused me to ran into this.
Comment #20
Wim LeersThis is blocked on #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi', because it will definitely conflict with that.
Comment #21
Wim LeersComment #22
Wim LeersThis has shifted course since it started, because now it's about removing the ability to use "ID" at all.
Comment #24
Wim LeersEntityConverterField
toEntityUuidConverter
, which is consistent withEntityRevisionConverter
.Not testing this patch, because it's relative to #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi'.
Comment #25
hampercm CreditAttribution: hampercm at Acquia commentedPatch in #24 looks good to me!
Comment #26
Wim LeersNow working to remove
ResourceConfig::getIdKey()
, because with this change it no longer has a reason to exist.This also forced me to update quite a bit of test coverage. In fact, this required the addition of some very arduous missing test coverage (and especially mocks) to
\Drupal\Tests\jsonapi\Unit\Normalizer\JsonApiDocumentTopLevelNormalizerTest
. Missing, because the existing test coverage only handledid
, notuuid
.Comment #27
Wim LeersBlocked on #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi'.
Comment #28
hampercm CreditAttribution: hampercm at Acquia commentedThe patch in #26 looks good as well. Verified tests pass when applied on top of #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi'. Can be RTBC once that lands!
Comment #29
Wim LeersWow, thanks for your thorough testing!
Comment #30
Wim Leers#2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi' is in, reuploading #25 for testing.
Comment #31
Wim LeersRTBC per #28.
Comment #32
e0ipsoSome comments (none of them require a code change):
w00t!
I think that this could be a good converter for Drupal core. Thoughts?
This shows that even with the current context class instead, we are exploding the type here. This should be fixed in a follow up.
We should remove host_uuid in that follow up.
Node types as UUIDs are not ideal, but that's life.
I'm curious, more than anything else, about the reason of this variable reassign.
Comment #34
e0ipsoMerged.
Comment #35
Wim Leers#32.2: yep, I was thinking the same. BUT! One major concern is that this would mean you can access
example.com/node/1
andexample.com/node/<UUID>
, and they'd return exactly the same. But, good news: #2353611: Make it possible to link to an entity by UUID is doing exactly this! Once that's in, we can delete this code. You asking this made me remember that issue. I'll create a follow-up to add that@todo
.#32.6: because we need to access
$this
in a closure:… but due to copy/pasting this code and refactoring it in one other case, that other case has an unnecessary
$self = $this
. We should fix that.Comment #36
e0ipsoI'll merge this and also remove
host_uuid
in the commit.Comment #38
e0ipsore-fixed