Cannot reference an entity with a big int (size setting of ID field not propagated to reference field)

Wow, this a pretty nice find.

Thinking about this some more I wondered whether we couldn't just use a copy of* the actual id field definition and override the label. But there is no method for doing that, and even if there were - despite sounding nice at first - it might actually be a can of worms, not sure. So scratch that.

Then I thought: at least we could take over all settings of the ID field, i.e. $target_id_definition->setSettings($id_definition->getSettings()). That would also catch the unsigned setting which we currently just assume exists for the id field. But again, I'm not really sure, what all else this would entail and if that isn't really a can of worms. And this particular piece of code is pretty awful anyway with the explicit data type checking, so I think there is actually value in being explicit.

...so I think this is really good to go. Could use another pair of eyes, though, so not setting RTBC.

* I'm intentionally not using the word "clone" here, because cloning in typed data land is a rabbit hole I'm neither intellectually capable nor emotionally stable enough to go down...

Looks great to me!

Seems worth adding test coverage for.

Since it looks like it's not possible to actually have a working reference field at the moment that suffers from this bug, I don't think we need an upgrade path - but would be good to double check this.

Would you be content with unit test coverage or do you think integration test coverage is necessary here?

Unit test coverage might be OK. However I'm rethinking not needing an upgrade path here due to this hunk:

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -127,6 +128,14 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
+
+      // Prevent Drupal from assuming that every field needs updating by
+      // excluding adding 'normal' to any existing normal sized fields.
+      if ($size = $properties->getSetting('size')) {
+        if ($size != 'normal') {
+          $columns['target_id']['size'] = $size;
+        }
+      }

Wouldn't the state entry be out of sync anyway in this case?

@catch, @alexpott, @xjm, @Berdir, @swentel, @plach, and I discussed this issue and agree with it being Major priority, per https://www.drupal.org/core/issue-priority#major-bug: "a PHP error triggered under rare circumstances or affecting only a small percentage of all users".

Wrote some tests for the bug, an upgrade path and a test for the upgrade path.

Note that the new patch adds the 'size' setting to the schema unconditionally, because I think it's easier to maintain the code like that :)

Fixed that.

This fixes the target_id problem when setting a entity id field to big int. However the entity_id fields in the tables created for fields do not follow the big int setting.

I don't know if my error is related with this case or I have to create another issue to explain it (let me know if you think that I have to create another issue).

The problem is related with a "big" entity id, like this:

    $fields['id'] = BaseFieldDefinition::create('integer')
      ->setLabel(t('ID'))
      ->setSettings([
        'size' => 'big',
      ])
      ->setDescription(t('The ID of the Order entity.'))
      ->setReadOnly(TRUE);

The problem ocours when I try to change the entity removing another field and perform an entity-update. To remove a field Drupal create an auxiliar table "field_deleted_data_" and This table is thrown me an error related with the id size.

This is the error:

 [error]  Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'entity_id' at row 1: INSERT INTO {field_deleted_data_06d1a86f33} (entity_id, revision_id, uid_value, bundle, deleted, langcode, delta) SELECT entity_table.id AS entity_id, entity_table.id AS revision_id, entity_table.uid AS uid_value, :bundle AS bundle, :deleted AS deleted, :langcode AS langcode, :delta AS delta
FROM 
{orders} entity_table
WHERE entity_table.uid IS NOT NULL FOR UPDATE; Array
(
    [:bundle] => order
    [:deleted] => 1
    [:langcode] => und
    [:delta] => 0
)

I think that the problem is strongly related with the main case because is an unhandled situation caused for a big integer id.

EDIT: searching again in the issue queue, I found this: https://www.drupal.org/project/drupal/issues/2889619 mybe related with the issue too.

@estoyausente, yes your problem is related to the current issue.

We need to add the int size also to the $id_schema in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getDedicatedTableSchema(), which is used for the entity_id column of dedicated field tables. To test this case we'll need to add a field with a dedicated table to the new entity type "entity_test_big_integer_id" - note that this should be a normal field, not an entity reference field as entries of normal fields with dedicated tables reference the entity they belong to through the "entity_id" column.

Ok and thanks @hchonov, I found a little solution not tested yet (only tested in my use case) adding something like this:

I found a problem related with the default integer size. By default integer doesn't have any size and if you set the $id_schema size as a 'normal' (the default value) all entities will be updated. I think that is better only set the 'size' property when the value is different than the default value as, for example in my case, big. (See patch attached)

Has my code sense? If the code is enough to resolve the problem (as it seems) I can try to create the test (although I never did a test like this before).

Thanks for helping.

I found a problem related with the default integer size. By default integer doesn't have any size and if you set the $id_schema size as a 'normal' (the default value) all entities will be updated. I think that is better only set the 'size' property when the value is different than the default value as, for example in my case, big. (See patch attached)

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1996,6 +1996,11 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
+      $size = $id_definition->getSetting('size');
+      if ($size != 'normal') {
+        $id_schema['size'] = $size;

You could simply exchange this line by

if ($size = $id_definition->getSetting('size')) {
  $id_schema['size'] = $size;
}

in which case we'll set the size only if it has been explicitly set on the ID field definition.

@estoyausente, would you please post the full patch so that we are sure that all the tests pass.

For the additional test you could add a base field to EntityTestBigIntegerId::baseFieldDefinitions() with cardinality bigger than 1, which is a multiple values field and as such needs a dedicated table:

$base_fields['dedicated_table_field']  = BaseFieldDefinition::create('string')
      ->setLabel('Dedicated table field')
      ->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED);

+++ b/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceItemTest.php
@@ -201,6 +203,27 @@ public function testContentEntityReferenceItemWithStringId() {
+    $entity_big_int_id = EntityTestBigIntegerId::create([
+      'id' => PHP_INT_MAX,
+    ]);

For the additional test to be complete we need to provide here a value for the field with a dedicated table:

$entity_big_int_id = EntityTestBigIntegerId::create([
  'id' => PHP_INT_MAX,
  'dedicated_table_field' => 'test',
]);

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1996,6 +1996,11 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
+ $size = $id_definition->getSetting('size');
+ if ($size != 'normal') {
+ $id_schema['size'] = $size;
You could simply exchange this line by

if ($size = $id_definition->getSetting('size')) {
$id_schema['size'] = $size;
}
in which case we'll set the size only if it has been explicitly set on the ID field definition.

It was exactly my first approach but when I performed the entity-update, with this code all entities were updated. `$id_definition->getSetting('size')` return 'normal' as a default value although the value doesn't be set explicitly. It's a little bit difficult to find the reason because for a reason that I don't know my breakpoints were ignored when I set it inside this function and I cannot debug properly.

Anyway, I will create the full patch and try to create the test.

For the additional test you could add a base field to EntityTestBigIntegerId::baseFieldDefinitions() with cardinality bigger than 1, which is a multiple values field and as such needs a dedicated table:

$base_fields['dedicated_table_field'] = BaseFieldDefinition::create('string')
->setLabel('Dedicated table field')
->setCardinality(FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED);

It's a great idea. I will try to do it.

Thanks!

`$id_definition->getSetting('size')` return 'normal' as a default value although the value doesn't be set explicitly. It's a little bit difficult to find the reason because for a reason that I don't know my breakpoints were ignored when I set it inside this function and I cannot debug properly.

I took a deeper look into that and the default value for the size setting comes from \Drupal\Core\Field\Plugin\Field\FieldType\IntegerItem::defaultStorageSettings().

This means that your change makes sense, otherwise we would need an update path and I think we actually could skip it by only setting sizes different than the "normal" one. Could you only please perform a strict string comparison?

- Rerolled the code of #21 for the las 8.5 Drupal version (I'm not sure which version have to use in this case, I follow the version noted in the issue, 8.5.x-dev)
- Added my code from #29
- Added a strict string comparision

- Still need tests, I will try to develope it tomorrow.

I'm not sure which version have to use in this case, I follow the version noted in the issue, 8.5.x-dev)

We work with the 8.6.x branch until the 8.6.0 release. If needed the patch might be backported and committed to the current release branch.

When you name your patch please append the comment number - [issue-number]-[comment-number].patch.

It is useful to provide an interdiff between the patch you're uploading and the one on top of which you've made modifications. The naming convention for this is - interdiff-[comment-number-first]-[comment-number-second].txt.

Hi again!

I just added the test to the case, now the bug seems resolved.

Edit: the patch applied correctly in 8.7.x too.

I tried to check why the patch fail the test, I think that my code fail in `Testing Drupal\Tests\Core\Entity\Sql\SqlContentEntityStorageSchemaTest`because I restested the code of #29 and it throw 1 fail but the other error are related with the entity reference changes but I don't know exactly why.

Any idea or help to find the correct path to resolve it?

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -1971,6 +1971,11 @@ protected function getDedicatedTableSchema(FieldStorageDefinitionInterface $stor
   +      if ($size !== 'normal') {
   

   Looking at the test fails we see that now a size of "null" is being added and it is because we don't check here if the size setting is present at all. The condition here has to be if ($size && ($size !== 'normal')).

   Thinking over the patch again I think that it will be better if we are consistent and always set the size if present. In this case we'll have to provide an update for that change.

2. +++ b/core/modules/field/field.install
   @@ -133,3 +135,70 @@ function field_update_8500() {
   +function field_update_8501() {
   

   This update has to be moved to the system module, as the field module isn't required, which means that in some special installations the update might not run.

I was thinking about this problem lately and @tstoeckler was right in #8, that's exactly what we should do. And it's not really a can of worms either :)

Here's a mostly re-written patch.

Seems patch works well for common cases, but why it fails so much automated test (24,828 pass, 53 fail)? Maybe those test must be updated too for support current changes?

Seems the main reason of test fails is missing upgrade scripts for SQL schemas of some already created entities, example:
The SQL storage cannot change the schema for an existing field (content_translation_uid in entity_test_update entity) with data.
So, maybe easier will be don't touch SQL schemas of already exists fields, but allow big int only for newly created entity reference fields? And provide manually upgrade functions for needed fields.

hear a reroll.

In patch at #50 there is:

function system_update_8701() {

this function is already exists in module, as result we got an error on update.php:

PHP Fatal error:  Cannot redeclare system_update_8701() (previously declared in core/modules/system/system.install:2593) in core/modules/system/system.install on line 2985

so we must bump it to something like system_update_8902()?

Interesting, that on Drupal 8.9.6 I successfully create a new entity reference field to entity, that have bigint id field (via bigint contrib module), and all works well without this (or any other related) patch! The issue is only that field_entityreferencefield_target_id have 'varchar(255)' format instead of 'int'.

So, maybe create a less massive patch (maybe as alternative to current), that will fix only this problem, and don't touch id's of all other entities via hook_update_N()?

Here is simplified patch, that change field schema only for fields, that have size, differ from default ("normal").

Sorry, in previous patch I forgot to cleanup debug code, here is cleaned patch.

Seems there are also problem with deleting entity_reference fields with bigint, because creating field_deleted_data_xxxxxx tables on field deletion action produce SQL errors. I will try to investigate this later, or maybe anyone else can do this?

I have added failing test with bigint id, and moved my implementation to branch https://git.drupalcode.org/issue/drupal-2680571/-/tree/2680571-bigint-re...

Yeah! It's works.

MR applies and works well on Drupal 9.3.x-dev branch, and tests are passed too. What other steps do we need to commit this feature?

I've rebased my issue fork from Drupal 9.2.x to Drupal 9.3.x branch, what other steps we need to continue fixing this issue?

Let's get green CI run, as it's a bug it needs test-only patch to proove that test fails without fix

I also wonder how it will affect 32-bit users (I know few still exists yet)

PS: patches are hidden as there's MR

I also wonder how it will affect 32-bit users (I know few still exists yet)

If 32-bit users are not using bigint base fields, those changes must not change anything for them, but if they use - they already have a problem even without this patch ;)

I think this is an important and too long overlooked issue 🤓🙈

1. Drupal core in #1823494: Field API assumes serial/integer entity IDs, but the entity system does not added the ability to reference entities with various kinds of IDs, not just (32-bit) integer IDs. It made it possible (for example) to link to entities that use UUIDs as entity IDs.
2. There's a sibling issue to this in the Dynamic Entity Reference module: #3007899: Support entities with big integer IDs.
3. The Entity Usage module already added support for big integer IDs too: #2989033: Support entities with big integer IDs (which ironically was already referencing this very issue! 😄).
4. There are many interesting use cases that would benefit from bigint support, such as Openstreetmap: https://wiki.openstreetmap.org/wiki/64-bit_Identifiers

I added a note in the Api changes section that the consensus for not doing an upgrade path was based on discussion in Drupal Slack

Here is patch file with only test the bigint reference field, that fails on original Drupal without fixes from this issue.

🥳 Thanks for the test-only patch, @Murz!

Moving back to needs review for the MR. (had to stop myself writing NR for the MR)

To reduce anxiety I also implemented migration from mismatched entity reference columns to make column size match target entity id field size, please review.
Also I'm not sure is there a right place to put this migration, or anybody can recommend some better place for it?

I've published my OSM Localities module, that requires BigInt entity id
support in reference fields, to show how this feature is used in contrib modules.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I added this to my test install that has commerce 2.38 enabled.
Then I got an error:
Could not load the "commerce_order_item" order item type. in Drupal\commerce_order\Entity\OrderItem::bundleFieldDefinitions() (line 509 of /var/www/drupal10.2/html/modules/contrib/commerce/modules/order/src/Entity/OrderItem.php)

And the drush updatedb could not be run because of the error.