SqlContentEntityStorage::onFieldStorageDefinition(Create|Update|Delete)() is broken for base fields

I think a separate issue would be better, given the patch size.

We have to remember to block entity schema changes before the last iteration as some storage classes do not support them yet, AAMOF they hard-code quite some assumptions about table layouts.

+1 on #5

Useless assertions. What should they be changed to?

Mmh, it seems I forgot to complete that refactoring: we just need to convert those assertions to expectations for the ::createTable() method of the db schema service (see ::testOnEntityTypeCreate()). I can fix those tomorrow, bed time now :)

Implements DefaultTableMapping::allowsSharedTableStorage() as returning true for single-valued base-fields only. Multivalued base fields get a dedicated table, which is a database schema change compared to HEAD, but as a result, now work. There are no core examples of a multi-valued base field (except ones with custom storage, which are by definition not managed by the storage schema handler), but there might be in contrib.

Core actually has at least two use cases for this, we just couldn't use it yet. That is a) user.roles and b) taxonomy parents (which are right now lazy-loaded and lazy-updated, but with all the other writes that are now going on and the entity cache, I think we can drop that stuff and save a lot of custom code.

I've started working on this a while ago in #2248977: Complete support for multi-value base fields in ContentEntitySchemaHandler and use it for the user.roles field, will pick it up when this is in.

Will try to review this today.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -237,18 +247,38 @@ protected function schemaHandler() {
      /**
   +   * Updates the wrapped entity type definition.
   +   *
   +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   +   *   The update entity type.
   +   *
   +   * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
   +   *   See https://www.drupal.org/node/2274017.
   +   */
   +  public function setEntityType(EntityTypeInterface $entity_type) {
   

   Adding deprecated functions to get the beta in kind of sucks.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -237,18 +247,38 @@ protected function schemaHandler() {
   +  public function getTableMapping(array $storage_definitions = NULL) {
   +    $table_mapping = $this->tableMapping;
   
   @@ -321,20 +351,43 @@ public function getTableMapping() {
   +      if (!$storage_definitions) {
   +        $this->tableMapping = $table_mapping;
   +      }
   ...
   -    return $this->tableMapping;
   +    return $table_mapping;
   

   The $this->tableMapping and $table_mapping logic is really difficult to follow. Basically it seems as though we statically cache when we are not using custom storage definitions. A comment would help somewhere. We have one usage of this functionality - in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::deleteSharedTableSchema() - i wonder if this would be simpler with an additional function for this and then refactor the common code into a protected function.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -260,30 +378,54 @@ protected function checkEntityType(EntityTypeInterface $entity_type) {
   +            throw new FieldException(String::format('Fieled storage definition for "@field_name" could not be found.', array('@field_name' => $field_name)));
   

   Fieled -> Field

Given 7, 12 is fine by me. I wrote most of the patch code so I am obviously generally +1 on it :)

Adding deprecated functions to get the beta in kind of sucks.

I agree that's not nice, but I think solving it would require to fix #2274017: Make SqlContentEntityStorage table mapping agnostic , which still needs some thoughts/exploration and that can be addressed mostly as internal refactoring, thus not requiring to be addressed before beta. Since that method is not part of any public API, although being public, I think the problems implied by going this way are more theoretical than practical. @eff suggested a similar approach to address #2326719-25: Move pseudo-private table mapping functions from ContentEntityDatabaseStorage to public API of DefaultTableMapping, that is making some methods public but not part of an interface, which works fine. Addressing this the same way makes sense IMHO.

Working on #21.2 and #21.3, while we wait for Alex's reply.

I reviewed the patch and I think it's ok to proceed as is. The most unfortunate part about it is defnitely the already mentioned setEntityType() part and table mapping instantiation, but cleaning this up is no simple task and would postpone this issue unnecessarily on a non-API breaking clean-up. I do think it's better done in a separate issue where we can focus on it anyway.

@plach: setEntityType() seems to be already a public non-interface method. We discussed moving more logic in the table mapping classes, do we have a separate issue for that or is that supposed to be part of #2274017: Make SqlContentEntityStorage table mapping agnostic ?

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1412,96 +1466,22 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
   +    $this->schemaHandler()->onFieldStorageDefinitionCreate($storage_definition);
   

   Deprecated naming, it's not called schema handler anymore. It's just internal though, so no show-stopper.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1671,23 +1658,49 @@ public function finalizePurge(FieldStorageDefinitionInterface $storage_definitio
      public function countFieldData($storage_definition, $as_bool = FALSE) {
   ...
   +    if ($table_mapping->requiresDedicatedTableStorage($storage_definition)) {
   +      $is_deleted = $this->storageDefinitionIsDeleted($storage_definition);
   

   I think it would make sense to add an method to the table mapping that can give you the right table for a given fields given some parameters (old-revisions vs new, default language or not).

3. +++ b/core/modules/system/src/Tests/Entity/FieldSqlStorageTest.php
   @@ -460,19 +459,9 @@ function testFieldSqlStorageForeignKeys() {
   -
   -    // Verify the SQL schema.
   -    $schemas = SqlContentEntityStorage::_fieldSqlSchema($field_storage);
   -    $schema = $schemas[$this->table_mapping->getDedicatedDataTableName($field_storage)];
   -    $this->assertEqual(count($schema['foreign keys']), 1, 'There is 1 foreign key in the schema');
   -    $foreign_key = reset($schema['foreign keys']);
   -    $foreign_key_column = $this->table_mapping->getFieldColumnName($field_storage, $foreign_key_name);
   -    $this->assertEqual($foreign_key['table'], $foreign_key_name, 'Foreign key table name preserved in the schema');
   -    $this->assertEqual($foreign_key['columns'][$foreign_key_column], 'id', 'Foreign key column name preserved in the schema');
   

   Looks like this got deleted without replacement?

   Update: Covered by testDedicatedTableSchema() of SqlContentEntityStorageSchemaTest now, so that's fine.

cross-post

This should fix #21, I'll answer to @fago's review in a separate post as soon as I get a chance. I just boarded a plane :)

[double post]

Deprecated naming, it's not called schema handler anymore. It's just internal though, so no show-stopper.

+1 on renaming that in a separate issue, it's a totally internal change.

I think it would make sense to add an method to the table mapping that can give you the right table for a given fields given some parameters (old-revisions vs new, default language or not).

@efff opened an issue for all of this, see https://www.drupal.org/comment/9095763#comment-9095763

$this->schemaHandler : is that such a big deal to rename here ? Seems a bit artificial to push this to a followup, and those kind of internal cleanups tend to get easily forgotten. But no showstopper, agreed.

We discussed moving more logic in the table mapping classes, do we have a separate issue for that or is that supposed to be part of #2274017: Make SqlContentEntityStorage table mapping agnostic ?

What we have for now is only that: I think it migth make sense to address moving instantiation logic in the table mapping and refactor the storage to use it together, as I imagine the required changes to be related. Otherwsie we need a separate issue and we need to update the patch @todo references.

ad #32: I'd still think that it should be getStorageSchema() and $storageSchema property as this matches the interface. I'd keep the storage prefix also as the dependencies of the storage are not all storage related.

Minor:

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -271,6 +271,13 @@ public function setEntityType(EntityTypeInterface $entity_type) {
+    // of field storage definitions is passed,for instance when comparing old

Missing space before "for".

I gave this some manual testing with the following results:
1. Changed a base field without a storage change - all fine, no updates detected.
2. Added a new base field to user entity, all created fine.
3. Tried deleting an existing base field. This generally seems to think it deals with dedicated fields and fails:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd8.comment__homepage' doesn't exist: UPDATE {comment__homepage} SET deleted=:db_update_placeholder_0; Array ( [:db_update_placeholder_0] => 1 ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->onFieldStorageDefinitionDelete() (line 1508 of /var/www/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

4. Tried updating field settings influencing storage, i.e. update comment id to be not unsigned. FieldStorageDefinitionUpdateForbiddenException has been correctly thrown.

5. Tried changing the schema provided by a field type. This seems to try to change all fields but fails to detect the data migration. I guess that is as the schema for the before and after comparison is calculated both with the updated way. However, that's something what is officially not supported in d7 either so I think it's fine to not have that case covered for now. That's something we should probably fix for release though -> I'd suggest creating a follow-up, critical issue for this. Thoughts?

Thus, setting needs work for 3.

+++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
@@ -113,4 +113,57 @@ public function testUpdateWithData() {
+    // and a custom code-defined bundle field. Services need to be reinstantiated

comment exceeds 80chars.

Thanks eff, I took a look at fixing the fail / issue. Problem is that currently the table mapping creation relies on the entity manager to determine whether something is a base field, but when the field has been deleted this information is not available any more as we do only keep field storage definitions in state.
However, we should be able to determine the storage by looking at the field storage definitions only - as that's their purpose. Thus, passing in base field definitions into the DefaultTableMapping is problematic in general. As we need to know whether a field is a base field in there I suggest adding FSDI::isBaseField(). Imo that makes sense to have as it's required to determine the storage.

Attached patch contains those test fixes and just adds in the existing but not previously unused logic to drop the columns of a deleted shared field. This seems to work fine - but I did not check whether influences other tests, so we'll see whether it is green.
However, it drops the columns only where there is no data, because else we need to do field purging in order to call delete() on each of the purged field items. That's not implemented yet for base fields and a bigger topic left for #2282119: Make the Entity Field API handle field purging. So in the case of having data we can throw an update-not-possible exception for now. However, I do think that's something we should fix for release, thus bump #2282119: Make the Entity Field API handle field purging to critical.

I've repeated my test for #36.3 and it works as it should now. However, imo it still needs some more test coverage to make sure this won't break people's data after beta:
- EntityDefinitionUpdateTest misses an testFieldCreateDeleteWithData() what makes sure we throw the respective exception when we try to delete a base field with data.

Given that improved coverage and green tests, I think this is ready to go!

So, I'm trying to test this using my file_entity use case that adds a bundle field to the file entity. And unfortunately, I'm still stuck on my 'initial' problem. My attempt in the previous issue with putting it in a setting of the field definition wasn't accepted, which is OK with me :)

However, I've now tried the only other thing I can think of (short of providing a custom field type plugin for this) and that is to override the storage_schema handler and add the initial key there. That doesn't seem to work. Apparently, onFieldStorageDefinitionCreate() doesn't go through getEntitySchema(), so it doesn't pick up whatever changes are made there... That seems problematic? It would mean that any addition there on base fields will be skipped when you call onFieldStorageDefinitionCreate()/Update(), like those not null calls or the uid type override in UserStorageSchema.

Is that a known issue? Should we somehow route the schema definition for base fields through getEntitySchema()? If you think that we can solve this as a follow-up without outside API changes then I'm OK with that :) Just wanted to make sure we're aware of this limitation...

If you want to try it out yourself, the code is in https://github.com/md-systems/file_entity/tree/schema_handling, run FileEntityFileTypeClassificationTest, which is testing exactly this scenario.

See https://github.com/md-systems/file_entity/compare/schema_handling for the relevant changes compared the current manual creation of that field.

Implements DefaultTableMapping::allowsSharedTableStorage() as returning true for single-valued base-fields only. Multivalued base fields get a dedicated table, which is a database schema change compared to HEAD, but as a result, now work. There are no core examples of a multi-valued base field (except ones with custom storage, which are by definition not managed by the storage schema handler), but there might be in contrib.

One more thing on this :)

I started re-rolling #2248977: Complete support for multi-value base fields in ContentEntitySchemaHandler and use it for the user.roles field on top of this, to test it.

This, as a generic statement, is not (yet) true. Yes, it has logic to build the schema for those tables, but it doesn't actually create the necessary tables yet when a module is installed.

I added the following to onEntityTypeCreate() to allow that in my issue (no patch posted yet, still working on it):

    // Explicitly create the tables for base fields with dedicated tables.
    $table_mapping = $this->storage->getTableMapping();
    foreach ($this->entityManager->getFieldStorageDefinitions($this->entityType->id()) as $field_storage_definition) {
      if ($field_storage_definition->isBaseField() && $table_mapping->requiresDedicatedTableStorage($field_storage_definition)) {
        $this->onFieldStorageDefinitionCreate($field_storage_definition);
      }
    }

Feedback is very welcome if there is a better way to do it :) Again, mostly wanted to make sure that the current state is clear and what works and what doesn't.

Created #2339855: Handle changes to the schema provided by a field type for #36.5

That seems problematic? It would mean that any addition there on base fields will be skipped when you call onFieldStorageDefinitionCreate()/Update(), like those not null calls or the uid type override in UserStorageSchema.

Agreed. That means we always have to go through entity schema to derive the schema of a certain field, i.e. we have to make sure we can invoke getEntitySchema() with old field storage definitions also. However, if we add the possiblity to add indexes in a storage acgnostic way I'm not sure the getEntitySchema() override would be still needed? So maybe that would be alternative, cleaner way to address the problem - but that's clearly follow-up.

Imo, we have to fix bailing out to getEntitySchema() for getting the schema of a single field storage definition, i.e. add a possibility to pass in the field storage definitions for which to generate the schema similar getTableMapping() on the storage. I think we need plach's input on this one though :/

But moreover, the changes of #53 make the system to fail to detect changes to this customizations, or to detect changes like #2339855: Handle changes to the schema provided by a field type, thus I'm not sure this is a good idea?

This should fix #44 by moving field schema alteration to ::getSharedTableFieldSchema() implementations. As discussed in Skype with @berdir, @eff anf @fago, this restores entity schema index/key difference tracking, which allows to detect changes also in field index/key definitions, as alterations are not prefixed with 'field_' and thus are stored together with entity index/keys.

I opted for allowing ::getSharedTableFieldSchema() to return a different definition depending on the shared table, since there are edge cases where the same field can have slightly different definitions depending on the table. For instance {node}.vid is nullable while {node_revision}.vid is not.

thanks d.o. for eating my comment :(

Changes look good in general, although I must say that the DX of getSharedTableFieldSchema() is worse than the single getEntitySchema() before. Having everything in a single array makes it nicer, but moreover we have to make clear when to use which method now. I do think this is solvable with documentation for now, plus we should be able to do away with the per-field optimizations rather easily by adding storage-acnostix indexes support.

+++ b/core/modules/user/src/UserStorageSchema.php
@@ -21,20 +22,6 @@ class UserStorageSchema extends SqlContentEntityStorageSchema {
-    // The "users" table does not use serial identifiers.
-    $schema['users']['fields']['uid']['type'] = 'int';

That valuable comment got lost.

It's great to have the improved test coverage now, as this covers now all important cases. I did test my manually tests once again and came to the same result: everything's good except the problematic case of adding a field having optimizations. In my test, it detected an entity type change + the field change, thus I'd assume the problem is that the entity type change already re-initialized the schema, so the field change afterwards fails.

ok, I fixed the tests, re-added above comment and added some docs on when to override which method. Given that, this now takes care of the remaining problems as well and should be ready to go imo.

This should fix installation.

RTBC +1 if this is green :)

omg. +1 if RTBC

Unable to find any nitpicks so RTBC as per #75.

1. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
   @@ -105,10 +105,11 @@
   +   * Tests creating, updating, and deleting a base field when there are no existing entities.
   

   nitpick - 80 chars, but shouldn't hold up the patch.

2. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
   @@ -135,18 +173,17 @@
   +   * Tests creating, updating, and deleting a bundle field when there are no existing entities.
   

   same here

Marking RTBC so core committers can have a first look up at it.

I gave the patch thorough review in #21 and the discussion since has addressed further issues with interdiffs all looking good. Let's do this :)

Committed f27fd1f and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
index 3755d0f..752a1f3 100644
--- a/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
+++ b/core/modules/system/src/Tests/Entity/EntityDefinitionUpdateTest.php
@@ -105,7 +105,7 @@ public function testEntityTypeUpdateWithData() {
   }

   /**
-   * Tests creating, updating, and deleting a base field when there are no existing entities.
+   * Tests creating, updating, and deleting a base field if no entities exist.
    */
   public function testBaseFieldCreateUpdateDeleteWithoutData() {
     // Add a base field, ensure the update manager reports it, and the update
@@ -179,7 +179,7 @@ public function testBaseFieldCreateUpdateDeleteWithoutData() {
   }

   /**
-   * Tests creating, updating, and deleting a bundle field when there are no existing entities.
+   * Tests creating, updating, and deleting a bundle field if no entities exist.
    */
   public function testBundleFieldCreateUpdateDeleteWithoutData() {
     // Add a bundle field, ensure the update manager reports it, and the update
@@ -224,7 +224,7 @@ public function testBundleFieldCreateUpdateDeleteWithoutData() {
   }

   /**
-   * Tests creating and deleting a base field when there are existing entities.
+   * Tests creating and deleting a base field if entities exist.
    *
    * This tests deletion when there are existing entities, but not existing data
    * for the field being deleted.
@@ -255,7 +255,7 @@ public function testBaseFieldCreateDeleteWithExistingEntities() {
   }

   /**
-   * Tests creating and deleting a bundle field when there are existing entities.
+   * Tests creating and deleting a bundle field if entities exist.
    *
    * This tests deletion when there are existing entities, but not existing data
    * for the field being deleted.

YEEEEAAAAAHHHHHH!!

Incredible!

Congratulations and many thanks to all core devs for working on this and other issues making Drupal 8 a step closer for all the rest of us!!!

WHO-HOO!

Yay ! Congrats, folks ! Sorry I couldn't help much :-/

Congrats.. There are 0 beta blockers now. Let's wait and see :-D

Hooray!

For everyone who noticed that this was the last known beta blocker and is wondering when the beta will come out, see: Today there are zero Drupal 8 beta blockers! Here's what's next.

It seems that in the course of this the code for supporting 'unique keys' got lost. It's still documented, there is code to remove them, but not create them even if they are in the schema array.

I've opened a new issue at #2742103: SqlContentEntityStorageSchema::getDedicatedTableSchema() ignores 'unique keys' from the schema definition