Skip to content

Inherit unnamed indexes from mapped superclasses - #12633

Open
SulimanAbdulrazzaq wants to merge 1 commit into
doctrine:3.7.xfrom
SulimanAbdulrazzaq:fix/unnamed-inherited-indexes-3.7.x
Open

SulimanAbdulrazzaq wants to merge 1 commit into
doctrine:3.7.xfrom
SulimanAbdulrazzaq:fix/unnamed-inherited-indexes-3.7.x

Conversation

@SulimanAbdulrazzaq

Copy link
Copy Markdown

Fixes #12179

This is a port of #12630, which targeted 2.20.x, to 3.7.x. The bug is still present in 3.x: addInheritedIndexes() is unchanged, and the attribute and XML drivers still store unnamed indexes and unique constraints under positional keys.

ClassMetadataFactory::addInheritedIndexes() copies the indexes and unique constraints of a mapped superclass into the entity. It skips every key the entity already has, which lets an entity override an inherited index by reusing its name. Indexes declared without a name only get a positional key, though, so every unnamed index the entity declared shadowed the superclass's unnamed index at the same position: declaring one dropped the first inherited index, declaring two dropped the first two, and so on. The generated schema then silently lacked those indexes.

This builds on the patch @speller posted in the issue:

  • Unnamed entries of the superclass are appended to the entity's list, unless the entity already declares an index of the same kind on the same columns or fields. That index then overrides the inherited one, so it is not generated twice.
  • Named entries keep the existing override-by-name behaviour.

One visible change beyond restoring the dropped indexes: an entity that declares a named index on the same columns as an unnamed superclass index now gets only its own index, instead of both.

BasicInheritanceMappingTest gets two tests (group GH-12179). Without the change in ClassMetadataFactory, both fail: the entity's unnamed index replaces the superclass's first unnamed index, and a named entity index sits next to an unnamed superclass index on the same column.

Unnamed indexes and unique constraints only get a positional key, so
addInheritedIndexes() dropped the first unnamed index of a mapped superclass
as soon as the entity declared one of its own. Append unnamed entries of the
superclass unless the entity already declares one on the same columns or
fields, and keep overriding named entries by name.
@SulimanAbdulrazzaq

Copy link
Copy Markdown
Author

@greg0ire this is the 3.7.x port of #12630, which I closed after your note about ORM 2. addInheritedIndexes() still drops a mapped superclass's unnamed indexes and unique constraints when the entity declares unnamed ones of its own (#12179). Could you approve the workflow runs when you have a moment? They are waiting for approval, and the branch has no conflicts.

@SulimanAbdulrazzaq

Copy link
Copy Markdown
Author

The "PHPUnit (fail on deprecations)" failure isn't caused by this change. That job installs doctrine/dbal 4.5.x-dev, and doctrine/dbal#7490 (merged 2026-09-23) deprecates passing a Type instance under the "type" key of a column definition array. The multi-table UPDATE executor still does that, so AdvancedDqlQueryTest::testUpdateAs triggers it. The same job fails the same way on an unmodified 3.7.x (6b47a02): Tests: 3656, Assertions: 12382, Deprecations: 1. Every other job passes, and the full suites show no new deprecations from this PR.

Fixing that one call site isn't enough: with deduplication off, 3.7.x shows about 30 deprecations from dbal#7490, including new Column(..., Type) in DatabaseDriverTest and Column::getType() in DatabaseDriver. So I've left it out of this PR. I'm happy to send the temporary-table part (passing typeName next to type) as its own PR if that helps.

@greg0ire greg0ire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't know how many projects suffer from this. A patch release suddenly generating extra indices makes me uneasy, however since we are talking about new indices, I think it should be OK. It's not like we are modifying a database column. If other maintainers agree, let's merge this, if they don't, then we should retarget to 3.8.x and introduce a configuration knob just for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unnamed indexes from the parent class are ignored if there are unnamed indexes in the subclass

2 participants