Support non-numeric entity ID's

I'm giving it a lot of thought I need to collect some more info about it and http://larowlan.github.io/ds.core-til/#/6/3 so please bear with me. Thanks.

The idea is to introduce the new DER field type for config entity see #2365331-16: Update DynamicEntityReferenceItem according to EntityReferenceItem for more details. When I converted target_type to integer i thought all the fieldable entities have integer ids. After some research I realized I was wrong.

After discussing with @larowlan we decided to do following.

1) Show all the entities config/content on field storage form. Just like ER field in core.
2) If all traget_types have integer id. We create a schema with integer column.
3) If any one of the traget_type has a string id. We create a schema with varchar column.

If the field has data then it is not a problem because we can't change target types and we don't have to change schema.
If the field has no data and the schema was int before and after re-saving the field it changed to varchar or vise versa then Drupal will update the table automatically as it is done for ER field.

Content entities can have string ID's.

That comment doesn't matter now we have new plan, turn target_id column to string by default and if all the target entity types id has integer schema then we'll change the column type of target_id form string to integer automatically. I hope this makes it clearer.

Edit: deleted.

I had a chat with @larowlan today and we both think that it'd be good to add a new field type for sting entity ID's so that we don't end up in the mixed situation. ER is doing the same thing as well.

Other then flag #2678756: Allow config entities to be flagged I don't see any use case of mixed entity IDs. flag can have two DER basefields one for int entity id and other is for sting entity id and flagging/unflagging action or preSave can populate the respective DER field base on flagged entity id.

Given that @berdir is already involved. I'll ping @yched and @fago about it as well.

Edit: deleted.

Edit: OK I will bite.

The solution is for Drupal to grow up and use databases for real and not like MySQL 4.1 or so.

On the application side, target_id should be string so that it can encompass everything.

The SQL database should maintain a cast-to-unsigned column and index it. MySQL 5.7 and Oracle, virtual column, PostgreSQL, SQLite and MySQL <5.7 triggers. The only purpose for this column is faster querying, it shouldn't be visible to (most of the app).

The app only needs to be aware of this column in the two abstraction APIs we put on top of SQL: Views and entity query. For Views, this is really simple, doesn't even need a core patch, just the views data needs to look at whether the referred entity type uses strings or ints as IDs, for entity query we would need to patch up the eldritch horror that is Tables::addField, it can't get much worse.

So

1. Add a backend overrideable service to DER which adds this trigger/virtual column. Core patch is not needed, just use db_query. In the MySQL version split on database server version. Copy the necessary two LoC from Drupal\Core\Database\Driver\mysql\Install\Tasks::checkEngineVersion to figure out the MySQL server version. Figuring out the column name is a bit of work but I do not think it's too hard. I believe it's event based and EntityTypeEventSubscriberTrait is where we need to listen.
2. dynamic_entity_reference_field_views_data should switch between these columns as necessary.
3. Finally, this is the only core patch necessary: field type plugins need a new method that Tables::addField can call to rewrite the database column as necessary.

Altogether, hopefully all this is not a lot of code. The user does not need to be aware of anything funny going on. What's not to love?

This is just the boilerplate. It doesn't do anything and indeed it'll break because it switches to _int in views unconditionally and it's not created yet. But it shows the way.

This apporoch is quiet interesting IMO. Let's see how this shape up. Nice start @chx. Here are couple of minor point.

1. +++ b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
   @@ -73,7 +73,7 @@ class DynamicEntityReferenceItem extends EntityReferenceItem {
   -    $properties['target_id'] = DataReferenceTargetDefinition::create('integer')
   +    $properties['target_id'] = DataReferenceTargetDefinition::create('string')
   

   We need an upgrade path for and upgrade path test for this.

2. +++ b/src/Storage/IntColumnHandler.php
   @@ -0,0 +1,23 @@
   +/**
   + * @file
   + * Contains \Drupal\dynamic_entity_reference\Storage\CreateColumn.
   + */
   

   Not needed.

With the attached patch, DynamicEntityReferenceItemTest runs...

You can copy EntityReferenceItemTest::testContentEntityReferenceItemWithStringId to DynamicEntityReferenceItemTest to check your changes as well.

Hrm, tests pass here :(

Here are some review points.

1. +++ b/dynamic_entity_reference.services.yml
   @@ -2,3 +2,19 @@ services:
   +  dynamic_entity_reference.entity_type_subscriber:
   ...
   +  dynamic_entity_reference.storage.create_column:
   ...
   +  mysql.dynamic_entity_reference.storage.create_column:
   ...
   +  sqlite.dynamic_entity_reference.storage.create_column:
   

   Some doc love here please.

2. +++ b/dynamic_entity_reference.views.inc
   @@ -50,7 +50,7 @@ function dynamic_entity_reference_field_views_data(FieldStorageConfigInterface $
   -        'relationship field' => $field_name . '_target_id',
   +        'relationship field' => $field_name . '_target_id_int',
   

   We'd need a follow up to update this for string target ids?

3. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,110 @@
   + * Hands off field storage events to the integer column handler.
   

   Maybe explain it a little.

4. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,110 @@
   +   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   ...
   +   * @var \Drupal\dynamic_entity_reference\Storage\IntColumnHandler
   ...
   +   * @var \Drupal\Core\Entity\EntityFieldManagerInterface
   

   Desc is missing.

5. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,110 @@
   +   * EntityType constructor.
   

   Nope.

6. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,110 @@
   +    if ($storage_definition->getType() == 'dynamic_entity_reference' && $storage instanceof SqlEntityStorageInterface) {
   

   I'm always confused, should this have to be instance or SESI or should we check the type of entity_id key?

7. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,110 @@
   +      $field_name = $storage_definition->getName();
   +      $storage_definitions = $this->entityFieldManager->getFieldStorageDefinitions($entity_type_id) + [$field_name => $storage_definition];
   +      $mapping = $storage->getTableMapping($storage_definitions);
   +      $table = $mapping->getFieldTableName($field_name);
   +      $columns = $this->getColumns($storage_definitions, $mapping, $table);
   +      $column = $columns[$field_name];
   

   Some more doc love please.

8. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,110 @@
   +   * @param \Drupal\Core\Field\FieldStorageDefinitionInterface[] $storage_definitions
   +   * @param \Drupal\Core\Entity\Sql\TableMappingInterface $mapping
   +   * @param string $table
   +   * @return array
   

   Incomplete docs. :)

9. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,110 @@
   +    $filter = function (FieldStorageDefinitionInterface $definition) use ($mapping, $table) {
   +      return $definition->getType() == 'dynamic_entity_reference' && $mapping->getFieldTableName($definition->getName()) == $table;
   +    };
   +    $getter = function (FieldStorageDefinitionInterface $definition) use ($mapping) {
   +      return $mapping->getFieldColumnName($definition, 'target_id');
   +    };
   +    return array_map($getter, array_filter($storage_definitions, $filter));
   

   This is some real coding :D. More docs please.

10. +++ b/src/Plugin/Field/FieldType/DynamicEntityReferenceItem.php
    @@ -73,7 +73,7 @@ class DynamicEntityReferenceItem extends EntityReferenceItem {
    -    $properties['target_id'] = DataReferenceTargetDefinition::create('integer')
    +    $properties['target_id'] = DataReferenceTargetDefinition::create('string')
    

    DERItem::schema also needs an update.

11. +++ b/src/Storage/IntColumnHandler.php
    @@ -0,0 +1,29 @@
    +   * @var \Drupal\Core\Database\Connection
    

    Desc missing.

12. +++ b/src/Storage/IntColumnHandler.php
    @@ -0,0 +1,29 @@
    +    ¶
    ...
    +    ¶
    

    Nope

13. +++ b/src/Storage/IntColumnHandlerMySQL.php
    @@ -0,0 +1,36 @@
    +  public function onCreate($table, array $columns, $column) {
    

    Doc block missing.

14. +++ b/src/Storage/IntColumnHandlerMySQL.php
    @@ -0,0 +1,36 @@
    +    $this->connection->query('LOCK TABLES {' . $table . '} WRITE');
    +    $schema = $this->connection->schema();
    +    $column_int = $column . '_int';
    

    Comments please.

15. +++ b/src/Storage/IntColumnHandlerMySQL.php
    @@ -0,0 +1,36 @@
    +        'default' => 0,
    

    Do we need this?

16. +++ b/src/Storage/IntColumnHandlerMySQL.php
    @@ -0,0 +1,36 @@
    +    $this->connection->query("DROP TRIGGER IF EXISTS {$prefixed_name}_update");
    +    $this->connection->query("DROP TRIGGER IF EXISTS {$prefixed_name}_insert");
    +    $update_trigger = "CREATE TRIGGER {$prefixed_name}_update BEFORE UPDATE ON $prefixed_name FOR EACH ROW SET";
    +    $insert_trigger = "CREATE TRIGGER {$prefixed_name}_insert BEFORE INSERT ON $prefixed_name FOR EACH ROW SET";
    +    $body = '';
    +    $delimiter = '';
    +    foreach ($columns as $current_column) {
    +      $body .= "$delimiter NEW.{$current_column}_int = CAST(NEW.$current_column AS UNSIGNED)";
    +      $delimiter = ',';
    +    }
    +
    +    $this->connection->query("$update_trigger$body");
    +    $this->connection->query("$insert_trigger$body");
    +    $this->connection->query('UNLOCK TABLES');
    

    This is gibberish to me so please add @see, @link, @code or docs to elaborate it. :)

> We'd need a follow up to update this for string target ids?

Aye but since config entities are not stored in "regular" tables the follow up is only for "freak" content entities that have string IDs. I believe the entity system allows for that but AFAIK it is not tested anywhere in core.

> I'm always confused, should this have to be instance or SESI or should we check the type of entity_id key?

The type entity_id doesn't matter. SESI holds the getTableMapping method (and nothing else, in fact) and that's what we need.

This patch has significantly rewritten the subscriber, the code is much simpler and allows reaction to entity type create which makes the code work with hook_entity_base_field_info. When I get around to write an update hook it will also be helpful.

There is no interdiff as the interdiff is larger than the patch... it's a big change with only the trigger code remaining untouched although it has gotten a bit of defensive programming and comments.

There's a test content entity in core that has string ids

o_O well then! Hello entity_test_string_id! Since DER is already broken if you point to that, can we postpone handling that case to a followup still or should we roll it into this one? My plans for this issue were adding a query into a test or two to check the int column contains the right value; this is tested implicitly with views tests but I want to add an explicit test and also pgsql and sqlite drivers. That'll be enough for one go, what do you think?

Here we go for SQLite. I factored out the common parts into IntColumnHandler. Yes, some doxygen is missing.

Thanks @chx for updating the issue summary.
Let me have a look at theses so you can work on rest of them.

1. Add a views field alias (if possible, I am not sure) to Views so target_id_int never shows up in the Views result rows. Probably won't allow reverting all tests since they directly test a mostly internal structure.
2. Still in Views, support string ID'd entity types. With the current patch Views only supports integer IDs because it unconditionally switches to the integer column.

can we postpone handling that case to a followup still or should we roll it into this one?

I'd like to fix it here but this patch is already pretty big so let's wait for now.

Nice work on SQLite. Moar docs please.

PostgreSQL. Yesssss.

Hrm, let's drop.

Do you think this will be enough for views fix?

PostgreSQL is _annoying_ . I had DynamicEntityReferenceRelationshipTest passing before ; but that didn't drop things ; then I needed to DROP things and that broke PostgreSQL because even a DROP statement is elaborate and annoying. Oh well. I have shortened the trigger name while at it.

The views fix imo is a followup.

Note that #2751363: Don't allow test entities to share base table fixes the fails as far as I can see. Not sure how to proceed.

Dump file.
Steps to reproduce.
Install testing profile.
Run drush scr test.php
Run php ./core/scripts/dump-database-d8-mysql.php --no-ansi > dump.php
Run gzip dump.php

The PostgreSQL tests are failing with

DETAIL:  Key (proname, proargtypes, pronamespace)=(rand, , 2200) already exists.: CREATE OR REPLACE FUNCTION "rand"() RETURNS float AS

This is a core bug, when you try to install two Drupals in PostgreSQL in parallel then CREATE OR REPLACE runs in parallel and results in this error. A race condition gets triggered here.

This will fail without #2751363: Don't allow test entities to share base table but here we are.

protected $installProfile = 'testing'; is necessary since the testing profile was used to dump.

Also, I coded around #275136: TinyMCE don't submit format, but only text. It's not pretty but it hopefully makes for a green test. Please do not add additional tests before mysql comes back green.

Oh damn I typoed the module name. Oh well :D It's #2751363: Don't allow test entities to share base table .

These fails will go away after merging 8.x-1.x into the patch.

I dunno about those fails, I am rebased on origin/8.x-1.x. Here's a patch that passed locally and tests referring to a config entity because we can do that. I have also uploaded the script I use to test this patch, for it to work correctly commit the patch to branch 2550227 under modules/dynamic_entity_reference .

+++ b/src/Tests/Update/derUpdateTest.php
@@ -0,0 +1,42 @@
+    $this->assertEqual([1, 1, 1, 1, 0], db_query('SELECT field_test_mul_target_id_int FROM {entity_test_mul__field_test_mul} ORDER BY entity_id, delta')->fetchCol());
+    $this->assertEqual([1, 1, 1, 1, 'test'], db_query('SELECT field_test_mul_target_id FROM {entity_test_mul__field_test_mul} ORDER BY entity_id, delta')->fetchCol());

Oh! my.

Let's see those fails.

+++ b/src/Storage/IntColumnHandler.php
@@ -0,0 +1,115 @@
+ */
+class IntColumnHandler implements IntColumnHandlerInterface {
...
+   */
+  protected function createBody($column_int, $column) {
+    throw new \LogicException('Not implemented');
+  }

What about making it abstract ?

In general it would be great to explain a bit of the reasoning behind the SQL trigger on the relevant classes, but of course, this is contrib, this could totally happen later.

Thanks @chx for making it green. Should we create a new 8.x-2.x branch for this patch? We can release an alpha tag for this. There is an upgrade path and test for this so I think it is doable. Meanwhile we can make 8.x-1.x stable and forward port the patches as well.

+1 to #64. I think we should improve the overall docs.

I think we can move these things to followup:

• Convert DER field storage setting form to reference config entities + string ids.
• Fix the views integration as described in #37 patch is in #46.
• We need a test for #22.
• We can also fix #2458353: DER fields should add selection handler config dependencies after this.

I'll work on the trivial feedback.

1. +++ b/dynamic_entity_reference.install
   @@ -0,0 +1,47 @@
   +  $kv = \Drupal::keyValue('entity.storage_schema.sql');
   

   Let's add a comment about it and let's change the name as of variable as well.

2. +++ b/dynamic_entity_reference.install
   @@ -0,0 +1,47 @@
   +    if ($storage instanceof SqlEntityStorageInterface) {
   

   Let's add a comment here as well.

3. +++ b/dynamic_entity_reference.install
   @@ -0,0 +1,47 @@
   +      foreach (array_intersect_key($storage_definitions, $map) as $storage_definition) {
   

   Let's add a comment here as well.

4. +++ b/dynamic_entity_reference.install
   @@ -0,0 +1,47 @@
   +        $spec = [
   

   Let's move this out of the loop.

5. +++ b/dynamic_entity_reference.install
   @@ -0,0 +1,47 @@
   +        $schema->changeField($table, $column, $column, $spec);
   ...
   +      $service->handleEntityType($entity_type_id);
   

   So we are converting the existing target_id column form int to string and service will add the trigger and target_id_int? Let;s add a comment about it.
   I think we can pass the field_name and field_storage_definition as well.

6. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,137 @@
   +  public static function getSubscribedEvents() {
   

   We need KTB for all these events.

7. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,137 @@
   +    $events[EntityTypeEvents::CREATE][] = ['onEntityType', 100];
   +    $events[EntityTypeEvents::CREATE][] = ['onEntityType', 100];
   

   Aren't these two the same lines?

8. +++ b/src/Storage/IntColumnHandler.php
   @@ -0,0 +1,115 @@
   +   * @TODO not sure whether we want to bother with deleting.
   

   Do we need tests to be sure about it?

9. +++ b/src/Tests/Update/derUpdateTest.php
   @@ -0,0 +1,43 @@
   +class derUpdateTest extends UpdatePathTestBase {
   

   DerUpdateTest

10. +++ b/src/Tests/Update/derUpdateTest.php
    @@ -0,0 +1,43 @@
    +  protected $installProfile = 'testing';
    

    {@inheritdoc} is missing.

11. +++ b/src/Tests/Update/derUpdateTest.php
    @@ -0,0 +1,43 @@
    +      __DIR__ . '/der_dump.php.gz',
    

    It should be good to document the original generating script here.

12. +++ b/src/Tests/Update/derUpdateTest.php
    @@ -0,0 +1,43 @@
    +    $this->assertEqual([1, 1, 1, 1], db_query('SELECT field_test_mul_target_id_int FROM {entity_test_mul__field_test_mul}')->fetchCol());
    

    Let's document this.

13. +++ b/src/Tests/Update/derUpdateTest.php
    @@ -0,0 +1,43 @@
    +    $etb = EntityTestBundle::create([
    +      'id' => 'test',
    +      'label' => 'Test label',
    +      'description' => 'My test description',
    +    ]);
    +    $etb->save();
    +    $entity = EntityTestMul::load(3);
    +    $entity->field_test_mul[] = $etb;
    +    $entity->save();
    +    $this->assertEqual([1, 1, 1, 1, 0], db_query('SELECT field_test_mul_target_id_int FROM {entity_test_mul__field_test_mul} ORDER BY entity_id, delta')->fetchCol());
    +    $this->assertEqual([1, 1, 1, 1, 'test'], db_query('SELECT field_test_mul_target_id FROM {entity_test_mul__field_test_mul} ORDER BY entity_id, delta')->fetchCol());
    

    Let's move this to follow up issue because we have to change the DER field storage form to allow config entities. Let's use entity_test_string_id instead.
    But I love this assert.

14. +++ b/tests/modules/dynamic_entity_reference_275136/dynamic_entity_reference_275136.module
    @@ -0,0 +1,15 @@
    +function dynamic_entity_reference_275136_entity_type_alter(array &$entity_types) {
    

    so cool

RE: Trivial feedback.

1. Fixed.
2. Fixed.
3. Fixed.
4. This is well placed let's keep it that way.
5. Moved inside the loop and added a comment.
6. Added two passing tests for this. yay!!
7. Converted the other line to UPDATE.
8. Let's create an issue about it and discuss some more.
9. Fixed.
10. Fixed.
11. Added a comment.
12. Added docs and some more asserts.
13. Used entity_test_string_id and it is causing some fails. PFA my updated script with field settings. Patch is still green on old db dump.

In this patch.

• New DB dump using updated script.
• Updated column lenght on @dawehner's feedback.
• Fixed PHPCS errors.
• Added more asserts to update test.
• Added a test for referencing entities with string IDs.
• Added a test for referencing entities with string and int IDs.
• Attached interdiff only includes the code changes without DB dump.
• I ran into this error.
  

  SQLSTATE[42000]: Syntax error or access violation: 1059 Identifier name 'simpletest381403entity_test__entity_reference_string_id_der_update' is too long: DROP TRIGGER IF EXISTS simpletest381403entity_test__entity_reference_string_id_der_update; Array
  (
  )
  
  core/lib/Drupal/Core/Database/Connection.php:671
  core/lib/Drupal/Core/Database/Connection.php:635
  core/lib/Drupal/Core/Database/Driver/mysql/Connection.php:81
  modules/dynamic_entity_reference/src/Storage/IntColumnHandler.php:63
  modules/dynamic_entity_reference/src/EventSubscriber/FieldStorageSubscriber.php:128
  modules/dynamic_entity_reference/src/EventSubscriber/FieldStorageSubscriber.php:85
  core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php:111
  core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php:77
  core/lib/Drupal/Core/Entity/EntityManager.php:411
  core/modules/field/src/Entity/FieldStorageConfig.php:332
  core/modules/field/src/Entity/FieldStorageConfig.php:286
  core/lib/Drupal/Core/Entity/EntityStorageBase.php:434
  core/lib/Drupal/Core/Entity/EntityStorageBase.php:389
  core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:259
  core/lib/Drupal/Core/Entity/Entity.php:361
  core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:637
  modules/dynamic_entity_reference/tests/src/Kernel/DynamicEntityReferenceFieldTest.php:249

Complete interdiff can be found at https://github.com/jibran/dynamic_entity_reference/commit/9ac74e6087dbe2...

Thanks for the amazing amount of quick work! I have re-dumped and rerolled. Note that dumping this without https://www.drupal.org/files/issues/2555027.sh__0.txt is not feasible: as far as I can see the previous patch didn't include the fixup module. I reuploaded the script because now it points to test.php__17.txt instead of 15. There's no interdiff as there's no change (yet). The identifier issue is worrysome but easy to fix with a little hashing. Will do it next but first I want to see what the bot says.

Easy as pie. Instead of simpletest381403entity_test__entity_reference_string_id_der_update we have simpletest425511entity_test__entity_reference_string_id_208e2b58. It won't always be this nice but close. It uses the first 56 characters of the prefixed table and so in the non-test, most common case where the user has no prefix the hash won't be necessary: generateFieldTableName will produce a max 48 character name and _der_update only adds 11 which is below the max 64. It's only with simpletest (or other prefix) the hashing becomes necessary.

Did the abstract change dawenher asked for as well since I had the class open.

Note for reviewers: the patch is much smaller than 160kb, most of it is just the dump. Also, the only real change is

-    $properties['target_id'] = DataReferenceTargetDefinition::create('integer')
+    $properties['target_id'] = DataReferenceTargetDefinition::create('string')

everything else is just working around the strictly typed nature of SQL...

Thanks @chx for fixing my useless reroll. :D Great to see patch is green again. I think this is really ready. We can create a 8.x-2.x and continue work in follow ups.
As for the basefields upgrade path is concerned I think patch is already too big. Let's move basedfields upgrade path tests to follow up. Leaving this RTBC for two three days so that people can review this.
Meanwhile I'll provide the basefields script so that we can create another dump for testing.

As I said before this is ready. These are just doc improvements/questions and can be moved to follow up.

1. +++ b/dynamic_entity_reference.install
   @@ -0,0 +1,56 @@
   +function dynamic_entity_reference_update_8001() {
   

   Would you like to add more technical description for this update? You told me to add references to SqlContentEntityStorageSchema::updateDedicatedTableSchema(), FieldStorageDefinitionUpdateForbiddenException and SqlContentEntityStorageSchema::saveFieldSchemaData()

2. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,137 @@
   +class FieldStorageSubscriber implements EventSubscriberInterface {
   

   I think some description for this service and how der uses the whole trigger idea would be good. What do you think?

3. +++ b/src/EventSubscriber/FieldStorageSubscriber.php
   @@ -0,0 +1,137 @@
   +    $events[FieldStorageDefinitionEvents::CREATE][] = ['onFieldStorage', 100];
   +    $events[EntityTypeEvents::CREATE][] = ['onEntityType', 100];
   +    $events[EntityTypeEvents::UPDATE][] = ['onEntityType', 100];
   

   You explained the reason to me in IRC about these events i.e. why we can't use FieldStorageDefinitionEvents::UPDATE and why we need EntityTypeEvents. Can we state that reason here?

4. +++ b/src/Storage/IntColumnHandler.php
   @@ -0,0 +1,116 @@
   +abstract class IntColumnHandler implements IntColumnHandlerInterface {
   

   Let's add the trigger related docs here. I can understand the concept by looking at this patch but I don't now the complete idea. What triggers are how they work and etc.

5. +++ b/src/Storage/IntColumnHandler.php
   @@ -0,0 +1,116 @@
   +        if (strlen($trigger) > 64) {
   

   Is there a constant for this we can use? I tried to find out this limit but I had no idea where to look.

6. +++ b/src/Storage/IntColumnHandler.php
   @@ -0,0 +1,116 @@
   +   * @TODO not sure whether we want to bother with deleting.
   

   Let's create an issue for this and explore further.

7. +++ b/src/Storage/IntColumnHandler.php
   @@ -0,0 +1,116 @@
   +  public function delete($table, $column) {
   

   This will be an abstract function too when we'll implement it?

RE: #73 this will go into new branch so you don't have to upgrade immediately but feedback is welcome. :-)
@chx: Here is the interdiff for multiple basefields. For me this test is passing but you can add more specific asserts.

Note: the fixup module could be nicer as EntityType has get/set methods. I can't even be bothered to reroll to simplify a very temporary module.

+++ b/src/Storage/IntColumnHandler.php
@@ -0,0 +1,116 @@
+  public function delete($table, $column) {

Is this intentionally blank?

Got to say I was cautious about this patch, but now having read it, I think it is architected really nicely.

Great job folks.

Got to say I was cautious about this patch, but now having read it, I think it is architected really nicely.

Same here but now I'm really excited about it.

Regarding delete, there's a TODO and I am well aware this might or even likely to break if you drop a DER field from a shared field table. That's extremely rare and I am confident to leave it to a followup.

Since this is supposed to be a new branch could we go ahead with the commit so I can file followups?

I have already created 8.x-2.x branch. I just added you as a branch maintainer. Given that you have come up with the solution and build this from ground up it seems fair to us, Lee and me, that you should maintain this branch.

Thank you for your work and WELCOME TO THE JUNGLE!

But... but... I don't want to commit without your most valued reviews and other help. What about I still post patches, you review and I commit?

I will commit this one soon. Thanks for the confidence.

But... but... I don't want to commit without your most valued reviews and other help. What about I still post patches, you review and I commit?

Who said you are getting away from my annoying reviews? :D

Well, we always followed core practices in this module and we intend to do the same for 8.x-2.x as well.

What sarcasm? I really do value jibran's review and help. This issue has been one of the most delightful experiences I had in Drupal land in years. Speedy and collaborative, as it should be.

Committed!

What sarcasm?

Yup, there is no sarcasm here only gratitude. :)

I really do value jibran's review and help.

You are a legend of Drupal world and it's an honour to work with you.

This issue has been one of the most delightful experiences I had in Drupal land in years.

I'm so happy that I'm able to participate in this.

I have created a new dev release so that we can create 8.x-2.x backlog. I'm going to create some follow ups.

I have created 6 child issues

1. #2766175: Fix the Views integration for entities with string ids.
2. #2766181: Remove dynamic_entity_reference_275136
3. #2766187: Allow config entity references
4. #2766189: Fix IntColumnHandler::delete
5. #2766193: Add tests for multiple base fields
6. #2766195: Improve documentation of non-numeric entity ID's support

Write a core patch so that the field type can rewrite the field column as necessary when entity query runs.

Would you like to file a core issue for this? I think we should create a DER issue as well to track the progress we'll have to create an issue any way to make the code changes. Just a side note this going to be an api addition so I think we should speed up this process if we want it to be included in 8.2.x releases. First beta is due on 3rd August.