After fixing this issue SqlContentEntityStorage tries to update identity/serial values by default, there is still the outstanding issue of Drupal mandating the ability to change identity values in the unit tests. The function testPrimaryKeyUpdate() is in the file core/tests/Drupal/KernelTests/Core/Database/UpdateTest.php. SQL Server does not allow users to update identity column values. Could this test either be flagged for core databases only, or removed? If anything, I would think we would want to make sure identity values are not changed, to ensure referential integrity.

CommentFileSizeAuthor
#3 drupal-3108287-Remove_or_mark_as_core_only_testPrimaryKeyUpdate-3.patch1.07 KBbeakerboy

Comments

Beakerboy created an issue. See original summary.

daffie’s picture

daffie commented
Issue tags: +Contrib database driver
Related issues: +#1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed

The test testPrimaryKeyUpdate was added in #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed.
The question is do we need this functionality.
I do not see why you want to update a primary key value in an update on a Drupal website.
I can see that there would be a need for something like that, but not on a Drupal website.
My suggestion would be to create a patch to remove the test and create a change record that explicitly states that updating primary key values is not supported by Drupal.

beakerboy’s picture

beakerboy
Primary language English
Location Kentucky
commented
Status: Active » Needs review
StatusFileSize
new drupal-3108287-Remove_or_mark_as_core_only_testPrimaryKeyUpdate-3.patch1.07 KB

Patch to remove test.

daffie’s picture

daffie commented

@Beakerboy: I am not sure what "identity columns" are, but we change the condition to be on the name field, will it then work for SQL Server or not?

beakerboy’s picture

beakerboy
Primary language English
Location Kentucky
commented

@daffie: Identity columns are primary keys. This test is to ensure that the database is able to update the value of the primary key. Changing the field to the name WILL lead to a successful test, but this is not the intention of this test case. There are plenty of other tests that ensure that a non-primary-key fields can be updated. SQL Server (and other databases) do not allow the user to update the value of the primary key because this can break referential integrity. The core supported database are more flexible in this regard.

daffie’s picture

daffie commented

@Beakerboy: So if I understand you correctly then updating a field while the same field is also in the condition is not a problem? The original issue was about updating fields that are also in the condition. If we change the test to that, that would be better. What is your idea?

beakerboy’s picture

beakerboy
Primary language English
Location Kentucky
commented

We’re having some sort of communication lapse, so I apologize in advance if it sounds like I’m talking with my 7-year-old.

Database tables (A) often have a primary key field, typically named ‘id’. This is a unique number associated with the row of the data table. When a table is created with a primary key, it is noted as such in the table schema because it has special characteristics. It typically cannot be NULL, must be unique, and is often a ‘serial’ data type (int and auto-incrementing)

If another data table (B) wants to refer to the data in table A, the ‘id’ can be used a foreign key...so table B will have a foreign key in some row identical to a specific primary key in A.

The Drupal test suite tests that we are able to create an UPDATE statement which will change the value of the ID field in table A. MySQL is super loosely-goosey on ‘rules’ and will let a user do it. PostgreSQL is a little more strict in that the table can be designed to “cascade” changes to the foreign keys, restrict the ability to change them, or allow it. MSSQL completely disallows primary key updates.

My proposal (In my patch) is to remove the test because I do not feel that Core should be requiring that primary keys can be changed as a requirement for successful operation. There are no actual situations in the the core code where updating a primary key to a new value is performed. In fact, if a primary key is changed without removing the data that refers to it as a foreign key, then the relationship is lost.

daffie’s picture

daffie commented

My idea was something like changing the name of the test to testUpdateFieldInCondition and changing the query to:

$num_updated = $this->connection->update('test')
 ->fields(['id' => 42, 'name' => 'Eric'])
 ->condition('name', 'John')
 ->execute();

The original issue where the test came from is about updating fields that are in the condition. And they added a test for a primary key field, but it does not have to be a primary key field. If we change the test this way it test the original issue and the SQL server does not choke on it.
Everybody happy!

beakerboy’s picture

beakerboy
Primary language English
Location Kentucky
commented

You would have to change the test name too. As I said above, the case you are proposing is already covered. See testMultiUpdate() in the same file.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

@Beakerboy: You are right, there is already such a test.

The test that this issue wants to remove has been added by #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed. The test coverage for that issue can be found in Drupal\KernelTests\Core\Database\testMultiUpdate(). The test that this issue wants to remove does something that is AFAIK not necessary for Drupal projects. It is not supported by MSSQL and therefore a blocker in running tests with MSSQL.
For me it is RTBC.

  • catch committed 748111e on 9.0.x 
    Issue #3108287 by Beakerboy, daffie: Remove, or mark as core only:...

  • catch committed 4999a09 on 8.8.x 
    Issue #3108287 by Beakerboy, daffie: Remove, or mark as core only:...

  • catch committed afa79e3 on 8.9.x 
    Issue #3108287 by Beakerboy, daffie: Remove, or mark as core only:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Makes sense to me to just remove the test method.

Committed/pushed to 9.0.x/8.9.x/8.8.x, thanks!

Status: Fixed » Closed (fixed)

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