Problem/Motivation

#2616724: Warn when trying to create a database table with a NOT NULL => FALSE primary key introduced a check which flags invalid primary keys in schema specifications. This check does not work for composite primary keys if they specify an explicit prefix length.

Proposed resolution

Fix Schema::ensureNotNullPrimaryKey for primary key specifications with explicit prefix length.

Remaining tasks

  1. Tests
  2. Fix
  3. Review
  4. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#9 3068310-composite-primary-key-prefix-length-8.patch3.43 KBznerol
#6 3068310-composite-primary-key-prefix-length-6.patch3.43 KBznerol
#6 interdiff.txt1.1 KBznerol
#2 3068310-composite-primary-key-prefix-length.patch2.96 KBznerol
#2 interdiff.txt950 bytesznerol
composite-primary-key-prefix-length-TEST-ONLY.patch2.03 KBznerol

Comments

znerol created an issue. See original summary.

znerol’s picture

znerol commented
StatusFileSize
new interdiff.txt950 bytes
new 3068310-composite-primary-key-prefix-length.patch2.96 KB

Fix might look something like this.

The last submitted patch, composite-primary-key-prefix-length-TEST-ONLY.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 2: 3068310-composite-primary-key-prefix-length.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Hmm... tests without assertions are no longer allowed, maybe just collapse the two new test methods into one? Not sure what the best solution is.

Fix looks good, though.

znerol’s picture

znerol commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.1 KB
new 3068310-composite-primary-key-prefix-length-6.patch3.43 KB

Or just add an assertion. Regrettably there is no Schema::primaryKeyExists() but we can use Schema::addPrimaryKey and look for the exception.

Status: Needs review » Needs work

The last submitted patch, 6: 3068310-composite-primary-key-prefix-length-6.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented 
+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
@@ -961,6 +961,52 @@ public function testInvalidPrimaryKeyChange() {
+  public function testCompositePrimaryKeyWithPrefixLengthOnTableCreation() {

Actually, I think this is the test that needs an assertion. The other one is fine because setExpectedException() counts as an "assertion" as far as I know.

znerol’s picture

znerol commented
Status: Needs work » Needs review
StatusFileSize
new 3068310-composite-primary-key-prefix-length-8.patch3.43 KB

Right.

znerol’s picture

znerol commented
Issue summary: View changes

Patch still applies.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Thanks for the bump. still looks good, and test makes sense, as well. Let's do it!

mondrake’s picture

mondrake
Location 🇮🇹
commented

A fundamental/conceptual question here: are we sure we want to support partial lenght columns in a unique key context here (primary/unique keys)? IMHO it seems weird you design a table with fields that you want to be kind of unique via PK, then to have a PK that would fatally fail when trying to insert records that lead to duplicate key, which can happen if your field varies in the part that is not part of the key -- how are we going to decide which is the 'right' key and record?

Recently, Doctrine DBAL have introduced partial length support to indexes https://github.com/doctrine/dbal/pull/2412, but have done so only for indexes, that allow duplicated keys - not for primary/unique keys.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Agreed with #12, this seems like the wrong direction to me.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Filed #3082239: Forbid limited length primary and unique keys, allow only in indexes, which is basically the opposite of what is being done here.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3082239: Forbid limited length primary and unique keys, allow only in indexes

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Closed (won't fix)

I suggest to wont fix this.

znerol’s picture

znerol commented

I suggest to wont fix this.

Because the patch is too trivial? Or because the bug is too obscure?

znerol’s picture

znerol commented

Oh, I somehow missed #12. I completely agree, this is wontfix.