Problem/Motivation

This was discovered in #2841291: [PP-1] Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

The stored field schema does not contain:

  • primary keys, if the field is part of the primary key
  • unique keys, unless they are declared by the field type itself (the UUID field does this, for example)
  • indexes, unless they are declared by the field type or the field itself (for example composite indexes are currently missing)

This means that the field schema is not sufficient when updating field storage definitions. We instead need to fetch the entity schem which is unideal and potentially problematic due differing states of the entity type and field storage definitions.

Proposed resolution

Let's add them!

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

Status: Active » Needs review
FileSize
5.68 KB
30.61 KB

Here we go. Extracted this from #2928906: [PP-1] The field schema incorrectly stores serial fields as int. Interdiff is relative to #4 over there and should hopefully fix the tests. Could be that I messed something up in the split though...

Status: Needs review » Needs work

The last submitted patch, 2: 2929120-2.patch, failed testing. View results

mayurjadhav’s picture

Issue tags: +DrupalMumbaiCodeSprint

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tstoeckler’s picture

Title: The field schema is missing field-level primary keys, unique keys and indexes » [PP-1] The field schema is missing field-level primary keys, unique keys and indexes
Status: Needs work » Postponed

Looked at the test failures, but similarly to #2928906: [PP-1] The field schema incorrectly stores serial fields as int this requires #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field because we are updating the field storage definitions as part of the update, so we need to recreate the primary keys.

tstoeckler’s picture

Status: Postponed » Needs review
FileSize
31.43 KB
47.3 KB

OK, so here's a re-roll on top of #33 from #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field.

Once we get a green here, we can start tackling those todos from that issue that point here to prove that they are warranted.

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
@@ -1384,8 +1369,8 @@ protected function createSharedTableSchema(FieldStorageDefinitionInterface $stor
+              if (!empty($schema[$table_name]['primary key']) && in_array($name, $entity_schema[$table_name]['primary key'], TRUE)) {

Ahh borked the re-roll here. This should be $schema in both cases, and we should remove $entity_schema altogether here. Should in theory not affect green-ness, though. We'll see...

Status: Needs review » Needs work

The last submitted patch, 8: 2929120-8--for-robots.patch, failed testing. View results

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
3.54 KB
58.29 KB
73.02 KB

This fixes #9 and one other thing I borked up in the re-roll + bumps the update functions to 8.6

tstoeckler’s picture

index 0000000000..5a86d59e2c
--- /dev/null

--- /dev/null
+++ b/2929120-2.patch

index 0000000000..740c90403a
--- /dev/null

--- /dev/null
+++ b/test.php

Sorry, that's stupid. Will leave that out of the next one.

Status: Needs review » Needs work

The last submitted patch, 11: 2929120-11--for-robots.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
4.48 KB
32.49 KB
47.12 KB

This should fix at least some of the fails.

Of course, I ran into #2928906: [PP-1] The field schema incorrectly stores serial fields as int here. Adding more duct-tape for now in order to keep those separate.

tstoeckler’s picture

FileSize
47.12 KB

D'uh, wrong extension. Here you go, little bot.

Status: Needs review » Needs work

The last submitted patch, 15: 2929120-15--for-robots.patch, failed testing. View results

tstoeckler’s picture

The last submitted patch, 17: 2929120-17--for-robots.patch, failed testing. View results

tstoeckler’s picture

So this revealed a bug, yay ;-): When we change the serial field to an int during an update of a field definition we never actually change it back. So we should do that. ...except not for user entities of course, as those do not use serial field at all. Hah, this is so much fun... ;-)

Unless I broke anything else - which is entirely possible - this should be green. Will then attempt to remove some of the hacks introduced by #2938951: The primary key is not correctly re-created when updating the storage definition of an identifier field.

tstoeckler’s picture

tstoeckler’s picture

Now testing my theory that the ugly workaround in SqlContentEntityStorageSchema::processBaseTable() is only needed due to this.