Issue #2330121 by plach, effulgentsia, fago: Replace ContentEntityDatabaseStorage::getSchema() with a new EntityTypeListenerInterface implemented by SqlContentEntityStorageSchema.

Problem/Motivation

Sub-issue of #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions. As part of the parent issue, we want to make the entity storage system fully responsible for managing its storage needs. Currently in HEAD, we have code in ModuleHandler::install() and other places that call $entity_storage->getSchema() to get a Schema API array, and then communicate with the database to create/drop those tables. That division of responsibility makes no sense. Why should ModuleHandler and all other callers bias SQL-based storage in such a way? Better for it to tell the storage handler that an entity type definition is being created, updated, or deleted, and let the storage handler decide how to implement that. This refactor becomes especially important for implementing the update operation (not part of the scope of this child issue), which is quite involved and needs to be encapsulated.

Proposed resolution

Replace the SQL-specific getSchema() method with the more generic onEntityType(Create|Update|Delete)() methods that can be implemented by any storage system. See "API changes" for details.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

  • Removes EntitySchemaProviderInterface. This just had the getSchema() method that we are removing.
  • Removes EntitySchemaHandlerInterface. Same as above.
  • Adds an EnityTypeListenerInterface for reacting to creation, deletion, and updates of entity types. Makes SqlContentEntityStorageSchema implement this interface. Getting the schema is no longer part of SqlContentEntityStorageSchema's interface, it is a protected implementation detail.
  • Also makes ContentEntityDatabaseStorage and EntityManager implement EnityTypeListenerInterface. Thereby, outside code invokes the methods on EntityManager, which forwards to the storage handler, and ContentEntityDatabaseStorage forwards to the schema handler.
  • Changes former callers of EntitySchemaProviderInterface::getSchema() (e.g., ModuleHandler, KernelTestBase, contact_storage_test.install, and tests) to call the new EnityTypeListenerInterface methods on EntityManager (outside of unit tests) or the handler being tested (within unit tests) instead.
Files: 
CommentFileSizeAuthor
#35 interdiff.txt3.73 KBeffulgentsia
#35 entity-storage_operations-2330121-35.patch41.87 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,835 pass(es).
[ View ]
#32 entity-storage_operations-2330121-32.patch40.61 KBplach
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,829 pass(es).
[ View ]
#32 entity-storage_operations-2330121-32.interdiff.txt520 bytesplach
#29 interdiff.txt2.33 KBeffulgentsia
#29 entity-storage_operations-2330121-29.patch40.66 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,832 pass(es).
[ View ]
#27 interdiff.txt1.76 KBeffulgentsia
#27 entity-storage_operations-2330121-27.patch41.09 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,830 pass(es).
[ View ]
#24 interdiff.txt1.02 KBeffulgentsia
#24 entity-storage_operations-2330121-24.patch40.85 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,691 pass(es).
[ View ]
#21 interdiff.txt15.87 KBeffulgentsia
#21 entity-storage_operations-2330121-21.patch40.87 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#20 entity-storage_operations-2330121-20.patch39.54 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,691 pass(es).
[ View ]
#3 interdiff.txt1.39 KBeffulgentsia
#3 entity-storage_operations-2330121-3.patch47.43 KBeffulgentsia
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,577 pass(es).
[ View ]
entity-storage_operations.patch47.4 KBeffulgentsia
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Comments

effulgentsia’s picture

Note that the patch includes the hunks from #2326949-34: Move entity-type-specific schema information from the storage class to a schema handler pending that patch landing.

Status:Needs review» Needs work

The last submitted patch, entity-storage_operations.patch, failed testing.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new47.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,577 pass(es).
[ View ]
new1.39 KB
plach’s picture

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?

plach’s picture

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

cosmicdreams’s picture

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

plach’s picture

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.

sun’s picture

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

cosmicdreams’s picture

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?

plach’s picture

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

cosmicdreams’s picture

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.

fago’s picture

+   * 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.

plach’s picture

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.

effulgentsia’s picture

Re #13: if I understand #12 correctly, then what I think fago is suggesting is something like a EntityTypeListenerInterface with methods like onEntityTypeCreate(), onEntityTypeUpdate(), and onEntityTypeDelete() (the word "Definition" is unnecessary). So both ContentEntityDatabaseStorage and ContentEntitySchemaHandler (or whatever we rename it to) can implement EntityTypeListenerInterface.

In another issue, we could do something similar for a FieldStorageDefinitionListenerInterface and then have CEDS and CESH implement that too.

In which case, we don't have a need yet for either a EntitySchemaHandlerInterface or ContentEntitySchemaHandlerInterface, though perhaps something else in #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions will create that need, and if so, we can introduce it then.

Thoughts?

effulgentsia’s picture

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

I agree with doing this, and am fine with us doing it here if others approve. But I also think that can be done post-beta, so wouldn't want it holding up this issue if there isn't fast consensus on it.

plach’s picture

Both #14 and #15 work for me :)

fago’s picture

yeah, the install/uninstall schema wording is better!

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

plach’s picture

@eff:

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

effulgentsia’s picture

Assigned:Unassigned» effulgentsia
Status:Needs review» Needs work

#2326949: Move entity-type-specific schema information from the storage class to a schema handler landed, so working on a reroll of this plus incorporating all of the feedback.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new39.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,691 pass(es).
[ View ]

Starting with just a reroll. CNR for bot only.

effulgentsia’s picture

Assigned:effulgentsia» Unassigned
StatusFileSize
new40.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new15.87 KB

This implements #14 and #15, and also addresses #6 with respect to having the interim onEntityTypeUpdate() implementation be clearer and safer. If green, it's ready for human review; I think all existing feedback has been addressed.

effulgentsia’s picture

Title:Replace ContentEntityDatabaseStorage::getSchema() with EntityStorageInterface::onEntityTypeDefinition(Create|Update|Delete)()» Replace ContentEntityDatabaseStorage::getSchema() with a new EntityTypeListenerInterface implemented by SqlContentEntityStorageSchema
Issue summary:View changes

Updated title and summary to reflect the latest patch.

Status:Needs review» Needs work

The last submitted patch, 21: entity-storage_operations-2330121-21.patch, failed testing.

effulgentsia’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new40.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,691 pass(es).
[ View ]
new1.02 KB
Gábor Hojtsy’s picture

Status:Needs review» Needs work

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.

plach’s picture

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.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new41.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,830 pass(es).
[ View ]
new1.76 KB
fago’s picture

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!"?

effulgentsia’s picture

Related issues:+#2332935: Allow code to respond to entity/field schema changes
StatusFileSize
new40.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,832 pass(es).
[ View ]
new2.33 KB

Addresses #28.

effulgentsia’s picture

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

The unit tests being changed are those that were calling getSchema(), which is being removed by the patch.

effulgentsia’s picture

#29 is green and hoping it gets RTBC'd and committed quickly :)

Meanwhile, the next step, built on top of this one, is #2333113: Add an EntityDefinitionUpdateManager so that entity handlers can respond (e.g., by updating db schema) to code updates in a controlled way (e.g., from update.php). Please start reviewing that. Thanks!

plach’s picture

StatusFileSize
new520 bytes
new40.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,829 pass(es).
[ View ]

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.

Berdir’s picture

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.

plach’s picture

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

effulgentsia’s picture

StatusFileSize
new41.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,835 pass(es).
[ View ]
new3.73 KB

This addresses #33 in a way that doesn't consider non-SQL-storage to be an error.

effulgentsia’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

The interdiff in #35 is minor; plach and I reviewed each other's work; and fago, Berdir, and Gábor all reviewed this pretty thoroughly (see each of their most recent comments) and found nothing else to complain about. So, in the interest of expediency to keep the last beta blocker moving along, I'm RTBC'ing this. Thanks, everyone, for the great reviews! Hope to see you all in the next step: #2333113: Add an EntityDefinitionUpdateManager so that entity handlers can respond (e.g., by updating db schema) to code updates in a controlled way (e.g., from update.php).

effulgentsia’s picture

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.

Berdir’s picture

Updates look good to me, RTBC +1.

fago’s picture

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!

alexpott’s picture

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.

  • alexpott committed 5c3b66c on 8.0.x
    Issue #2330121 by plach, effulgentsia, fago: Replace...
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Status:Fixed» Closed (fixed)

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