Introduce a sample value entity method to ease plugin filtering via context

1. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +namespace Drupal\Core\Entity;
   +
   +
   +use Drupal\Core\Config\Entity\ConfigEntityType;
   

   double blank lines

2. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +class EntityGenerator {
   

   Since this is a service, please provide an interface

3. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +   * EntityGenerate constructor.
   

   "Constructs a new EntityGenerator."

4. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   ...
   +  public function __construct(EntityTypeManagerInterface $entityTypeManager) {
   

   I don't think we're using lowerCamelCase in core like this yet

5. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +   * Generate a fieldable entity.
   ...
   +   * Generate a configuration entity.
   

   Generates

6. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +   *   The entity type id.
   ...
   +      // Bundle is already set, and id and revision should never be set.
   ...
   +   *   The entity type id.
   

   s/id/ID

7. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +   * @throws \Exception
   ...
   +      throw new \Exception("Chosen entity type is not fieldable. Use \Drupal\Core\Entity\EntityGenerator::generateConfigurationEntity() instead.");
   ...
   +   * @throws \Exception
   ...
   +      throw new \Exception("Chosen entity type is not a configuration entity. Use \Drupal\Core\Entity\EntityGenerator::generateFieldableEntity() instead.");
   ...
   +      throw new \Exception("Missing generator handler. This entity type cannot be generated.");
   

   This should be a classed exception. And don't end exception methods with a .

8. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +    $definition = $this->entityTypeManager->getDefinition($entity_type_id);
   +    if (!$definition instanceof ContentEntityType) {
   ...
   +    $definition = $this->entityTypeManager->getDefinition($entity_type_id);
   +    if (!$definition instanceof ConfigEntityType) {
   

   a) Usually we call this local variable $entity_type.
   b) Use something like $entity_type->entityClassImplements(FieldableEntityInterface::class) (or ConfigEntityInterface), because the current checks do not guarantee that.

9. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,117 @@
   +    /** @var \Drupal\Core\Entity\FieldableEntityInterface $entity */
   

   There is no $entity in this method. Also, not fieldable

10. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
    @@ -259,7 +259,7 @@ public function view($display_options = []) {
    -    $field_type_class = \Drupal::service('plugin.manager.field.field_type')->getPluginClass($field_definition->getType());
    +    $field_type_class = $field_definition->getItemDefinition()->getClass();
    

    This change is not immediately clear to me. Why does this help?

11. +++ b/core/modules/user/src/Entity/User.php
    @@ -498,6 +498,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +    $fields['timezone']->getItemDefinition()->setClass('\Drupal\user\TimeZoneItem');
    

    TimeZoneItem::class

12. +++ b/core/modules/user/src/TimeZoneItem.php
    @@ -0,0 +1,24 @@
    +    $keys = array_keys(system_time_zones());
    +    $count = count($keys);
    +    $key = rand(0, $count);
    +    return $keys[$key];
    

    Can we get a comment explaining this?

13. +++ b/core/modules/user/src/UserNameItem.php
    @@ -23,4 +24,13 @@ public function isEmpty() {
    +    $values['value'] = substr($values['value'], 0, 60);
    

    Any reason to hardcode 60? Is there a setting on the field definition that would be better to check? A comment would help.

14. +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/EntityGeneratorTest.php
    @@ -0,0 +1,88 @@
    + * Tests the EntityGenerator API.
    + *
    + * @group Entity
    + */
    +class EntityGeneratorTest extends KernelTestBase {
    

    @coversDefaultClass would be nice here, and @covers on the individual methods below.

15. +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/EntityGeneratorTest.php
    @@ -0,0 +1,88 @@
    +   * Modules to enable.
    +   *
    +   * @var array
    

    {@inheritdoc}

16. +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/EntityGeneratorTest.php
    @@ -0,0 +1,88 @@
    +    $node_type = NodeType::create(['type' => 'article', 'name' => 'Article']);
    +    $node_type->save();
    +    $node_type = NodeType::create(['type' => 'page', 'name' => 'Page']);
    +    $node_type->save();
    +    $vocabulary = Vocabulary::create(['name' => 'Tags', 'vid' => 'tags']);
    +    $vocabulary->save();
    

    Super nit, but you can chain the ->save

17. +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/EntityGeneratorTest.php
    @@ -0,0 +1,88 @@
    +            $entity = $this->entityGenerator->generateFieldableEntity($entity_type_id, $bundle->id());
    ...
    +          $entity = $this->entityGenerator->generateFieldableEntity($entity_type_id, $entity_type_id);
    

    This tests generateFieldableEntity, needs coverage for the config one.

    And also needs coverage for the thrown Exceptions.

    A coverage report of this would be telling. I don't see any GeneratorHandlers being used, even test ones.
    So far this seems more like a Unit test than a Kernel test?

1. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,95 @@
   +    $entityType = $this->entityTypeManager->getDefinition($entity_type_id);
   

   Nit: should be $entity_type

2. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,95 @@
   +      // Bundle is already set, and id and revision should never be set.
   

   s/id/ID

3. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,95 @@
   +    $definition = $this->entityTypeManager->getDefinition($entity_type_id);
   +    if (!$definition instanceof ConfigEntityType) {
   

   This should also be $entity_type not $definition, and should also use entityClassImplements as the method above does.

4. +++ b/core/lib/Drupal/Core/Entity/EntityGenerator.php
   @@ -0,0 +1,95 @@
   +      throw new EntityGeneratorMismatchException("Provided entity type \"%s\" is not a configuration entity. Use \Drupal\Core\Entity\EntityGenerator::generateFieldableEntity() instead", $entity_type_id);
   
   +++ b/core/lib/Drupal/Core/Entity/Exception/EntityGeneratorMismatchException.php
   @@ -0,0 +1,28 @@
   +    $message = sprintf($message, $entity_type_id);
   

   I'm not sure I've ever seen it done this way, most of the time I see the sprintf call where the exception is thrown.

   Even in the case of \Drupal\Component\Plugin\Exception\PluginNotFoundException, the sprintf is for the fallback.

   Just struck me as odd.

5. +++ b/core/lib/Drupal/Core/Entity/EntityGeneratorInterface.php
   @@ -0,0 +1,50 @@
   +   * @param array $values
   +   *   Any default values to use during generation.
   ...
   +  public function generateFieldableEntity($entity_type_id, $bundle, array $values = []);
   ...
   +   * @param array $values
   +   *   Any default values to use during generation.
   ...
   +  public function generateConfigurationEntity($entity_type_id, array $values = []);
   

   The oneline doc should start with (optional)

6. +++ b/core/lib/Drupal/Core/Entity/Exception/MissingEntityGeneratorHandlerException.php
   @@ -0,0 +1,27 @@
   +  public function __construct($entity_type_id) {
   +    parent::__construct(sprintf("Missing generator handler. The \"%s\" entity type cannot be generated", $entity_type_id));
   

   In this case I would mimic \Drupal\Component\Plugin\Exception\PluginNotFoundException, and allow an optional $message and use this sprintf as a fallback

7. +++ b/core/lib/Drupal/Core/Entity/GeneratorHandlerInterface.php
   @@ -0,0 +1,28 @@
   +   * @param array $values
   +   *   An array of values to include as part of the entity.
   ...
   +  public function generate(array $values = []);
   

   (optional)

8. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -259,7 +259,7 @@ public function view($display_options = []) {
   -    $field_type_class = \Drupal::service('plugin.manager.field.field_type')->getPluginClass($field_definition->getType());
   +    $field_type_class = $field_definition->getItemDefinition()->getClass();
   

   I still think this change deserves dedicated test coverage, that doesn't use the EG

The interdiffs look great!

From what I see in the patch, this is basically a "Entity::createWithSampleValues()", right? In which case, can't we simply call it that (or something similar) and put it in the storage handler?

Can't we use config schema to derive sample values for config entities on a best-effort basis? I guess what raises an alarm bell for me is the naming: generator is very broad and.. strong (can't really describe it better, sorry :)), so how about SampleValuesEntityGenerator or something like that?

Potentially do cool stuff like generate previews of an entity w/o the need to actually create and save an entity.

What do you mean by "a preview of an entity"?

Also, I'm wondering how does this help solve the issue linked in the issue summary?

This looks much better now.

Looks much better indeed!

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -90,6 +90,31 @@ protected function doCreate(array $values) {
   +  public function createWithSampleValues($bundle, array $values = []) {
   

   I'm not sure whether the $bundle parameter should exist or not, so let's hear more opinions on this one :)

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -90,6 +90,31 @@ protected function doCreate(array $values) {
   +    $bundle_key = $this->entityType->getKey('bundle');
   +    $values[$bundle_key] = $bundle;
   

   You have to take into account that an entity type might not provide a bundle key, in which case the value for the bundle is the ID of the entity type.

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -90,6 +90,31 @@ protected function doCreate(array $values) {
   +    // Fallback entity generation in case no custom generator is supplied.
   

   There is no fallback anymore ;)

4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -23,4 +23,21 @@
   +   * Generates an entity with sample field values.
   

   Creates an entity...

5. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -23,4 +23,21 @@
   +   * @throws \Drupal\Core\Entity\Exception\EntityNotFieldableException
   +   *   Thrown if the chosen entity type is not a content entity.
   

   This is not thrown anymore.

6. +++ b/core/modules/node/src/Entity/NodeType.php
   @@ -19,7 +19,7 @@
   + *     "list_builder" = "Drupal\node\NodeTypeListBuilder"
   

   This change is not needed anymore.

7. +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/EntityGeneratorTest.php
   @@ -0,0 +1,77 @@
   + * Tests the EntityGenerator API.
   ...
   +class EntityGeneratorTest extends KernelTestBase {
   ...
   +   * Tests generation of all enabled content entity types.
   ...
   +  public function testGenerateContentEntity() {
   ...
   +        // Generate entities with bundles.
   ...
   +        // Generate entities without bundles.
   

   These strings should be updated to not mention the generator approach anymore.

This looks much cleaner!

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -23,4 +23,21 @@
   +   * @throws \Drupal\Core\Entity\Exception\EntityNotFieldableException
   +   *   Thrown if the chosen entity type is not a content entity.
   

   I don't see this thrown anymore, figure it's leftover?

2. +++ b/core/modules/node/src/Entity/NodeType.php
   @@ -19,7 +19,7 @@
   - *     "list_builder" = "Drupal\node\NodeTypeListBuilder",
   + *     "list_builder" = "Drupal\node\NodeTypeListBuilder"
   

   Leftover change. Trailing commas are okay in annotations.

3. +++ b/core/modules/user/src/UserNameItem.php
   --- a/core/modules/user/tests/src/Kernel/UserEntityTest.php
   +++ b/core/modules/user/tests/src/Kernel/UserEntityTest.php
   
   +++ b/core/modules/user/tests/src/Kernel/UserEntityTest.php
   @@ -22,6 +22,14 @@ class UserEntityTest extends KernelTestBase {
   +    $this->installEntitySchema('user');
   

   Pretty incredible that this wasn't needed already!

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/EntityGeneratorTest.php
   @@ -0,0 +1,77 @@
   +      if ($definition instanceof ContentEntityType) {
   

   entityClassImplements() please

EDIT: Crosspost! Lots of similar feedback

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -90,6 +91,41 @@ protected function doCreate(array $values) {
   +  public function createWithSampleValues($bundle, array $values = []) {
   ...
   +    $bundle_key = $this->entityType->getKey('bundle');
   +    $values[$bundle_key] = $bundle;
   ...
   +      $bundle_key
   

   You also need to take into account that an entity type might not have bundles (e.g. User), so the first method argument should be optional and $bundle_key needs to be added to the forbidden keys only if it exists.

   Which makes me wonder, why do we forbid passing an entity ID and revision ID in the $values array?

   Entity types with string IDs will not have an ID generated automatically by the DB layer, so we need to allow passing an initial ID for them.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -90,6 +91,41 @@ protected function doCreate(array $values) {
   +    $bundle_type = $this->entityType->getBundleEntityType();
   +    if (!$bundle_type && $bundle != $this->entityTypeId) {
   +      $bundle = $this->entityTypeId;
   +    }
   +    if ($bundle_type) {
   +      $bundle_type = $this->entityManager->getStorage($bundle_type);
   +      $bundle_entity = $bundle_type->load($bundle);
   +      if (!$bundle_entity) {
   +        throw new MissingEntityBundleException($bundle);
   +      }
   +    }
   

   This is too complicated and not really correct because not all bundles are managed through a config entity type. You should use getBundleInfo() method from the entity_type.bundle.info service, and throw a regular EntityStorageException like \Drupal\Core\Entity\ContentEntityStorageBase::doCreate().

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -23,4 +23,21 @@
   +   * @throws \Drupal\Core\Entity\Exception\MissingEntityBundleException
   +   *   Thrown if the requested entity bundle does not exist.
   

   As mentioned above, this needs to be a EntityStorageException.

4. +++ b/core/modules/user/tests/src/Kernel/UserEntityTest.php
   @@ -65,4 +73,22 @@ public function testUserMethods() {
   +    $this->assertFalse((bool) $violations->count());
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/CreateSampleEntityTest.php
   @@ -0,0 +1,77 @@
   +            $this->assertEquals(0, $violations->count());
   ...
   +          $this->assertEquals(0, $violations->count());
   

   We can use assertCount() here.

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/CreateSampleEntityTest.php
   @@ -0,0 +1,77 @@
   +        // Create sample entities without bundles.
   +        else {
   +          $entity = $this->entityTypeManager->getStorage($entity_type_id)->createWithSampleValues($entity_type_id);
   

   This shows the problem with having $bundle as an explicit argument, it forces the caller to know that an entity type doesn't use bundles and guess that it can use the entity type ID for the bundle name.

   Also, the caller can also be wrong in assuming that an entity doesn't have bundles, since the entity type definition can be altered by any other module to add bundle support for that entity type.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -43,12 +50,15 @@
   -  public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, CacheBackendInterface $cache) {
   +  public function __construct(EntityTypeInterface $entity_type, EntityManagerInterface $entity_manager, CacheBackendInterface $cache, EntityTypeBundleInfoInterface $entity_type_bundle_info) {
   
   +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -159,11 +161,13 @@ public function getFieldStorageDefinitions() {
   -  public function __construct(EntityTypeInterface $entity_type, Connection $database, EntityManagerInterface $entity_manager, CacheBackendInterface $cache, LanguageManagerInterface $language_manager) {
   -    parent::__construct($entity_type, $entity_manager, $cache);
   +  public function __construct(EntityTypeInterface $entity_type, Connection $database, EntityManagerInterface $entity_manager, CacheBackendInterface $cache, EntityTypeBundleInfoInterface $entity_type_bundle_info, LanguageManagerInterface $language_manager) {
   +    parent::__construct($entity_type, $entity_manager, $cache, $entity_type_bundle_info);
   

   These changes will force every overridden content storage handler to require an update, including contrib and custom ones, so I think we're better off using getBundleInfo() from the deprecated entity manager service, since we already have that injected.

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/CreateSampleEntityTest.php
   @@ -0,0 +1,77 @@
   +  public function testGenerateContentEntity() {
   

   This method still uses the old "generate" naming.

And.. last but not least, we need to change the issue title :)

I think it looks perfect now! Except for this small nit:

+++ b/core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
@@ -93,6 +93,13 @@ class SqlContentEntityStorageTest extends UnitTestCase {
+   * The entity type bundle information object.
+   *
+   * @var \Drupal\Core\Entity\EntityTypeBundleInfoInterface|\PHPUnit_Framework_MockObject_MockObject
+   */
+  protected $entityTypeBundleInfo;

This leftover from the previous patch should be removed.

Really sorry for yet another nitpick review, I found two more things on a second pass :/

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
   @@ -23,4 +23,21 @@
   +   * @throws \Drupal\Core\Entity\Exception\MissingEntityBundleException
   

   This is not the exception that's being thrown anymore.

2. +++ b/core/modules/user/tests/src/Kernel/UserEntityTest.php
   --- /dev/null
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/CreateSampleEntityTest.php
   
   +++ b/core/tests/Drupal/KernelTests/Core/Entity/Element/CreateSampleEntityTest.php
   @@ -0,0 +1,77 @@
   +namespace Drupal\KernelTests\Core\Entity\Element;
   

   This is a weird namespace for this test, why is it in the Element subdirectory?

Perfect :)

Updating issue credits to include @tim.plunkett and @amateescu for their reviews.

We need a change record here.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -90,6 +90,40 @@ protected function doCreate(array $values) {
   +      if (!in_array($field_name, $forbidden_keys)) {
   

   nit: these are strings, we should use the third argument

2. +++ b/core/modules/user/src/TimeZoneItem.php
   @@ -0,0 +1,24 @@
   +    $key = rand(0, count($timezones));
   

   If this yields the maximum, we'll get an error here?

   e.g. if keys are [0, 1, 2]

   count() is 3.

   $timezones[3] does not exist.

   php.net/rand tells me that rand is inclusive of the maximum

3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/CreateSampleEntityTest.php
   @@ -0,0 +1,77 @@
   +        // Create sample entities with bundles.
   +        if ($bundle_type = $definition->getBundleEntityType()) {
   +          foreach ($this->entityTypeManager->getStorage($bundle_type)->loadMultiple() as $bundle) {
   +            $entity = $this->entityTypeManager->getStorage($entity_type_id)->createWithSampleValues($bundle->id());
   +            $violations = $entity->validate();
   +            $this->assertCount(0, $violations);
   +          }
   +        }
   +        // Create sample entities without bundles.
   +        else {
   +          $entity = $this->entityTypeManager->getStorage($entity_type_id)->createWithSampleValues();
   +          $violations = $entity->validate();
   

   We don't have any coverage for passing the second argument - $values

The nitpick police strikes again:

+++ b/core/modules/user/src/TimeZoneItem.php
@@ -17,7 +17,7 @@ class TimeZoneItem extends StringItem {
+    $key = rand(0, count($timezones)-1);

Missing spaces before and after '-' here :D

And we still need the change record requested in #34.

Cool, let's ship it :)

updating issue credits, adding myself for catching the off by one error

Committed as dff09a9 and pushed to 8.5.x.

Published the change record.

Tagging for 8.5.x release notes.

Great to have this capability!

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageInterface.php
@@ -23,4 +23,21 @@
+  public function createWithSampleValues($bundle = FALSE, array $values = []);

Devil's advocate: why is this not a BC break?

Devil's advocate: why is this not a BC break?

Two months ago, I said the same thing, but then I was pointed to this bit in our BC policy.

Interfaces within non-experimental, non-test modules not tagged with either @api or @internal
Interfaces that are not tagged with either @api or @internal can be safely used as type hints. No methods will be changed or removed from these interface in a breaking way.
However, we reserve the ability to add methods to these interfaces in minor releases to support new features. When implementing the interface, module authors are encouraged to either:

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.
Where the interface is implemented directly for other reasons, be prepared to add support for new methods in minor releases

I don't think there's anything here that site owners must know when upgrading, so switching to the highlights tag.