Problem/Motivation

As discussed in #3068310: Composite primary key with specified index length fails the NOT NULL check, limited length keys are applicable only to indexes, not to primary and unique keys.

See #11 there:

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.

Proposed resolution

  • Deprecate createion of PKs or unique keys with limited length in 10.2 then throw a SchemaException or similar in 11.
  • Update database API docs accordingly.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#45 3082239-nr-bot_yx4n867l.txt548 bytesneeds-review-queue-bot
#40 3082239-nr-bot_n3x53e0y.txt90 bytesneeds-review-queue-bot
#38 3082239-nr-bot_b2lk7y8k.txt4.55 KBneeds-review-queue-bot
#36 3082239-nr-bot_u26c1a9o.txt6.37 KBneeds-review-queue-bot
#12 3082239-12-test-only.patch2.7 KBmondrake
#11 3082239-11-test-only.patch2.91 KBmondrake

Issue fork drupal-3082239

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mondrake created an issue. See original summary.

cilefen’s picture

cilefen commented
Issue tags: +Documentation

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.

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake opened merge request !1126

mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: Unassigned » mondrake
Status: Active » Needs work

working on this

daffie’s picture

daffie commented

@mondrake: I cannot find any support for this in the PostgreSQL and the SQLite documentation. AFAIK Doctrine DBAL only supports this on MySQL.

mondrake’s picture

mondrake
Location 🇮🇹
commented

@daffie I think here we should not look at what databases allow or not, but take a decision on what we want to support in Drupal: do we allow partial length for the columns in unique key (unique/primary) contexts, or not?

BTW - I have serious doubts that trying to specify a primary key with partial column length works at all in HEAD right now. It's not tested, though. Here we'd be simply certifying that and adding the testing.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 3082239-11-test-only.patch2.91 KB

A test only patch to check #10 re. what happens specifying a primary key with partial column length in HEAD on all the dbs.

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 3082239-12-test-only.patch2.7 KB
mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs change record

Ready for review.

With this patch, we clean up the situation a bit.

  1. We communicate that we will not support partial length keys in primary or unique keys.
  2. If a primary/unique key specification includes partial length fields:
    • in Drupal 9.3 we emit a deprecation error, but nevertheless process the request, disregarding the partial length specification, and including the columns in their full length
    • in Drupal 10.0, passing a partial length specification will throw a SchemaException
  3. we add testing cases for primary/unique keys creation for Schema::createTable(), Schema::addPrimaryKey(), Schema::addUniqueKey(), Schema::addField(), Schema::changeField()
  4. we deprecate Schema::ensureNotNullPrimaryKey() in favour of a more generic Schema::validatePrimaryKeySchema() method

This needs a CR and its number to be reflected in the MR.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#2278493: Drop support for unique keys specified as an array of column name and length (breaks PG installation), +#1148856: Postgres schema doesn't support keylength on a unique index, +#3068310: Composite primary key with specified index length fails the NOT NULL check
mondrake’s picture

mondrake
Location 🇮🇹
commented

BTW - there are no cases of partial length primary/unique keys in core HEAD at the moment, so this is entirely related to clarifying the status on behalf of contrib.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Version: 9.5.x-dev » 10.1.x-dev
mondrake’s picture

mondrake
Location 🇮🇹
commented

rebased

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Rebased and adjusted

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Wonder if the title/issue summary could be updated. Since it's applying to more then documents. Also see it was tagged for a CR if that could happen too please.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Clarify in docs that limited length keys apply to indexes only, enforce in code » Forbid limited length primary and unique keys, allow only for indexes only.
Assigned: Unassigned » mondrake
Issue summary: View changes
Issue tags: -Documentation

Trying to make issue title and summary clearer.

Agreed this is not a Documentation only issue, removed tag - docs need update because these changes are applied.

Working on CR and patch update.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Forbid limited length primary and unique keys, allow only for indexes only. » Forbid limited length primary and unique keys, allow only in indexes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Assigned: mondrake » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs change record

Added a draft CR, https://www.drupal.org/node/3375071

smustgrave’s picture

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

Thanks @mondrake! Much clearer.

Wasn't entirely sure how to test so I did something similar to testInvalidPrimaryKeyColumnsOnCreateTable and your CR and created a table with a a primary key of 10. And got the exception as expected.

The CR was clear of the change and included great examples.

Think this is ready for committers to take a look.

catch’s picture

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

Mostly looks good but a handful of comments on the MR.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Replied inline and adjusted the deprecation messages.

smustgrave’s picture

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

lets find out!

catch’s picture

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

I'm pretty sure it won't, so we should either duplicate the documentation, or add @see in the relevant places, and/or file an api.drupal.org issue to support it.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Related issues: +#3376270: Support PHPStan's annotations for local type aliases and array shapes

Filed #3376270: Support PHPStan's annotations for local type aliases and array shapes in the API module issue queue - if in the future we want to use PHPStan to higher levels we can't be stopped by that, so better request a change there now.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3389202: migrate\Plugin\migrate\id_map\Sql assumes a PDO db driver

Tests for index limits for pgsql/sqlite requested in #3389202-10: migrate\Plugin\migrate\id_map\Sql assumes a PDO db driver

mondrake’s picture

mondrake
Location 🇮🇹
commented

@fjgarlin I commented inline - can you please elaborate the suggestion?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3082239-nr-bot_u26c1a9o.txt6.37 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3082239-nr-bot_b2lk7y8k.txt4.55 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3082239-nr-bot_n3x53e0y.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
mondrake’s picture

mondrake
Location 🇮🇹
commented

#3411490: Replace array-based DB Schema API with a value object structure and #3558426: Add property validation for SchemaDefinition value objects would make most of this duplicate.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

smustgrave commented

@daffie curious your thoughts on the MR?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3082239-nr-bot_yx4n867l.txt548 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented

Wanted to follow up on this one if any of the previous reviewers had a look at the latest changes?

daffie’s picture

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

All the code changes look good to me.
The required tests have been added.
The IS and the CR are in order.
For me it is RTBC.

quietone’s picture

quietone commented
Status: Reviewed & tested by the community » Needs work

All the deprecation messages state 11.3.0, so they need to be changed.

quietone’s picture

quietone commented
Issue tags: +Needs change record updates

Forgot to add that I updated credit.

Also, the change record is out of date.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record updates

#49 and #50 done. Hopefully for the last time, the original CR was prepared for 10.2.