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"

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

Title: [PP-1] Allow entity types to be marked 'internal' » [PP-1] Entity types cannot be marked 'internal'
Category: Feature request » Bug report
Issue summary: View changes
gabesullice’s picture

Title: [PP-1] Entity types cannot be marked 'internal' » [PP-1] Entity type definitions cannot be set as 'internal'
e0ipso’s picture

@gabesullice why the change to Bug Report?

Version: 8.5.x-dev » 8.6.x-dev

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

gabesullice’s picture

@e0ipso, my reasoning was that with 8.5, entity definitions already have isInternal, but it's not possible to (effectively) call setInternal(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.

Sam152’s picture

Sam152’s picture

Sam152’s picture

Adding 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)?

gabesullice’s picture

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.

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

So it seems the names "api" and "internal" is slightly overloaded.

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 like text or integer.

Perhaps we can use "http_internal" or "rest_internal", or "rest_enabled" (defaulting to TRUE)

I like where you're going with that! My preference would be a little more generic though. Maybe "data_type_internal", defaulting to FALSE?

Wim Leers’s picture

Title: [PP-1] Entity type definitions cannot be set as 'internal' » Entity type definitions cannot be set as 'internal'
Status: Postponed » Active
Wim Leers’s picture

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

Agreed, 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":

  /**
   * Determines whether the data value is internal.
   *
   * This can be used in a scenario when it is not desirable to expose this data
   * value to an external system.
   *
   * @return bool
   *   Whether the data value is internal.
   */
  public function isInternal();

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.

I've been using PHP API and HTTP API.

gabesullice’s picture

Status: Needs review » Needs work

The last submitted patch, 14: 2936714-14.patch, failed testing. View results

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
@@ -89,6 +89,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
         'constraints' => $entity_type->getConstraints(),
+        'internal' => $entity_type->getKey('internal'),

I'm quite sure you actually want to use $entity_type->get('internal'). Entity keys are about which field is the bundle etc.

Berdir’s picture

Or add a dedicated method for it, yes.

gabesullice’s picture

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

I'm quite sure you actually want to use $entity_type->get('internal').

Good catch.

Or add a dedicated method for it, yes.

I would love to do that, but I'm pretty sure that it would be a BC break.

Berdir’s picture

EntityType/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."

Status: Needs review » Needs work

The last submitted patch, 18: 2936714-18.tests_only.patch, failed testing. View results

gabesullice’s picture

@Berdir thanks for pointing me to that. Good to know. This does feel more "right".

gabesullice’s picture

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

  1. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php
    @@ -138,4 +140,37 @@ public function testEntityReferences() {
    +      // Test entity types which have not defined a value for 'internal'.
    +      [NULL, FALSE],
    

    Let's turn this into the label, by doing
    'blah blah' => [NULL, FALSE]

    Let's also add labels for the other provided cases?

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
    @@ -127,6 +127,18 @@ public function providerTestGetKeys() {
    +    $entity_type = $this->setUpEntityType(['internal' => TRUE]);
    +    $this->assertTrue($entity_type->isInternal());
    +    $entity_type = $this->setUpEntityType(['internal' => FALSE]);
    +    $this->assertFalse($entity_type->isInternal());
    +    $entity_type = $this->setUpEntityType([]);
    +    $this->assertFalse($entity_type->isInternal());
    

    ❤️

The last submitted patch, 21: 2936714-21.tests_only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 21: 2936714-21.patch, failed testing. View results

gabesullice’s picture

Pretty sure last failure was due to the testbot.

Status: Needs review » Needs work

The last submitted patch, 26: 2936714-26.tests_only.patch, failed testing. View results

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -17,6 +17,13 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
    +   * Indicates whether the entity definition should be internal.
    

    I 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."?

  2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -17,6 +17,13 @@ class EntityType extends PluginDefinition implements EntityTypeInterface {
    +  protected $internal = FALSE;
    

    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?

  3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
    @@ -354,6 +361,13 @@ public function set($property, $value) {
    +  public function isInternal() {
    
    +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -116,6 +116,16 @@ public function getKey($key);
    +  public function isInternal();
    

    Same argument about placement as above :)

  4. +++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
    @@ -116,6 +116,16 @@ public function getKey($key);
    +   *   TRUE if the entity definition should be internal.
    

    Needs a ", FALSE otherwise." suffix, and the same point from above applies here as well, the entity type is internal, not its definition :)

  5. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/Deriver/EntityDeriver.php
    @@ -89,6 +89,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
    +        'internal' => $entity_type->isInternal(),
    

    Why is this needed?

  6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php
    @@ -138,4 +140,36 @@ public function testEntityReferences() {
    +  public function testEntityDefinitionIsInternal($internal, $expected) {
    ...
    +  public function entityDefinitionIsInternalProvider() {
    
    +++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
    @@ -127,6 +127,18 @@ public function providerTestGetKeys() {
    +  public function testIsInternal() {
    

    Aren't we basically testing the same thing twice? :)

    IMO we could keep just one test here, the one in EntityTypeTest.

  7. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php
    @@ -138,4 +140,36 @@ public function testEntityReferences() {
    +   * Provides cases for testEntityDefinitionIsInternal.
    

    "Provides *test* cases" maybe?

Wim Leers’s picture

amateescu’s picture

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

Berdir’s picture

I 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 :)

gabesullice’s picture

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

Status: Needs review » Needs work

The last submitted patch, 32: 2936714-32.tests_only.patch, failed testing. View results

amateescu’s picture

Thanks @gabesullice, this looks much better to me now! Just a small nit:

+++ b/core/lib/Drupal/Core/Entity/EntityTypeInterface.php
@@ -562,6 +562,24 @@ public function getBundleLabel();
+   *   TRUE if the entity data is internal. FALSE otherwise.

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.

gabesullice’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
532 bytes
5.47 KB

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

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Nice! This looks ready to me :)

alexpott’s picture

Adding commit credit for patch reviews.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed a3144f1 on 8.6.x
    Issue #2936714 by gabesullice, Wim Leers, amateescu, Sam152, Berdir:...

  • alexpott committed c15775f on 8.5.x
    Issue #2936714 by gabesullice, Wim Leers, amateescu, Sam152, Berdir:...
Wim Leers’s picture

WOOT! 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. 🎉

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Version: 8.6.x-dev » 8.5.x-dev
Dave Reid’s picture

Did this ever get a change record associated with it? Should it?