Entity ID can be a string

I can confirm that this fixes the issue "Invalid datetime format: 1292 Truncated incorrect DOUBLE value: 'xxx': DELETE FROM feeds_clean_list..."

I had to be able to import & update from a CSV into custom tokens ( https://www.drupal.org/project/token_custom → )
This module provides a content entity which has a machine name (string) as id.
Just importing didn't trigger the error but once you want to update an existing item it fails to import and gives that nasty error.

With the patch the module is able to perform the import & update.

This is tricky because an entity id should be an int but it might be a string. Adding one of the main core issues about this topic for reference.

I'm not sure that core issue is a good one for reference because it's talking about a case where the id should be an int for those entity types but isn't, e.g. the database column is an int but due to some circumstance a string is returned ("1" instead of 1).

However EntityInterface does not limit the id to an int, the typehint is string|int|null so the statement "an entity id should be an int" is not true for all Drupal Entity types.

This issue is trying to fix the case where the entity id is intentionally a string (and the database column backing it is a varchar). Think about a table of US states where the id is the two-letter acronym—we're not trying to coerce "1" to 1, but instead trying to store the value "NY" as an int which is impossible.

I don't think EntityInterface supporting this is widely known so it's been catching out some other module assumptions (like Dynamic Entity Reference → which has some weird release versions to fix the same issue).

Check out the EntityTestStringId content entity type in core which tests this, and the issue that created it: #2205215: {comment} and {comment_entity_statistics} only support integer entity ids → , or the issue also using that same type to test core's Entity Reference field type making the same assumption: #2107249: Don't assume that content entities have numeric IDs in EntityReferenceItem → .

It would be good to have an automated test that tests importing and logging entities with a non-numerical ID. In core, in the test module "entity_test", there is a content entity type called "entity_test_string_id" (\Drupal\entity_test\Entity\EntityTestStringId). That entity type would probably be suitable for such a test. Do you want to write tests for this issue, @KarlShea?

@MegaChriz I added a test but I'm not sure that it's in the right spot, or that I'm putting a test for feeds_log outside of the submodule. I can make any changes here you'd like, just let me know the direction you're thinking.

When I remove the schema change for feeds_import_log_entry, the assertion fails checking for the log entry count, and when I remove the schema change for feeds_clean_list the test fails with an "Incorrect integer value for column" error.

For next minor PHPunit is failing: https://git.drupalcode.org/issue/feeds-3414632/-/jobs/1639583#L460
We can log a separate issue to fix this and the changes looks fine hence marking this RTBC

@KarlShea
I think it would be good to have one test focussed on Feeds Log, in feeds/modules/log/tests/Kernel. I think the other one could be added to the existing \Drupal\Tests\feeds\Kernel\UpdateNonExistentTest test class.

I think I'll give this a go myself, so I can hopefully merge this for the upcoming release.

Changes makes sense, thanks!

Thanks for checking, @KarlShea! I merged the code.

I got one issue when updating a site:

Data truncated for column 'entity_id' at row 789

feeds_import_log_entry.entity_id is allowed to be NULL.

Fixed that on commit.

@KarlShea
The database update feeds_update_8007() caused an error on MySQL 8.0:

SQLSTATE[42000]: Syntax error or access violation: 1235 This version of MySQL doesn't yet support 'existing primary key drop without adding a new primary key. In @@sql_generate_invisible_primary_key=ON mode table should have a primary key. Please add a new primary key to be able to drop existing primary key.': ALTER TABLE "feeds_clean_list" DROP PRIMARY KEY; Array

It looks like to have something to do with the MySQL setting "sql_generate_invisible_primary_key". It is said that if that setting is turned off, the error should disappear.

Do you know if the database update could be rewritten in a way that it doesn't cause an error when the MySQL setting "sql_generate_invisible_primary_key" is turned on?

See 🐛 SQL error after upgrading to 8.x-3.0-beta5 Active

Hmm my understanding is the only way to change a primary key is to drop it first and then recreate it. I asked in #contribute on Slack, let's see if anything comes from that.

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