EntityDataDefinition::create() does not respect derived entity definitions

Can we get a failing test-only patch?

1. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
   @@ -18,13 +18,25 @@ class EntityDataDefinition extends ComplexDataDefinitionBase implements EntityDa
   -  public static function create($entity_type_id = NULL) {
   ...
   +  public static function create($entity_type_id = NULL, $bundle = NULL) {
   

   Isn't this a violation of the interface?

   My guess is that it's not, because \Drupal\Core\Field\TypedData\FieldItemDataDefinition::create() goes much further still: rather than receiving a string as its parent method \Drupal\Core\TypedData\DataDefinition::create() dictates, it receives an object, which can contain arbitrary complexity!

   Oh, and in fact, \Drupal\Core\Entity\TypedData\EntityDataDefinition::createFromDataType() already supports this! So ::createFromDataType() supports entity type + bundle, it's just ::create() that doesn't.

2. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
   @@ -18,13 +18,25 @@ class EntityDataDefinition extends ComplexDataDefinitionBase implements EntityDa
   +      // If the definition doesn't exist, just create from defaults.
   +      $values = \Drupal::typedDataManager()->getDefinition($data_type, FALSE);
   

   This is where the updated code does use \Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver::getDerivativeDefinitions(), correct?

3. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
   @@ -35,13 +47,15 @@ public static function createFromDataType($data_type) {
   +    $entity_type_id = isset($parts[1]) ? $parts[1] : NULL;
   +    $bundle = isset($parts[2]) ? $parts[2] : NULL;
   +    $definition = static::create($entity_type_id, $bundle);
        // Set the passed entity type and bundle.
   -    if (isset($parts[1])) {
   -      $definition->setEntityTypeId($parts[1]);
   +    if ($entity_type_id) {
   +      $definition->setEntityTypeId($entity_type_id);
        }
   -    if (isset($parts[2])) {
   -      $definition->setBundles([$parts[2]]);
   +    if ($bundle) {
   +      $definition->setBundles([$bundle]);
   

   AFAICT these changes are unnecessary?

#12.3: right, I see it now!

1. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
   @@ -18,13 +18,25 @@ class EntityDataDefinition extends ComplexDataDefinitionBase implements EntityDa
   +      // If a bundle was given, use the bundle-specific definition.
   +      $data_type = isset($bundle) ?
   +        sprintf('entity:%s:%s', $entity_type_id, $bundle) :
   +        sprintf('entity:%s', $entity_type_id);
   

   This might be slightly shorter than the following:

   <?php
   $data_type = 'entity:' . $entity_type_id;
   if ($bundle) {
   $data_type .= ':' . $bundle;
   }

   But I personally find that more readable and I don't like multi-line if/?: statements like that. But not feeling strongly about that.

   Also doesn't really need isset() on $entity_type_id/$bundle, it can't be not defined.

2. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
   @@ -35,13 +47,15 @@ public static function createFromDataType($data_type) {
   -    if (isset($parts[1])) {
   -      $definition->setEntityTypeId($parts[1]);
   +    if ($entity_type_id) {
   +      $definition->setEntityTypeId($entity_type_id);
        }
   -    if (isset($parts[2])) {
   -      $definition->setBundles([$parts[2]]);
   +    if ($bundle) {
   +      $definition->setBundles([$bundle]);
   

   entity type is already set in the create() call and we could/should also set bundles there, then this can all go away. Maybe it's already set through $values?

3. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -93,8 +93,10 @@ protected function setUp() {
          ->method('getDefinition')
   -      ->with($this->equalTo('field_item:string_long'))
   -      ->willReturn(['class' => '\Drupal\Core\Field\Plugin\Field\FieldType\StringLongItem']);
   +      ->will($this->returnValueMap([
   +        'entity:user' => $this->getMock('Drupal\Core\TypedData\DataDefinitionInterface'),
   +        'field_item:string_long' => ['class' => '\Drupal\Core\Field\Plugin\Field\FieldType\StringLongItem'],
   +      ]));
   

   Why does one call return an object and the other an array? doesn't the mock object just get ignored due to the is_array($values) call?

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php
   @@ -82,10 +89,15 @@ public function testFields() {
    
   +    // Entity definitions should inherit their labels from the entity type.
   +    $this->assertEqual($entity_definition->getLabel(), \Drupal::entityTypeManager()->getDefinition('node')->getLabel());
   +    $this->assertEqual($bundle_definition->getLabel(), \Drupal::service('entity_type.bundle.info')->getBundleInfo('node')['article']['label']);
   +
   

   lets use the non-deprecated assertEquals() which expects the expected value first.

   Might also be easier to just hardcode the expected label, makes tests easier to read IMHO.

1. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
   @@ -18,13 +18,32 @@ class EntityDataDefinition extends ComplexDataDefinitionBase implements EntityDa
   +      // If the definition doesn't exist, just create from defaults.
   ...
   +      // Set the passed entity type.
   ...
   +      // Set the passed bundle, if it was given.
   

   These two comments seems to be a bit pointless, given they describe the code exactly, or is it the code describing the comment ... :) For the first case we could describer rather, why the definition might not exist. In the other cases we could describe that for those cases it doesn't exist, we need to set the entity type explicitly.

2. +++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
   @@ -35,15 +54,10 @@ public static function createFromDataType($data_type) {
   -    $definition = static::create();
   -    // Set the passed entity type and bundle.
   -    if (isset($parts[1])) {
   -      $definition->setEntityTypeId($parts[1]);
   -    }
   -    if (isset($parts[2])) {
   -      $definition->setBundles([$parts[2]]);
   -    }
   -    return $definition;
   +    return static::create(
   +      isset($parts[1]) ? $parts[1] : NULL,
   +      isset($parts[2]) ? $parts[2] : NULL
   +    );
   

   Nice!

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php
   @@ -31,10 +32,16 @@ class EntityTypedDataDefinitionTest extends KernelTestBase {
   -  public static $modules = ['filter', 'text', 'node', 'user'];
   +  public static $modules = ['system', 'filter', 'text', 'node', 'user'];
   ...
   +    NodeType::create([
   +      'type' => 'article',
   +      'name' => 'Article',
   +    ])->save();
   
   @@ -82,10 +89,15 @@ public function testFields() {
    
   +    // Entity definitions should inherit their labels from the entity type.
   +    $this->assertEquals('Content', $entity_definition->getLabel());
   +    $this->assertEquals('Article', $bundle_definition->getLabel());
   

   IMHO it would be nice trying to avoid adding "node" specific test code. What about using \Drupal\entity_test\Entity\EntityTestWithBundle|\Drupal\entity_test\Entity\EntityTestBundle instead?

+++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
@@ -28,7 +28,10 @@
 
-      // Load definitions created by the EntityDeriver.
+      // It's possible that the given entity type ID or bundle wasn't discovered
+      // by the TypedData plugin manager and/or weren't created by the
+      // EntityDeriver. In that case, this is a new definition and we'll just
+      // create the definition from defaults by using an empty array.
       $values = \Drupal::typedDataManager()->getDefinition($data_type, FALSE);
       $definition = new static(is_array($values) ? $values : []);

This is really nice!

+++ b/core/lib/Drupal/Core/Entity/TypedData/EntityDataDefinition.php
@@ -35,15 +57,10 @@ public static function createFromDataType($data_type) {
+      isset($parts[1]) ? $parts[1] : NULL,
+      isset($parts[2]) ? $parts[2] : NULL

if only we could use php7's null coalesce operator and/or splat operator!

Adding review credits

Committed b2b7f46 and pushed to 8.6.x.

Cherry-picked as 8525135 and pushed to 8.5.x

Thanks folks

Woot, this means #2936725: EntityDataDefinition::create() does not respect derived entity definitions is unblocked! This also means there's one less blocker for #2932035: ResourceTypes should be internal when EntityType::isInternal is TRUE, which is a blocker for JSON API to get into 8.6.x.

Note that the successor #2936714: Entity type definitions cannot be set as 'internal' also landed, and we're now down to the final core issue: #2941622: Make REST's EntityResource deriver exclude internal entity types and mark ContentModerationState entity type as internal instead of the current hack!