SqlContentEntityStorage tries to update identity/serial values by default

I think this belongs to core:

Is it correct to allow UPDATE statements that change (even if new value is same as previous) PRIMARY KEY values?
Does it make sense?

I guess the database driver could be modified to remove primary key fields from updates, but does not look like a good solution to me.

Actually it is not primary keys that are not updatable in SQL Server, but IDENTITY fields. IDENTITY columns are created for SERIAL fields in Drupal.

IDENTITY column values are immutable.

Moving to the proper queue.

I've boosted this to Major. Without this issue solved we will never see D8 running on SQL Server.

I know it's just 10,000 sites (0.1%), but the issue itself makes sense.

Looks good and fixes original issue.

I'm not sure if we can write a useful test for this but we can try.

The change is wrong however, there is a method that should be used for this in 8.x: isNew()

Tested, it's working OK now, D8 starts to be usable on SQL Server with this!

Fixed the ->isNew() issue.

1) Improved original implementation code readability.

2) With the change, revision management was all broken. The disturbing thing about that is that Drupal Tests were not able to detect a completely broken entity revision management sql storage.

3) Fixed revision management in new patch.

Remaining tasks: investigate why tests were not deteting the issue, and propose tests if needed.

what a mess...

Let's see how this new one behaves with tests.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -927,7 +927,7 @@ protected function doSave($id, EntityInterface $entity) {
   -          ->condition($this->idKey, $record->{$this->idKey})
   +          ->condition($this->idKey, $entity->{$this->idKey}->value)
   

   If you switch to $entity, then use the id() method instead of accessing the field directly.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1086,13 +1085,23 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
   +        $serial_and_void = empty($value) && $serial;
   +        // Do not set serial values if this is a new entity
   +        // serials are inmutable and autoincremental.
   +        // @see https://www.drupal.org/node/2342699
   +        $serial_and_not_new = (!$entity->isNew()) && $serial;
   +        // Prevent setting serial field values.
   +        if ($serial_and_void || $serial_and_not_new) {
   +          continue;
            }
   

   This seems a lot more complicated than it has to be? both checks include serial, so something like this would IMHO be much more readable:

   // Skip serial fields for existing entities or if the value is empty.
   if ($serial && (!$entity->isNew() || empty($value)) {
     continue;
   }
   

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1171,13 +1180,13 @@ protected function saveRevision(EntityInterface $entity) {
   +      if (!isset($entity->{$this->revisionKey}->value)) {
   ...
   +          ->fields(array($this->revisionKey => $entity->{$this->revisionKey}->value))
   
   @@ -1185,14 +1194,11 @@ protected function saveRevision(EntityInterface $entity) {
   +        ->condition($this->revisionKey, $entity->{$this->revisionKey}->value)
   ...
   +    return $entity->{$this->revisionKey}->value;
   

   Same here, use getRevisionId(). Note that this won't work with isset(), so you will need to do a !== NULL or so instead.

@bedir

1. Done, makes sense.

2. I also don't like it like that, but I wanted to keep as much comments as possible and looked into Drupal coding standards where they recommend doing that rather than breaking the condition into several lines to keep the comments. Anyways, I am trying something new here in the last patch.

3. Done, makes sense.

My local tests are broken, so I will need to mess up a little with the .org tests.

BTW: Is this supposed to be the way to check if a column is serial? isColumnSerial is just checking if this is ID or Revision Id, but what about other serial fields. Shouldn't this check depend on the schema type?

protected function isColumnSerial($table_name, $schema_name) {
    $result = FALSE;

    switch ($table_name) {
      case $this->baseTable:
        $result = $schema_name == $this->idKey;
        break;

      case $this->revisionTable:
        $result = $schema_name == $this->revisionKey;
        break;
    }

    return $result;
  }

So, last patch has many failures, but looking into it that is (at least some of the failurse) because isColumnSerial is failing to properly tell if a column is serial or not. Making the asumption that if a field is the ID or the REVISION ID of the entity it must be serial is not reliable.

I need some help here, I'm totally lost with the new way of defining database Schema's in D8.

I am trying to retrieve storage schema specifications by means of:

$this->getStorageSchema();

in the scope of a SqlContentEntity class. But there is no hint of "serial" field information inside the provided schema.

NID field appears be "int" everywhere but according to Schema API it is "serial". But the field in database is created as "serial" (int with autoincremental) so I guess at some point Drupal is able to tell that the NID field from the NODE entity is serial, I just don't seem to be able to retrieve that information.

This 5 minute patch is transforming into a nightmare, looks like a chain of some broken stuff that was holding together thanks to the loose behaviour of mySQL.

I deeply inspected getFieldStorageDefinitions(), getTableMapping() and getStorageSchema() none of them is able to tell the NID field of the NODE table is "serial", it is shown everywhere to be "integer".

I am even unable to tell what drupal data type is the NID column in NODE by using drupal_get_complete_schema(), this is just returning a handfull of tables, but not the NODE table schema.

What is the way in D8 to tell the storage data type of an entity's property?

I am starting to think that whoever wrote the isColumnSerial the first time was unable to acquire that information so decided to implement as is now.

Bad patch, but let's see how it does against tests.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -1113,20 +1116,9 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
+  protected function isColumnSerial($table_name, $column) {
+    $storageschema = $this->getStorageSchema()->EntitySchema($this->entityType);
+    return $storageschema[$table_name]['fields'][$column]['type']=== 'serial';

This is not an option, sorry. We tried hard to remove the need to use schema at runtime, we should not add it back here. The basic idea behind the entity storage is that we know how the schema is shaped because it's derived from entity and field definitions. We don't need to go asking the storage schema handler whether a field is serial, we know which ones are. What's wrong with the current code?

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1085,14 +1083,19 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
   +        // Serial fields should only be set on entity creation,
   +        // even if a value is provided. Serials are autoincremental ¶
   +        // and inmutable
   +        // so database takes care of everything.
   

   This comment is not wrapping at column 80. Also should be "immutable" :)

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1085,14 +1083,19 @@ protected function mapToStorageRecord(ContentEntityInterface $entity, $table_nam
   +        // @see https://www.drupal.org/node/2342699
   

   We usually link only follow-ups in code, not sure why we have another reference a few lines above but no need to add one. Git blame can be used to find the issue if needed.

3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
   @@ -428,6 +428,10 @@ protected function checkEntityType(EntityTypeInterface $entity_type) {
   +  public function EntitySchema(ContentEntityTypeInterface $entity_type, $reset = FALSE) {
   +    return $this->getEntitySchema( $entity_type, $reset);
   +  }
   +  ¶
      /**
   

   The ::getEntitySchema() method is protected for a good reason, which is we don't need to expose the schema array. This should be removed altogether.

@plach: Glad to see someone with knowledge of how EntityStorage works jumping in, I am very lost here. Sorry for the coding standards, I was just trying this to overally pass tests as a starting point.

What concerns me now most about this issue is how SqlContentEntityStorage::isColumnSerial works.

We don't need to go asking the storage schema handler whether a field is serial, we know which one are.

Well, this is the ContentEntityStorage (in charge of dumping the entity to database) trying to figure out what the type of a field is in the database.

That means that, as per isColumnSerial current implementation, any Entity's PrimaryKey or EntityRevision Revision Key is assumed to be SERIAL (and cannot be differently) and all other columns ARE NOT SERIAL (and this also cannot be differently).

And if your entity does not work like this, then the ContentEntityStorage that is in charge of dumping the Entity into the database has now way of telling what database type a column is. So basically, all Entities in Drupal should work like that.

Investigating this issue I saw that in D8 schema is inferred from Entity's field definition, in Entity/Node.php:

$fields['nid'] = BaseFieldDefinition::create('integer')
      ->setLabel(t('Node ID'))
      ->setDescription(t('The node ID.'))
      ->setReadOnly(TRUE)
      ->setSetting('unsigned', TRUE);

Then StorageSchemaHandler is forcing to be serial any key that is of type integer when constructing the Schema:

protected function processIdentifierSchema(&$schema, $key) {
    if ($schema['fields'][$key]['type'] == 'int') {
      $schema['fields'][$key]['type'] = 'serial';
    }
    unset($schema['fields'][$key]['default']);
  }

Then why not use this insted (can't even wonder if it works):

   $fields['nid'] = BaseFieldDefinition::create('serial')
      ->setLabel(t('Node ID'))
      ->setDescription(t('The node ID.'))
      ->setReadOnly(TRUE)
      ->setSetting('unsigned', TRUE);

I can workaround this issue in the SQL Server database driver (and I guess it will need to be like that because looks like D6 migration is asuming you can UPDATE and/or INSERT values into a serial column, wich you can actualy do in SQL Server but with special techniques), but I don't think it is formally correct to have this sort of statement being issued against the database:

UPDATE xxx SET nid = 1, property0 = :value0 WHERE nid = 1

Futhermore, why does SqlContentEntityStorage have a getStorageSchema() and getFieldStorageDefinitions() method that is saying that storage type for the NID is 'int' instead of 'serial'?

Makes sense to have a consistent entity design all accross drupal regarding keys and serial columns, documentation should be extremely clear about this:

- Serial fields do not exist, you can only have a serial column per entity and that is it's key and it must always be set to 'integer'.
- If a key in an entity is an 'integer', then it is automatically assumed to be serial. Other types are allowed though... That means that you cannot have an entity key to be integer and not auto incremental, but you can have an entity key being a string?

I am still not fond of isColumnSerial implementation, if these asumptions are all made inside StorageSchemaHandler then why not ask that class what has it decided to do with keys and serials. Complete agree with this:

@mradcliffe wrote:

The implementation of mapToStorageRecord is very naive and makes assumptions that cannot be made for writing to a SQL database. It is absolutely necessary that code that sets up writes to a database knows how to write to a database i.e. knows what it should or should not do. The class name is SqlContentEntityStorage and thus it should behave how SQL expects it to.

Found the annoying bug in the patch, let's retest.

Failing tests are amazingly passing in my local environment with latest patch (SQL Server though...), try to retest.

@david_garcia:

[...] if your entity does not work like this, then the ContentEntityStorage that is in charge of dumping the Entity into the database has now way of telling what database type a column is. So basically, all Entities in Drupal should work like that.

Yep, this is how the default SQL entity storage is supposed to work. If you have an entity type that needs to behave differently you just need to provide an alternative storage class.

Then why not use this insted (can't even wonder if it works):

Because 'serial' is a Schema API type, while field definitions are using Type Data API types, for which serial is meaningless: it just cares about the native type.

I don't think it is formally correct to have this sort of statement being issued against the database.

I'm not saying it's correct, I totally agree with the goal of avoiding this kind of update statements. I'm just saying we can rely on a known schema to deal with them.

why does SqlContentEntityStorage have a getStorageSchema() and getFieldStorageDefinitions() method that is saying that storage type for the NID is 'int' instead of 'serial'?

The storage schema needs to be instantiated only in rare conditions, that is when dealing with schema updates, that are forwarded from the storage class to the storage schema class because the latter is not part of the "public API", in fact not every entity storage would require schema handling (the Mongo Storage will not, probably).

@mradcliffe:

The implementation of mapToStorageRecord is very naive and makes assumptions that cannot be made for writing to a SQL database.

I can only repeat that we are making no assumptions, we know how the schema is shaped. If the current implementation of mapToStorageRecord is incorrect or does not take into account all possible SQL flavors I'm all for fixing it, I'm just saying we don't need to retrieve the schema, because it's generated, hence it's well known.

I agree with @david_garcia that it should be possible to add other sequence fields, either base field definitions or complex field definitions, for an arbitrary content entity.

AFAIK only one serial field is allowed for each table on most RDBMS, anyway, as I said above, if you need a different behavior you can just code your own classes. The current approach has worked well for all core entity types and lots of contrib ones so far.

@david_garcia:

That means that you cannot have an entity key to be integer and not auto incremental, but you can have an entity key being a string?

Nope :) That just means that most entity types behave that way. For instance, the User storage class overrides isColumnSerial() to use a non-serial integer id column.

I am still not fond of isColumnSerial implementation, if these asumptions are all made inside StorageSchemaHandler then why not ask that class what has it decided to do with keys and serials.

From an architectural POV you are totally right: it would be better if this kind of logic were encapsulated in the storage schema class. Here we decided to go this way to avoid the need of instantiating the storage schema class on every request. This is a bit like cheating but given how much coupled the storage and storage schema classes are, it should not be a problem in practice. This is not set in stone however, we are still evaluating these implementation details: a possible alternative could be moving this knowledge into the table mapping class and make the storage and storage schema rely on it. However this is a different issue :)

Ok... this issue was great learning on to how EntityStorage works in D8 :)

A briefing of things to do:

- First we need to fix issuing update statements that have identity updates, while preserving al other behaviour. That's what my last patch tries to achieve, but fails some tests. I cannot replicate locally, they are all passing. I even moved from SQL Server to MySQL to run the tests, and still passing locally.

- Figure out why although patch in #6 passed tests, entity updates were left completely broken. Will probably require some new tests to be writen.

- Open a new Issue to move forward on this discussion:

From an architectural POV you are totally right: it would be better if this kind of logic were encapsulated in the storage schema class. Here we decided to go this way to avoid the need of instantiating the storage schema class on every request. This is a bit like cheating but given how much coupled the storage and storage schema classes are, it should not be a problem in practice. This is not set in stone however, we are still evaluating these implementation details: a possible alternative could be moving this knowledge into the table mapping class and make the storage and storage schema rely on it. However this is a different issue :)

Patch is no longer applying. This is tagged as needing tests, but it seems like a difficult thing to test for.

Re-rolled!

Rerolled the patch.

Changing the status to In review.

It would probably be a good idea to find out where are the 6 test failures comming from in #37 before introducing the extra code in #50.

Patch re-rolled for #39

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -783,7 +783,60 @@ protected function doSaveFieldItems(ContentEntityInterface $entity, array $names
+              ->fields((array) $record)

What this $record is? It is giving syntax error.

Removed $record variable as suggested. It was not used in query.

it is still a problem in v8.1.2

Fix a few brackets in patch #53 (which is based on #37). Added also feedback in #57.

This currently works for Drupal 8.2.3.

I haven't addressed the failing tests.

Let's test it, my local MSSQL is quiet slow.

Is there a stutus update on this issue? Anything I can do to test it?

Is there a simple change I can make to allow users to update values of custom entities? I could either modify core to prevent " SET id={id}" from appearing in entity update SQL, or add something to my custom module to rewrite the SQL statement before it executes. I attempted to use some of the simpler patches on my Drupal 8.4 site, but it caused my site to stop responding.

As an update. I fixed this with the following small change to SqlContentEntityStorage.php

if ($shared_table_fields) {
      $record = $this->mapToStorageRecord($entity->getUntranslated(), $this->baseTable);
      // Create the storage record to be saved.
      if ($update) {
        // 2 new lines here - Cast $record to an array and remove the id element
        $record_a = (array) $record;
        unset($record_a["id"]);

        $default_revision = $entity->isDefaultRevision();
        if ($default_revision) {
          $this->database
            ->update($this->baseTable)
 // replaced this            ->fields((array) $record)
            ->fields($record_a)
            ->condition($this->idKey, $record->{$this->idKey})
            ->execute();
        }

Patch File for my change.

Second Try. Removing DOS line endings.

Adding Missing Newline

Last try before I give up.

Please review and let me know if there is anything I need to do to get this into core.

@Beakerboy Sorry but the last patch is a dirty hack that does not fit the architecture. Please see the work in all the previous posts. At some point this got close to being solved, but needed test coverage. Thanks!

@david_garcia I would say it’s a very clean hack...three lines and breaks absolutely nothing. Everything else has been in the works for years, and continues to fail unit tests. Let me know where you feel the changes should be made and I’ll look into it. I like simple solutions over over-engineered projects. This code efficiently removes the problem, exclusively where it is an issue.

I’ve never used SimpleTest to do unit tests. How would you propose testing this? The way I would approach it would be to find where an entity update is tested, and examine the $query object to ensure that “id” is not in the field array, or to pass $query->__toString() through a regular expression to ensure it does not match “SET.*id=.*WHERE”.

Here is a version that checks the id status of each field, and only removes it if it is the base table's idKey

Missing Line break

This version handles the revision table as well.

The previous version was using an incorrect $record_array. If this works, then the alias storage will need to be updated.

This fixes the issue with the AliasStorage object

The pid element in the $fields array is used elsewhere. Putting it back in, but after the update query is constructed.

Here is an 8.5 patch

Typo in the previous patch. Please offer suggestion on the best way to test this. I'm more than happy to write a test or two. If you feel there is a better way to accomplish this that does not require refactoring several objects, please offer constructive advice. I would love it if someone with a sql server test install could test this patch. The only thing I think I might want to change is to rename $record_array to $fields, for consistency with other calls to the update method.

Is there anyone who could test this patch on a sql server installation? Can anyone recommend a way to unit test this? I have asked on both StackExchange and the drupal slack channel with no responses.

I found a unit test in /core/tests/Drupal/KernelTests/Core/Database/UpdateTest.php which specifically tests that primary keys CAN be updated. This test will need to be removed before SQL Server will pass testing.

Nobody on the Slack channel, stackExchange, the IRC channel, or the support forums here have been able to offer any advice on how to test this fix.

Hello @beakerboy Thanks for the patch.
We have our production running on Windows server and when we used SqlSrv module - we faced this issue.

• I applied the patch https://www.drupal.org/files/issues/not-update-entity-serial-columns-234... in our Drupal installation
• Tested on windows server and now I am able to save the entities.

I will mark this as RTBC. Thanks for this Contribution.

I think that all existing tests continue to work is enough test coverage, agreed that this would be very hard to test without very low-level unit test that would test the actual arguments to the query builder methods.

I also added test runs for PostgreSQL/SQlite.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -842,9 +842,15 @@ protected function doSaveFieldItems(ContentEntityInterface $entity, array $names
+          // We must remove the serial field to prevent it from being included in the

This goes over 80 characters. We should also explicitly mention that updating the serial fails on SQL server here I think.

The comment has been changed.

The only change since #90 is in comments. #90 passed tests, but the test server is out of hard drive space, which is preventing #98 from running.

Test server is back online. PHP 5.6 & MySQL 5.5 passed, others are queued.

Hello @Beakerboy,

I used your Patch - https://www.drupal.org/files/issues/not-update-entity-serial-columns-234....
Everything Worked fine. But when we use Paragraphs, We faced an issue with creation/updation of node.

The error was - Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Cannot update identity column 'revision_id'.: UPDATE paragraphs_item_revision.

So I had to create the new patch and it will fix the issue here. Please review once.

https://www.drupal.org/files/issues/drupal-paragraphs-revision-2342699-1...

I'll have a look ASAP.

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -843,9 +843,16 @@ protected function doSaveFieldItems(ContentEntityInterface $entity, array $names
+            unset($record_array[$this->idKey]);

@@ -855,10 +862,17 @@ protected function doSaveFieldItems(ContentEntityInterface $entity, array $names
+              unset($record_array[$this->revisionKey]);

@@ -1088,9 +1102,16 @@ protected function saveRevision(ContentEntityInterface $entity) {
+        unset($record_array[$this->revisionKey]);

The general direction of this patch is ok IMO, but I think this could be fixed in a single place by adjusting the logic in ::mapToStorageRecord(). We are already dealing with SQL server towards the bottom of the loop, so that seems to be the right place to address this issue. We'd just need to avoid setting the value in $record in the first place, regardless of $value being set, when we are going to perform an update query and we have a serial (identity) column. We can easily check this via ::isNew() or ::isNewRevision() depending on the table being the base table or the revision table.

@plach

Thanks for reviewing it. We would still need those keys in $record array, as we use those Identity column/Primary key as condition in Update queries.

Let me know your thoughts on this.

We have $entity: we can populate conditions via $entity->id() and $entity->getRevisionId().

I've changed the logic in mapToStorageRecord as suggested. Let me know if this looks better. It seems to work on my local install.

last patch was missing the changes outside mapToStoarageRecord()

wrong drupal repo on previous. This one is correct.

Last patch was missing calls to getValue(). This patch seems to fix updates on custom entities, but node updates fail.

This patch probably needs an edit for readability: /r/->getValue()[0]['values']/->value/g

Didn't look at the patch, but $entity->get($name)->value should be enough to retrieve the field value.

Which is preferable, $entity->{$this->idKey}->value or $entity->get($this->idKey)->value?

They are the same, I personally prefer to use ::get() when the field name is stored in a variable, but that's just me :)

> They are the same, I personally prefer to use ::get() when the field name is stored in a variable, but that's just me :)

That's me too, +1.

Can somebody help me figure out why all the patches in #111,#113-#115 are generating so much output as to completely fill the testbot disks? We didn't used to have this problem, and this is the third time this month that some bad patches create *mountains* of output, I suspect its not just an accident here, but somehow we've ratcheted up some logging or error handling that is compounding the issue.

Its causing the disks to fill up to the point that the testbot dispatcher cannot tell if the disks are full, and pull the testbot out of rotation. Which causes it to keep running whatever is in the queue, thus, killing testing for everything thats waiting for a test.

Please be careful when resubmitting any of the patches in #111-115 - Please run it locally first, at least to prove that the error common in all of those result sets is not going to replicate twenty thousand times.

And if anybody has any ideas about what may have recently changed in core to create all this extra output, I would certainly appreciate a pointer.

Thanks.

Here is a 8.4 patch that works on my system for custom entities, nodes, and node revisions. However, it includes a slight API change. The format is now starting to look like what was being done at around patch #12...which is where nobody was ever able to resolve all the testing failures, and prompting me to take a different approach. Maybe it has to do with entity->get($this->idKey)->value not matching up with $record->{$this->idkey}, such that replacing the later with the former causes failures?

I moved the call to fetch the $record inside each of the "update" and "insert" blocks since the necessary parameters will be different.

Please review. I can simplify this patch by using the existing $update variable within doSaveFieldItems() and calling mapToStorageRecord() once like it was originally. Please review and I'll roll the simplification out tomorrow. I don't see a way to accomplish this without changing the API like I did. mapToStorageRecord() would have to redo all the tests that the calling functions do to determine if the $query will be an update or insert.

@Mixologic: posted a reply in old testing issue so as not to clutter this one.

Looking good, thanks!

I still have to review the code more in depth, but this looks promising. A simplification would be welcome of course :)

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
@@ -959,11 +960,15 @@ protected function saveToSharedTables(ContentEntityInterface $entity, $table_nam
+   * @param bool $update
+   *   (optional) If set to true, serial fields will not be included, even if
+   *   their value has been set. This is important for update queries on some
+   *   databases. ¶

Trailing whitespace. Also, let's use TRUE please.

Here is a slightly simplified version. The API change is still in it, but instead of hard-coding a few TRUE s in the function call, I set and passed in an $update variable.

1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -959,11 +959,15 @@ protected function saveToSharedTables(ContentEntityInterface $entity, $table_nam
       *   (optional) The table name to map records to. Defaults to the base table.
   +   * @param bool $update
   +   *   (optional) If set to TRUE, serial fields will not be included, even if
   +   *   their value has been set. This is important for update queries on some
   +   *   databases.
   

   usually update refers to updating an existing entity. But for the revision case, saving as a new revision is also an update in how we use it in general in the entity system.

   If we need this argument then I would recommend a more explicit $include_serial_fields (or something like that) argument.

   The alternative would be a new wrapper method like mapToStorageRecordWithoutSerialFields() but adding a new optional argument to a protected method should be fine IMHO.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
   @@ -1086,19 +1089,17 @@ protected function saveRevision(ContentEntityInterface $entity) {
            ->fields((array) $record)
   -        ->condition($this->revisionKey, $record->{$this->revisionKey})
   +        ->condition($this->revisionKey, $entity->get($this->revisionKey)->value)
   

   if you're working with the entity then you can just use $entity->getRevisionId() ?

@Berdir

1. The $update parameter was already defined in this class as whether or not the SQL statement would be an update statement or not. I just maintained this. I would rather change the name than make a wrapper for simplicity.

2. I can change that, no problem.

1. Yes, it existed in one of two methods :) But that's kind of my point, in the doSaveFieldItems(), $update is correct, it's either a new new entity or not. But in the revision context, the meaning of $update isn't clear, so lets use a more specific variable name for the helper function that explicit about what it does.

Including suggestions from @Berdir

With the end of Drupal 8.4 development...please test this 8.5 patch so we can keep this moving. Thanks!

This is still broken in the main branches. The patch #133 referenced works for us on SQL Server 2016.

This needs merged into at least 8.4.x-dev, 8.5.x-dev and 8.6.x-dev. I haven't checked earlier than 8.4.

I'm not sure we're fixing this at the right level.

I'm wondering why

I guess the database driver could be modified to remove primary key fields from updates, but does not look like a good solution to me.

Was not attempted. The thing is contrib (or custom) will do things like what you see in AliasStorage. For me that makes this a db driver problem and not something that should be fixed in the entity storage level.

If we do decide to go for #133 then I think we should add a test for the new logic in mapToStorageRecord() so can be sure not to break this in the future.

Responding to #136.

IMHO, #133 fixes it in the most correct place. If fixed at the driver level and later there is a need to update a primary key, the driver would be incorrect and we would have to return to #133 and patch it this way anyway. I think it would be ideal if the core did not send unnecessary fields to update to the driver. Also, there's still the fix in the patch for what looks like a line ordering error of $fields['pid'] = $pid in core/lib/Drupal/Core/Path/AliasStorage.php?

@rbrenton the problem is wider than the entity system though. It's anything that updates a record in the database. Most contrib and custom modules are used with MySQL / Postgres and DrupalCI also offers SQLite coverage but we have no automated testing for a DB that behaves this way so if this isn't fixed in the DB layer there's a good chance something will be broken somewhere.

What I'm trying to say is that the restriction on not updating an identity column comes solely from mssql and the entity sql storage implementation will not be the only place this causes problems.

I've discussed this a bit with @amateescu in slack. He's pointed out that fixing this in the db driver level is going to be very expensive - mssql driver will have to get the schema for every table during an update and work stuff out.

I have two concerns:

1. This has no test coverage
2. The return of mapToStorageRecord() is mutable depending on whether you are inserting or updating - that just feels wrong to me.

So here's an even less testable version of #133 but the difference is that it doesn't change mapToStorageRecord(). The changes in that code concern me because it make the result mutable depending on whether you are updating or inserting whereas I think conceptually an entity with identical values should map to the same storage record regardless of whether you are inserting or updating. Therefore I think we should fix it closer to the queries by doing something like the patch attached.

Still no sure how to test this. Posting to see what people think and maybe someone can test on mssql.

So @amateescu pointed out the that @plach asked for the changes to mapToStorageRecord in #108. As stated in #140 I'm not sure this is the right way to go because I don't think it makes sense to have a mutable storage record for different operations. It feels like the wrong axis to change that on.

@alexpott...any updates in your thoughts on this patch, or this issue in general?

I discussed #141 with @alexpott: I'm not completely sold on that argument, but I'm fine with going that way. I'd rather see us making progress on this issue, given that the differences between the two approaches aren't likely to be that relevant in practice.

Regarding test coverage, we both agreed that the proper way to test these changes, would be to run our test suite on SQL server, so no need to add explicit coverage for that. OTOH while discussing possible alternatives to #140, I realized that we are missing an explicit test to cover entity ID assignment on creation, although we likely have implicit coverage for that via migration tests. I'm offering the attached patch as a complement to #140. Feel free to merge them, otherwise I'll open a separate issue for this.

If we don't end up merging the two patches, I'm +1 on RTBC-ing/committing #140.

@platch

Thanks for the update. I will test #140 on mssql shortly. Thinking ahead, do we need to be on the lookout for cases where a user creates an entity with additional IDENTITY fields beyond just the id column? Can a user create and entity with multiple IDENTITY fields? #140 appears to just remove "id", while #133 checks each field with isColumnSerial() and removes it if it is.

I don't think entities were ever intended to have more than one ID field. I cannot completely rule out the possibility that a heavily customized entity type could try to implement two ID fields, but I'm very skeptical it would be possible to do that.

@plach I think it is great to add an explicit test here. And whilst it does not 100% confirm the fix it does prove no regressions so adding it here makes sense. Wrt to multiple serial columns - does anyone have a use-case? We could open a follow-up to deprecate supporting that because it feels that doing multiple serial columns does not make a great deal of sense.

Patch attached merges #140 and #144.

I don't think we need to worry about advanced use cases implying multiple serial columns: this is not how core entities work. An highly specialized entity type requiring multiple serial columns could extend the storage to deal with its use case.

I'm in favor of RTBC-ing #148 as soon as @Beakerboy (or someone else) confirms the patch is good for MSSQL.

The latest patch resolves the issue on my Drupal site. Thanks everyone for all the thought and work over the past 4 years to get this changed!

Committed/pushed to 8.7.x and cherry-picked to 8.6.x, thanks!

FYI https://gist.github.com/chx/eb71941faae245bbf7d0f20747551c21

Some of our custom entities broke on core update (8.6.2 -> 8.6.7) and I tracked the issue to here:

-            ->condition($this->idKey, $record->{$this->idKey})
+            ->condition($this->idKey, $entity->get($this->idKey)->value)

This seems to assume that ID is stored in a property called value and breaks the update statements when using a field where that is not the case.

I’d suggest opening a new bug report. This one is already closed and the patch has been rolled out, so I don’t know if it would be re-opened. If you open a new report you can reference this issue, submit this patch, and run core tests against it. That would probably get more core developers attention since it’s a new report versus a comment on a closed one. Add as much detail as you can to your report, like how your custom entity is designed. It’s possible that there is some requirement that all entities have a ‘value’ property.

It took a year of prodding to get this issue fixed, so it’s possible yours may take some time as well.