Move getConfigPrefix() to ConfigEntityTypeInterface

+++ b/core/lib/Drupal/Core/Config/ConfigManager.php
@@ -63,7 +64,7 @@ public function __construct(EntityManagerInterface $entity_manager, ConfigFactor
   public function getEntityTypeIdByName($name) {
     $entities = array_filter($this->entityManager->getDefinitions(), function (EntityTypeInterface $entity_type) use ($name) {
-      return ($config_prefix = $entity_type->getConfigPrefix()) && strpos($name, $config_prefix . '.') === 0;
+      return ($entity_type instanceof ConfigEntityTypeInterface && $config_prefix = $entity_type->getConfigPrefix()) && strpos($name, $config_prefix . '.') === 0;
     });

It looks like this would be easier with #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() ?

Shall we postpone this issue on that one?

Can we fix the naming and docs while we're at it, or does that need to be a separate issue?

In #2226437: Create ContentEntityTypeInterface I created a ContentEntityTypeInterface and made it extend the EntityTypeInterface.

Shouldn't ConfigEntityTypeInterface also extend EntityTypeInterface?

Good point, yes it should.

alright, working on it, will post a new patch. This one needs a reroll so will do that too.

woops, forgot a use statement... this one should pass.

Awesome, thanks @mauzeh. I just have some minor points.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -12,10 +12,17 @@
   -   * Returns the config prefix used by the configuration entity type.
   +   * The config prefix set in the configuration entity type annotation.
   

   We have a standard of documenting functions starting with a third person verb so the previous one was actually correct.

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -12,10 +12,17 @@
   +   * The default configuration prefix is constructed from the name of the module
   +   * that provides the entity type and the ID of the entity type. If a
   +   * config_prefix annotation is present it will be used in place of the entity
   +   * type ID.
   

   Awesome that you added this documentation!

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -40,11 +47,6 @@ public function getConfigPrefix() {
   -    // Ensure that all configuration entities are prefixed by the module that
   -    // provides the configuration entity type. This ensures that default
   -    // configuration will be created as expected during module install and
   -    // dependencies can be calculated without the modules that provide the
   -    // entity types being installed.
   

   This is now sort of duplicated above, but I think it couldn't hurt to leave this in.

4. +++ b/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigEntityMapperTest.php
   @@ -48,6 +48,12 @@ class ConfigEntityMapperTest extends UnitTestCase {
      /**
   +   * The entity used for testing.
   +   *
   +   * @var \Drupal\Core\Config\Entity\ConfigEntityType|\PHPUnit_Framework_MockObject_MockObject
   +   */
   +
   

   I don't see any entity :-) ?! Also it should be "entity *type*" I think.

5. +++ b/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigEntityMapperTest.php
   @@ -93,6 +99,10 @@ public function setUp() {
   +    $this->entityType = $this->getMockBuilder('Drupal\Core\Config\Entity\ConfigEntityType')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   

   Minor, but due to this issue you can do

   $this->entityType = $this->getMock('Drupal\Core\Config\Entity\ConfigEntityTypeInterface')

   and therefore avoid the disable...() call.

Oops, #13.1 was actually documenting a variable not a method, so the change is correct.

Fixes the rest. Also re-includes the actual interface and the test for ConfigEntityType that was accidentally left out of the previous patch.

Ahh, now I know which use statement @mauzeh was talking about in #11.

We also already have ConfigEntityTest in 8.x now, so moving the test methods there. And fixing one method that needed updating after renaming the entity storage classes.

SQLSTATE[42S02]: Base table or view not found: 1146 Table [error]
'drupaltestbotmysql.user__user_picture' doesn't exist: SELECT t.*
FROM
{user__user_picture} t
WHERE (entity_id IN (:db_condition_placeholder_0)) AND (deleted =
:db_condition_placeholder_1) AND (langcode IN
(:db_condition_placeholder_2, :db_condition_placeholder_3,
:db_condition_placeholder_4))
ORDER BY delta ASC; Array
(
[:db_condition_placeholder_0] => 1
[:db_condition_placeholder_1] => 0
[:db_condition_placeholder_2] => en
[:db_condition_placeholder_3] => und
[:db_condition_placeholder_4] => zxx
)].

Don't see how that would be caused by this. Retesting

16: 2204697-16-config-entity-type-interface.patch queued for re-testing.

tstoeckler-- for the time it took me to figure this out... Let's see.

Because of the missing use statement the array_filter callback in getEntityTypeIdByName() was never returning TRUE. Thus the check whether a config object is in fact a config entity in ConfigInstaller failed and fields were being saved as dumb config objects and thus the tables were never created. Fun! :-)

...

+++ b/core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
@@ -88,4 +88,77 @@ public function testConfigPrefixLengthValid() {
+  /**
+   * Tests the getBaseTable() method.
+   *
+   * @covers ::getBaseTable()
+   */
+  public function testGetBaseTable() {
+    $entity_type = $this->setUpConfigEntityType();
+    $this->assertFalse($entity_type->getBaseTable());
+  }
+
+  /**
+   * Tests the getRevisionDataTable() method.
+   *
+   * @covers ::getRevisionDataTable()
+   */
+  public function testGetRevisionDataTable() {
+    $entity_type = $this->setUpConfigEntityType();
+    $this->assertFalse($entity_type->getRevisionDataTable());
+  }
+
+  /**
+   * Tests the getRevisionTable() method.
+   *
+   * @covers ::getRevisionTable()
+   */
+  public function testGetRevisionTable() {
+    $entity_type = $this->setUpConfigEntityType();
+    $this->assertFalse($entity_type->getRevisionTable());
+  }
+
+  /**
+   * Tests the getDataTable() method.
+   *
+   * @covers ::getDataTable()
+   */
+  public function testGetDataTable() {
+    $entity_type = $this->setUpConfigEntityType();
+    $this->assertFalse($entity_type->getDataTable());
+  }

Why would we want to add tests for those here?

Still confused about that...

Can you explain your confusion? This just increases the test coverage of the ConfigEntityType class. Nothing else. It's not technically related to this issue but it seemed easy enough to do here so I thought why not?

@tstoeckler, because it's not in scope and that makes the patch harder to review/commit. :) Might be good to spin off a separate issue "Increase test coverage for ConfigEntityType" with that part of the patch?

It adds test coverage for methods that have nothing to do with config entities and there's an issue to remove them completely, so why add it here and why add it at all? :)

Looks good to me.

It is a small API change, but having this interface is very useful and the only way it breaks if someone is calling getConfigPrefix() unconditionally on all entity types. I assume that outside of core, there are very few reasons to call that method at all.

This needs a change record. Thanks!

Thanks @alexpott.

I made the following updates:
https://www.drupal.org/node/2181667/revisions/view/6879507/8418563

So, per @alexpott, #2430219: Implement a key value store to optimise config entity lookups is now critical, and this issue is also now blocking #2432791: Skip Config::save schema validation of config data for trusted data, which is also critical. I asked @alexpott if it was a hard blocker, and he pointed out that it's increasingly fragile to have these various config-specific methods on EntityInterface but returning FALSE or NULL or whatnot for content entity types. Therefore, elevating this to critical.

1. I was going to suggest a followup issue to replace instanceof ConfigEntityType checks, but I see @Berdir is already looking at #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() which will address a related problem space.

2. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -102,7 +103,7 @@ public function __construct(EntityManagerInterface $entity_manager, ConfigFactor
      public function getEntityTypeIdByName($name) {
        $entities = array_filter($this->entityManager->getDefinitions(), function (EntityTypeInterface $entity_type) use ($name) {
   -      return ($config_prefix = $entity_type->getConfigPrefix()) && strpos($name, $config_prefix . '.') === 0;
   +      return ($entity_type instanceof ConfigEntityTypeInterface && $config_prefix = $entity_type->getConfigPrefix()) && strpos($name, $config_prefix . '.') === 0;
        });
        return key($entities);
      }
   

   Hmm. So if the entity type is an instance of ConfigEntityTypeInterface, but getConfigPrefix() returns something that is false, or the name doesn't begin with the config prefix expected... then what happens?

   This isn't introduced by this patch, but it seems like now this is a set of things that would mean the entity type were malformed if the first were true but not one of the others. I think. Can we get a followup to at least add an inline comment to document this, and maybe investigate whether this line is something that should be refactored?

3. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityType.php
   @@ -15,44 +15,17 @@
   +   * The default configuration prefix is constructed from the name of the module
   +   * that provides the entity type and the ID of the entity type. If a
   +   * config_prefix annotation is present it will be used in place of the entity
   +   * type ID.
   
   +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
   @@ -0,0 +1,64 @@
   +   * - The config prefix, which is returned by getConfigPrefix() and is
   +   *   composed of:
   +   *   - The provider module name (limited to 50 characters by
   +   *     DRUPAL_EXTENSION_NAME_MAX_LENGTH).
   +   *   - The module-specific namespace identifier, which defaults to the
   +   *     configuration entity type ID. Entity type IDs are limited to 32
   +   *     characters by EntityTypeInterface::ID_MAX_LENGTH.
   ...
   +   * Ensures that all configuration entities are prefixed by the module that
   +   * provides the configuration entity type. This ensures that if a
   +   * configuration entity is contained in a extension's default configuration it
   +   * be created during extension installation. Additionally, it allows
   +   * dependencies to be calculated without the modules that provide
   +   * configuration entity types being installed.
   

   So the first and third hunks of documentation are added by this patch, and the first is what we have in HEAD. That means we're explaining what a config prefix is composed of in why in different ways in three different places. We should consolidate that and clarify it.

   Normally, since these added lines of documentation are the majority of non-moved lines in the patch, I'd ask to fix this before it went in. However, @alexpott said that this issue is now blocking two criticals, so I'm okay for that to be done in a followup instead.

   I did fix a small bit of pirate speak on commit:

   diff --git a/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
   index 4b0fd15..a24c96f 100644
   --- a/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
   +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityTypeInterface.php
   @@ -51,8 +51,8 @@
       *
       * Ensures that all configuration entities are prefixed by the module that
       * provides the configuration entity type. This ensures that if a
   -   * configuration entity is contained in a extension's default configuration it
   -   * be created during extension installation. Additionally, it allows
   +   * configuration entity is contained in a extension's default configuration,
   +   * it will be created during extension installation. Additionally, it allows
       * dependencies to be calculated without the modules that provide
       * configuration entity types being installed.

   (Ensures it be created, arrr.)

Committed and pushed to 8.0.x. Thanks!

This has broken #2409677: add a method to EntityTypeInterface to determine whether an entity type is content or config. It also affects contrib -- AFAIK, calling getConfigPrefix() was the simplest way to determine if a given entity is a config or content entity.

Well, now the simplest way is instanceof ConfigEntityTypeInterface. Which is way more explicit.

The official way to do that so far was isSubclassOf(), wich I want to deprecate in favor of the instanceof check.

@joachim, core also does instanceof ConfigEntityType in a few places, which (per #41) need to be converted to instanceof ConfigEntityTypeInterface. See, for example, EntityManager::loadEntityByConfigTarget().

In #45 @Berdir is referring to #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements().

Published the change record. Config-specific entity type functionality moved to a new ConfigEntityTypeInterface