Problem/Motivation
REST, JSON API, and GraphQL all expose entities via HTTP APIs. However, there are some entity types which should not logically be exposed. As of Drupal 8.5, all TypedData definitions have the isInternal
method. Entities types implement this interface with EntityDataDefinition. However, it is impossible to set an entity type as internal.
REST module is using a hook to remove these entity types from the API and JSON API module has an issue for removing these as well.
Fixing this missing piece will allow new entity types to opt-out of these exposed APIs and it will prevent REST, JSON API and GraphQL (perhaps more in the future) from duplicating behavior.
Proposed resolution
Look for internal = TRUE
in EntityType annotations and set this on the typed data definition so that EntityDataDefinition::isInternal() will return a meaningful value.
Remaining tasks
#2936725: EntityDataDefinition::create() does not respect derived entity definitions
Add a line to Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver
so that it looks for an "internal" key on the entity type annotation.
User interface changes
None.
API changes
isInternal()
is added to Drupal\Core\Entity\EntityTypeInterface
and is implemented by Drupal\Core\Entity\EntityType
.
Data model changes
EntityType annotations will now be able to mark themselves as "internal"
Comment | File | Size | Author |
---|---|---|---|
#35 | 2936714-35.patch | 5.47 KB | gabesullice |
#35 | interdiff-2936714-35.txt | 532 bytes | gabesullice |
#18 | interdiff-2936714-14-18.txt | 2.82 KB | gabesullice |
#18 | 2936714-18.tests_only.patch | 2.04 KB | gabesullice |
#18 | 2936714-18.patch | 2.77 KB | gabesullice |
Comments
Comment #2
gabesulliceComment #3
gabesulliceComment #4
gabesulliceComment #5
e0ipso@gabesullice why the change to Bug Report?
Comment #7
gabesullice@e0ipso, my reasoning was that with 8.5, entity definitions already have
isInternal
, but it's not possible to (effectively) callsetInternal(TRUE)
. Which feels like an oversight from #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts.I updated the IS to reflect that line of reasoning in the same update as the category change.
Comment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #9
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdding a few kind of related issues.
A thought on the proposed property in the entity definition: what happens to code API surface of the entity type? Things like update/insert hooks, alter hooks, the entity definition/storage/query objects are still part of the public api for an entity type as far a module developer is concerned. So it seems the names "api" and "internal" is slightly overloaded. Perhaps we can use "http_internal" or "rest_internal", or "rest_enabled" (defaulting to TRUE)?
Comment #11
gabesulliceIn short, nothing. Doing so would be conflating the idea of an 'internal' typed data type too closely with
@internal
as described by #2917276: Allow entity types to be @internal in such a way that removing them doesn't violate any BC. This issue is not about removing the entity type from "developer APIs" (my feeble attempt at qualifying the "API" term), it's just about making the data definition consistent with other typed data definitions.The issue which introduced the
isInternal()
method was titled "Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts." I believe all the same hooks/access/storage of an "internal" entity still apply just as with any other entity. The scope of this change is simply to opt the entity out of "normalization and other contexts," where "other contexts" generally means HTTP APIs.Unfortunately, yes 😔.
Let's try to use "developer API" for things like hooks, PHP interfaces, etc. Let's use "HTTP API" for things like REST, JSON API, GraphQL etc.
As for "internal", I think the scope of this issue needs to be kept as small as possible. We're trying to ensure that an entity TypedData definition returns a value from
isInternal()
that is conceptually consistent with other TypedData definitions liketext
orinteger
.I like where you're going with that! My preference would be a little more generic though. Maybe "data_type_internal", defaulting to FALSE?
Comment #12
Wim Leers#2936725: EntityDataDefinition::create() does not respect derived entity definitions landed, which means this is unblocked! :)
Comment #13
Wim LeersAgreed, but this is the consensus that was achieved with Entity, Field and Typed Data API maintainers at DrupalCon Vienna in #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts, after months of debating.
Which is why
DataDefinitionInterface
defines this to be more broad than just "HTTP internal":I've been using
and .Comment #14
gabesulliceComment #16
dawehnerI'm quite sure you actually want to use $entity_type->get('internal'). Entity keys are about which field is the bundle etc.
Comment #17
BerdirOr add a dedicated method for it, yes.
Comment #18
gabesulliceI accidentally included a different change in the last patch which I think is probably out-of-scope. This is what caused most of the errors I think.
Good catch.
I would love to do that, but I'm pretty sure that it would be a BC break.
Comment #19
BerdirEntityType/EntityTypeInterface is a 1:1 class:interface thing, you're expected to extend from that. We've added methods to that in the past, I think that would be fine.
See https://www.drupal.org/core/d8-bc-policy, "Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added."
Comment #21
gabesullice@Berdir thanks for pointing me to that. Good to know. This does feel more "right".
Comment #22
gabesulliceComment #23
Wim LeersLet's turn this into the label, by doing
'blah blah' => [NULL, FALSE]
Let's also add labels for the other provided cases?
❤️
Comment #26
gabesullicePretty sure last failure was due to the testbot.
Comment #28
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI don't think the entity *definition* is internal, but the whole entity type is. So how about changing this to "Indicates whether the entity type is internal."?
Putting this property as the first one in the list makes it look like it's very important, but it's not IMO so how about placing it between
$data_table
and$translatable
?Same argument about placement as above :)
Needs a ", FALSE otherwise." suffix, and the same point from above applies here as well, the entity type is internal, not its definition :)
Why is this needed?
Aren't we basically testing the same thing twice? :)
IMO we could keep just one test here, the one in
EntityTypeTest
."Provides *test* cases" maybe?
Comment #29
Wim LeersThis would make the patch at #2300677: JSON:API POST/PATCH support for fully validatable config entities smaller.
Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedDiscussed a bit with @gabesullice in Slack, and I have to say that I didn't read the previous comments (or the issue summary) before posting #28 :)
So point 5. is now clear for me, but points 1. and 4. still stand IMO, meaning that we need to explain better what exactly "internal entity definition" means.
Comment #31
BerdirI guess one problem with the documentation on internal is that it doesn't actually mean/do much on its own. It's just a flag for other things that integrate with the entity system.
There are also two different audiences... people defining an entity type that want to know if they should set it or not and people that implement modules that generically interact with entities
We can mention that it for example means that it will not be exposed by the rest.module as a rest resource. But for example flag.module might decide that it doesn't want to allow flags there or entity_reference might not allow it as a target selection in the UI. who knows :)
Comment #32
gabesullice#28.1 and #31: I tried my best to clarify the comment based on that Slack convo and also to reflect the fact that it's intentionally ambiguous. Open to suggestions if y'all think it can be clearer :) or not, maybe it's just perfect as it is.
#28.2/3/4: Finito!
#28.5: I know this was already cleared up, but for others benefit... because this is what DataDefinition::isInternal() uses to determine if the typed data definition is internal or not, which is the whole point :)
#28.6: One test is ensuring that EntityDeriver is correctly defining this on the definition. The other is ensuring that the annotation is correctly reflected by EntityType.
#28.7: Fixed.
Comment #34
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks @gabesullice, this looks much better to me now! Just a small nit:
We need to sneak in a comma somewhere in this sentence :) (hint: it's near the end of it)
Btw, there's no need to upload test-only patches in every comment if the tests didn't change since the previous patch. Also, if you upload the test-only patch first, the test bot will not kick the issue to NW.
Comment #35
gabesullice@amateescu, good to know! I just was in the habit of going through my CLI back scroll and repeating the same three commands.
Fixed the nit.
Updated the IS to reflect the addition to EntityTypeInterface.
Comment #36
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNice! This looks ready to me :)
Comment #37
alexpottAdding commit credit for patch reviews.
Comment #38
alexpottCommitted and pushed a3144f1326 to 8.6.x and c15775f348 to 8.5.x. Thanks!
Backported to 8.5.x since this is classed as a major bug and blocks other fixes.
Comment #41
Wim LeersWOOT! This also unblocks another important clean-up: #2843753: Prevent ContentModerationState from being exposed by REST's EntityResource introduced a work-around for the internal
ContentModerationState
entity type, but this issue (and its blockers) gave us the API we were missing back then. So now we can clean that up, and at the same time make the REST module use this API: #2941622: Make REST's EntityResource deriver exclude internal entity types and mark ContentModerationState entity type as internal instead of the current hack. 🎉Comment #43
Gábor HojtsyComment #44
Dave ReidDid this ever get a change record associated with it? Should it?