Replace ContentEntityDatabaseStorage::getSchema() with a new EntityTypeListenerInterface implemented by SqlContentEntityStorageSchema

Quoting a related conversation @fago and me had on the parent issue:

(fago)

[...] should we continue routing change notification calls through the storage, such that a non-SQL storage could implement its own of storage adaptions as needed? [...]

(plach)

This is an area I spent quite some time thinking around: IMO ideally all these notifications should be routed through the entity manager, which in turn would be responsible for notifying interested entity handlers (probably only storage) and for proxying the notifications to other interested business objects, such as the schema manager. Among the rest this would allow the EM to clear its own caches as discussed with @yched in #2144263-87: Decouple entity field storage from configurable fields.

@eff:

I did not want to address this in the parent issue as it felt a bit out of scope, but this issue here seems appropriate. What do you think?

+++ b/core/lib/Drupal/Core/Entity/Schema/ContentEntitySchemaHandler.php
@@ -53,22 +61,69 @@ class ContentEntitySchemaHandler implements EntitySchemaHandlerInterface {
+  public function updateEntitySchema(ContentEntityTypeInterface $entity_type, ContentEntityTypeInterface $original) {
+    // @todo Implement proper updates: https://www.drupal.org/node/1498720
+    $this->createEntitySchema($entity_type);
+  }

What about dropping the schema before (re)creating it?

@#5 That would break my expectation of what a createEntitySchema would do and is potentially dangerous. If I told the API to create Entity schema for something, and it finds that the thing already exists, and then decides to destroy it and create a new one, I could potentially be very said if it also blew away all the data for that thing.

It would be far better if there was a dropAndCreateEntitySchema for that action if it is needed.

That line refers to the updateEntitySchema() method, which is only a thin temporary implementation wrt what is actually implemented in the parent issue. I updated #5 to make it more clear.

Not sure whether I interpret the summary correctly, but the intention appears to be closely related that issue.

Ah I see. For many frameworks "migrating" from one schema to another has been a source of trouble.

However, it still seems dangerous to drop the schema in every case of an update. Please correct me if I'm wrong, but wouldn't that destroy the data the schema has stored?

Consider this use case:

I have a site in production and I don't know any better so I run my updates and included in this patch is some schema change. And I don't know any better so I just run them. Then I find out that all of my content of a specific type are gone.

Or this use case:

I just want to add a field to my content type, and I know better, so I try to find ways around using the standard ways of doing this so I can keep the data I already have.

Are my concerns grounded? Is this a potential issue?

@sun:

Yep, that looks related.

@comsicdreams:

The parent issue checks whether data is available and drop the schema only if it is not. Otherwise it skips any change that would imply a data migration.

I was just suggesting to keep a behavior at least somehow close the complete implementation, the current one should never end-up in a production environment. This is just an intermediate step to make patches easier to review.

Oh ok, Yea, if it's always going to be empty before a schema change, and entities that aren't empty won't get a schema change, then yea, it would make the process a lot more tidy if the new schema replaced the new one.

+   * Creates the schema for the given entity type definition.
+   *
+   * @param \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type
+   *   The entity type.
+   */
+  public function createEntitySchema(ContentEntityTypeInterface $entity_type);

Well, it does not really create the schema, does it? It creates the table based upon the generated schema. Not sure what would be a storage independent naming for that though. :(

Renames EntitySchemaHandlerInterface to ContentEntitySchemaHandlerInterface, since it's just for content entities.

Well, the interface does not seem to be tied to content entities at all and could easily work with all entities. Also non-content entities might have schema - config entities do have schema, it's just used differently.
So, maybe this could be just a storage-schema class receiving notifications so in theory it could be a place for handling config schema as well. I've no idea whether it makes sense to do so, probably not - but it seems wrong to limit the interface to content entities just because our config entity implementation works differently.

Not sure what would be a storage independent naming for that though. :(

installEntitySchema?

Well, the interface does not seem to be tied to content entities at all and could easily work with all entities. [...]

Yep, we had a similar discussion in the parent issue: I was afraid that generalizing this to all entity types would require too many changes, so I opted for sticking to content entities for now. Generalizing that can be done without API changes.

Both #14 and #15 work for me :)

yeah, the install/uninstall schema wording is better!

#14+#15 sound good to me as well.

@eff:

@yched was +1 on #15 too so if @berdir is ok, the most interested people should be all on the same page.

Reviewed the code. I think the new approach makes sense. I cannot vouch for whether this is good architecture, I don't have the necessary know-how but several key people before me agreed it is. As for the code itself, these new unqualified @todo comments are the only ones concerning:

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -958,6 +958,39 @@ public function getEntityTypeFromClass($class_name) {
+    // @todo Forward this to all interested handlers, not only storage.
...
+    // @todo Forward this to all interested handlers, not only storage.
...
+    // @todo Forward this to all interested handlers, not only storage.

@todo should have a URL for an issue or resolved in this issue.

Since the storage_schema is now a regular handler what about looping on all the defined handlers and checking whether they implement the listener interface? That way we could skip proxying the calls from the storage.

Thanks, reviewing is much better that way! Patch looks good in general - I've not look through all the unit test changes yet though. Why are those required? (Seems like they clean them up a bit though.)

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -104,7 +104,7 @@ class ContentEntityDatabaseStorage extends ContentEntityStorageBase implements S
   +   * @var \Drupal\Core\Entity\EntityTypeListenerInterface
       */
      protected $schemaHandler;
   

   That seems a bit weird. Is it a schema handler or a listener now?

   It's still a schema handler, just one that listens. Thus, I think we should keep EntitySchemaHandlerInterface - even if empty / extending only. A handler should have an interface imo, so you know what's required to implement. Plus, this is where you'd look for docs describing what that handler is good for.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -958,6 +958,42 @@ public function getEntityTypeFromClass($class_name) {
   +    // @todo Forward this to all interested handlers, not only storage, once
   +    //   iterating handlers is possible: https://www.drupal.org/node/2332857.
   

   Instead of further continuing with that custom-event spreading system, I think it would make sense to start converting to symfony events and allow handlers to consume those events. That should be fine without API change, if we keep BC-support for storage and have the EM dispatch the event (what makes sense).

3. +++ b/core/lib/Drupal/Core/Entity/EntityTypeListenerInterface.php
   @@ -0,0 +1,41 @@
   +interface EntityTypeListenerInterface {
   

   Thus - should we add an todo to convert this to events so that we save developers stumbling over this from experiencing the WTF "Why aren't they using events!"?

RTBC +1 here, obviously I cannot actually change the status myself :)

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -12,7 +12,7 @@
+use Drupal\Core\Entity\ContentEntityTypeInterface;

This was no longer needed.

This looks great to me as well. Just two minor things.

1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -845,19 +844,12 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   -        // Install any entity schemas belonging to the module.
   +        // Notify the entity manager that this module's entity types are new,
   +        // so that it can install its schemas or whatever else is needed.
   

   Might be me, but "whatever else is needed" sounds a bit ... unprofessional? ;)

   I'd write something like "So that it can prepare the storage, for example install the necessary schemas/tables".

   Same for delete.

2. +++ b/core/modules/simpletest/src/KernelTestBase.php
   @@ -388,24 +389,15 @@ protected function installSchema($module, $tables) {
   -
   +    if ($storage instanceof SqlEntityStorageInterface && $storage instanceof EntityTypeListenerInterface) {
   +      $table_mapping = $storage->getTableMapping();
          $this->pass(String::format('Installed entity type tables for the %entity_type entity type: %tables', array(
            '%entity_type' => $entity_type_id,
   -        '%tables' => '{' . implode('}, {', array_keys($schema)) . '}',
   -      )));
   -    }
   -    else {
   -      throw new \RuntimeException(String::format('Entity type %entity_type does not support automatic schema installation.', array(
   -        '%entity-type' => $entity_type_id,
   +        '%tables' => '{' . implode('}, {', $table_mapping->getTableNames()) . '}',
          )));
        }
   

   Looks like we remove the exception here in case the entity type does not support this, so we would fail silently. We still have an if(), so we could keep the exception? Not feeling strongly about it.

I may be wrong, but I think the exception is thrown in another patch :)

Updates look good to me, RTBC +1.

Improvements look good - I agree this is ready.

Committers: please see the commit message at the top of the issue summary, which adds attribution to fago, who helped with earlier iterations of this code in the parent issue.

Thanks for taking care!

Committed 5c3b66c and pushed to 8.0.x. Thanks!

1. +++ b/core/modules/simpletest/src/KernelTestBase.php
   @@ -11,10 +11,11 @@
   +use Drupal\Core\Entity\EntityTypeListenerInterface;
   

   Not used.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityDatabaseStorageTest.php
   @@ -1078,9 +1055,56 @@ public function testCreate() {
   +   * @return \Drupal\Core\Field\FieldDefinition[]|\PHPUnit_Framework_MockObject_MockObject[]
   +   *   An array of mock field definitions.
   ...
   +    $definition = $this->getMock('Drupal\Tests\Core\Field\TestBaseFieldDefinitionInterface');
   

   No class called \Drupal\Core\Field\FieldDefinition exists. I guess this should be \Drupal\Tests\Core\Field\TestBaseFieldDefinitionInterface[]|\PHPUnit_Framework_MockObject_MockObject[]. Plus it returns an array of mock base field definitions.

3. +++ b/core/lib/Drupal/Core/Entity/Schema/SqlContentEntityStorageSchema.php
   @@ -53,18 +62,52 @@ class SqlContentEntityStorageSchema implements EntitySchemaHandlerInterface {
   +    //   Meanwhile, treat a change from non-Sql storage to Sql storage as
   +    //   identical to creation with respect to Sql schema handling.
   

   Sql => SQL in comments I think.

Fixed on commit.