[GH-8229] Prevent Illegal Inheritance Override (#8348)

* [GH-8229] Prevent AttributeOverride on fields from entities, only allowed for MappedSuperclass

* [GH-8229] Prevent AssociationOverride on fields from entities, only allowed for MappedSuperclass

* Revert "Fix SQL alias generation regression for simple inheritance (#8329)"

This reverts commit f4ebded.

* [GH-8229] Finalize checks for illegal attribute/assocation overrides.

* [GH-8229] Revert ccae8f7 PR #8234

* [GH-8229] Update documentation to clarify only mapped superclass or trait works with overrides

* [GH-8229] Fix style violations introduced by revert

* [GH-8229] Fix style violations introduced by revert

* [GH-8229] Temporarily disable the exception until 2.8.

* Make phpcs happy

docs/en/reference/inheritance-mapping.rst

@@ -289,9 +289,15 @@ column and cascading on delete.
 
 Overrides
 ---------
-Used to override a mapping for an entity field or relationship.
-May be applied to an entity that extends a mapped superclass
-to override a relationship or field mapping defined by the mapped superclass.
+
+Used to override a mapping for an entity field or relationship.  Can only be
+applied to an entity that extends a mapped superclass or uses a trait to
+override a relationship or field mapping defined by the mapped superclass or
+trait.
+
+It is not possible to override attributes or associations in entity to entity
+inheritance scenarios, because this can cause unforseen edge case behavior and
+increases complexity in ORM internal classes.
 
 
 Association Override

lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php

@@ -30,7 +30,7 @@
 use ReflectionClass;
 use ReflectionProperty;
 use RuntimeException;
-use function array_key_exists;
+use function count;
 use function explode;
 
 /**
@@ -2193,6 +2193,13 @@ public function setAssociationOverride($fieldName, array $overrideMapping)
 
         $mapping = $this->associationMappings[$fieldName];
 
+        //if (isset($mapping['inherited']) && (count($overrideMapping) !== 1 || ! isset($overrideMapping['fetch']))) {
+            // TODO: Deprecate overriding the fetch mode via association override for 3.0,
+            // users should do this with a listener and a custom attribute/annotation
+            // TODO: Enable this exception in 2.8
+            //throw MappingException::illegalOverrideOfInheritedProperty($this->name, $fieldName);
+        //}
+
         if (isset($overrideMapping['joinColumns'])) {
             $mapping['joinColumns'] = $overrideMapping['joinColumns'];
         }
@@ -2251,6 +2258,11 @@ public function setAttributeOverride($fieldName, array $overrideMapping)
 
         $mapping = $this->fieldMappings[$fieldName];
 
+        //if (isset($mapping['inherited'])) {
+            // TODO: Enable this exception in 2.8
+            //throw MappingException::illegalOverrideOfInheritedProperty($this->name, $fieldName);
+        //}
+
         if (isset($mapping['id'])) {
             $overrideMapping['id'] = $mapping['id'];
         }
@@ -2267,12 +2279,6 @@ public function setAttributeOverride($fieldName, array $overrideMapping)
             throw MappingException::invalidOverrideFieldType($this->name, $fieldName);
         }
 
-        // Fix for bug GH-8229 (id column from parent class renamed in child class):
-        // The contained 'inherited' information was accidentally deleted by the unset() call below.
-        if (array_key_exists('inherited', $this->fieldMappings[$fieldName])) {
-            $overrideMapping['inherited'] = $this->fieldMappings[$fieldName]['inherited'];
-        }
-
         unset($this->fieldMappings[$fieldName]);
         unset($this->fieldNames[$mapping['columnName']]);
         unset($this->columnNames[$mapping['fieldName']]);

lib/Doctrine/ORM/Mapping/MappingException.php

@@ -19,6 +19,8 @@
 
 namespace Doctrine\ORM\Mapping;
 
+use function sprintf;
+
 /**
  * A MappingException indicates that something is wrong with the mapping setup.
  *
@@ -820,4 +822,16 @@ public static function infiniteEmbeddableNesting($className, $propertyName)
             )
         );
     }
+
+    public static function illegalOverrideOfInheritedProperty($className, $propertyName)
+    {
+        return new self(
+            sprintf(
+                'Override for %s::%s is only allowed for attributes/associations ' .
+                'declared on a mapped superclass or a trait.',
+                $className,
+                $propertyName
+            )
+        );
+    }
 }

lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php

@@ -37,7 +37,6 @@
 use Doctrine\ORM\UnitOfWork;
 use Doctrine\ORM\Utility\IdentifierFlattener;
 use Doctrine\ORM\Utility\PersisterHelper;
-use function array_key_exists;
 use function array_map;
 use function array_merge;
 use function assert;
@@ -448,26 +447,14 @@ protected final function updateTable($entity, $quotedTableName, array $updateDat
             $types[]    = $this->columnTypes[$columnName];
         }
 
-        $where                = [];
-        $identifier           = $this->em->getUnitOfWork()->getEntityIdentifier($entity);
-        $quotedClassTableName = $this->quoteStrategy->getTableName($this->class, $this->platform);
+        $where      = [];
+        $identifier = $this->em->getUnitOfWork()->getEntityIdentifier($entity);
 
         foreach ($this->class->identifier as $idField) {
             if ( ! isset($this->class->associationMappings[$idField])) {
                 $params[] = $identifier[$idField];
                 $types[]  = $this->class->fieldMappings[$idField]['type'];
-
-                // Fix for bug GH-8229 (id column from parent class renamed in child class):
-                // This method is called with the updated entity, but with different table names
-                // (the entity table name or a table name of an inherited entity). In dependence
-                // of the used table, the identifier name must be adjusted.
-                $class = $this->class;
-                if (isset($class->fieldMappings[$idField]['inherited']) && $quotedTableName !== $quotedClassTableName) {
-                    $className = $this->class->fieldMappings[$idField]['inherited'];
-                    $class     = $this->em->getClassMetadata($className);
-                }
-
-                $where[] = $this->quoteStrategy->getColumnName($idField, $class, $this->platform);
+                $where[]  = $this->quoteStrategy->getColumnName($idField, $this->class, $this->platform);
 
                 continue;
             }
@@ -645,18 +632,7 @@ protected function prepareUpdateData($entity)
             $newVal = $change[1];
 
             if ( ! isset($this->class->associationMappings[$field])) {
-                $class = $this->class;
-
-                // Fix for bug GH-8229 (id column from parent class renamed in child class):
-                // Get the correct class metadata
-                foreach ($class->parentClasses as $parentClassName) {
-                    $parentClass = $this->em->getClassMetadata($parentClassName);
-                    if (array_key_exists($field, $parentClass->fieldMappings)) {
-                        $class = $parentClass;
-                    }
-                }
-
-                $fieldMapping = $class->fieldMappings[$field];
+                $fieldMapping = $this->class->fieldMappings[$field];
                 $columnName   = $fieldMapping['columnName'];
 
                 $this->columnTypes[$columnName] = $fieldMapping['type'];
@@ -1696,17 +1672,9 @@ public function getSelectConditionStatementSQL($field, $value, $assoc = null, $c
     private function getSelectConditionStatementColumnSQL($field, $assoc = null)
     {
         if (isset($this->class->fieldMappings[$field])) {
-            // Fix for bug GH-8229 (id column from parent class renamed in child class):
-            // Use the correct metadata and name for the id column
-            if (isset($this->class->fieldMappings[$field]['inherited']) && $this->class->inheritanceType !== ClassMetadata::INHERITANCE_TYPE_NONE) {
-                $className = $this->class->fieldMappings[$field]['inherited'];
-                $class     = $this->em->getClassMetadata($className);
-            } else {
-                $className = $this->class->name;
-                $class     = $this->class;
-            }
+            $className = $this->class->fieldMappings[$field]['inherited'] ?? $this->class->name;
 
-            return [$this->getSQLTableAlias($className) . '.' . $this->quoteStrategy->getColumnName($field, $class, $this->platform)];
+            return [$this->getSQLTableAlias($className) . '.' . $this->quoteStrategy->getColumnName($field, $this->class, $this->platform)];
         }
 
         if (isset($this->class->associationMappings[$field])) {

lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php

@@ -26,8 +26,8 @@
 
 use Doctrine\Common\Collections\Criteria;
 use Doctrine\ORM\Utility\PersisterHelper;
+
 use function array_combine;
-use function array_reverse;
 
 /**
  * The joined subclass persister maps a single entity instance to several tables in the
@@ -273,42 +273,35 @@ public function update($entity)
     public function delete($entity)
     {
         $identifier = $this->em->getUnitOfWork()->getEntityIdentifier($entity);
+        $id         = array_combine($this->class->getIdentifierColumnNames(), $identifier);
 
         $this->deleteJoinTableRecords($identifier);
 
-        // Delete parent entries in reverse order (root first, until the direct parent of the current class).
-        //
-        // If foreign key are supported (and set as well as active), the first deletion will cascade and the
-        // following queries won't delete anything, but it's better not to rely on a possible foreign key
-        // functionality which cannot be checked here (foreign keys can be missing or disabled although
-        // supported in general).
-        //
-        // On the other hand the supportsForeignKeyConstraints() method of the platform is also not reliable
-        // when it returns false, because it only means that Doctrine\DBAL doesn't support foreign keys for
-        // this platform, but they may be set in an existing database structure or added manually, so the
-        // previous order of deletion (current class to root) would have failed - this could i.e. happen with
-        // SQLite where foreign keys are theoretically possible but not supported by DBAL.
-        $deleted = false;
-        foreach (array_reverse($this->class->parentClasses) as $parentClass) {
+        // If the database platform supports FKs, just
+        // delete the row from the root table. Cascades do the rest.
+        if ($this->platform->supportsForeignKeyConstraints()) {
+            $rootClass = $this->em->getClassMetadata($this->class->rootEntityName);
+            $rootTable = $this->quoteStrategy->getTableName($rootClass, $this->platform);
+            $rootTypes = $this->getClassIdentifiersTypes($rootClass);
+
+            return (bool) $this->conn->delete($rootTable, $id, $rootTypes);
+        }
+
+        // Delete from all tables individually, starting from this class' table up to the root table.
+        $rootTable = $this->quoteStrategy->getTableName($this->class, $this->platform);
+        $rootTypes = $this->getClassIdentifiersTypes($this->class);
+
+        $affectedRows = $this->conn->delete($rootTable, $id, $rootTypes);
+
+        foreach ($this->class->parentClasses as $parentClass) {
             $parentMetadata = $this->em->getClassMetadata($parentClass);
             $parentTable    = $this->quoteStrategy->getTableName($parentMetadata, $this->platform);
             $parentTypes    = $this->getClassIdentifiersTypes($parentMetadata);
-            $parentId       = array_combine($parentMetadata->getIdentifierColumnNames(), $identifier);
 
-            $affectedRows = $this->conn->delete($parentTable, $parentId, $parentTypes);
-            $deleted      = $deleted ?: ($affectedRows > 0);
+            $this->conn->delete($parentTable, $id, $parentTypes);
         }
 
-        // Now delete the entries from the current class (if not deleted automatically
-        // because of a foreign key).
-        $currentTable = $this->quoteStrategy->getTableName($this->class, $this->platform);
-        $currentTypes = $this->getClassIdentifiersTypes($this->class);
-        $currentId    = array_combine($this->class->getIdentifierColumnNames(), $identifier);
-
-        $affectedRows = $this->conn->delete($currentTable, $currentId, $currentTypes);
-        $deleted      = $deleted ?: ($affectedRows > 0);
-
-        return $deleted;
+        return (bool) $affectedRows;
     }
 
     /**
@@ -421,11 +414,8 @@ protected function getLockTablesSql($lockMode)
             $parentClass    = $this->em->getClassMetadata($parentClassName);
             $joinSql       .= ' INNER JOIN ' . $this->quoteStrategy->getTableName($parentClass, $this->platform) . ' ' . $tableAlias . ' ON ';
 
-            // Fix for bug GH-8229 (id column from parent class renamed in child class):
-            // Use the correct name for the id column as named in the parent class.
-            $parentIdentifierColumns = $parentClass->getIdentifierColumnNames();
-            foreach ($identifierColumns as $index => $idColumn) {
-                $conditions[] = $baseTableAlias . '.' . $idColumn . ' = ' . $tableAlias . '.' . $parentIdentifierColumns[$index];
+            foreach ($identifierColumns as $idColumn) {
+                $conditions[] = $baseTableAlias . '.' . $idColumn . ' = ' . $tableAlias . '.' . $idColumn;
             }
 
             $joinSql .= implode(' AND ', $conditions);
@@ -601,11 +591,9 @@ private function getJoinSql($baseTableAlias)
             $tableAlias   = $this->getSQLTableAlias($parentClassName);
             $joinSql     .= ' INNER JOIN ' . $this->quoteStrategy->getTableName($parentClass, $this->platform) . ' ' . $tableAlias . ' ON ';
 
-            // Fix for bug GH-8229 (id column from parent class renamed in child class):
-            // Use the correct name for the id column as named in the parent class.
-            $parentIdentifierColumn = $parentClass->getIdentifierColumnNames();
-            foreach ($identifierColumn as $index => $idColumn) {
-                $conditions[] = $baseTableAlias . '.' . $idColumn . ' = ' . $tableAlias . '.' . $parentIdentifierColumn[$index];
+
+            foreach ($identifierColumn as $idColumn) {
+                $conditions[] = $baseTableAlias . '.' . $idColumn . ' = ' . $tableAlias . '.' . $idColumn;
             }
 
             $joinSql .= implode(' AND ', $conditions);
@@ -618,11 +606,8 @@ private function getJoinSql($baseTableAlias)
             $tableAlias  = $this->getSQLTableAlias($subClassName);
             $joinSql    .= ' LEFT JOIN ' . $this->quoteStrategy->getTableName($subClass, $this->platform) . ' ' . $tableAlias . ' ON ';
 
-            // Fix for bug GH-8229 (id column from parent class renamed in child class):
-            // Use the correct name for the id column as named in the parent class.
-            $subClassIdentifierColumn = $subClass->getIdentifierColumnNames();
-            foreach ($identifierColumn as $index => $idColumn) {
-                $conditions[] = $baseTableAlias . '.' . $idColumn . ' = ' . $tableAlias . '.' . $subClassIdentifierColumn[$index];
+            foreach ($identifierColumn as $idColumn) {
+                $conditions[] = $baseTableAlias . '.' . $idColumn . ' = ' . $tableAlias . '.' . $idColumn;
             }
 
             $joinSql .= implode(' AND ', $conditions);

lib/Doctrine/ORM/Query/SqlWalker.php

@@ -366,12 +366,8 @@ private function _generateClassTableInheritanceJoins($class, $dqlAlias)
 
             $sqlParts = [];
 
-            // Fix for bug GH-8229 (id column from parent class renamed in child class):
-            // Use the correct name for the id column as named in the parent class.
-            $identifierColumn       = $this->quoteStrategy->getIdentifierColumnNames($class, $this->platform);
-            $parentIdentifierColumn = $this->quoteStrategy->getIdentifierColumnNames($parentClass, $this->platform);
-            foreach ($identifierColumn as $index => $idColumn) {
-                $sqlParts[] = $baseTableAlias . '.' . $idColumn . ' = ' . $tableAlias . '.' . $parentIdentifierColumn[$index];
+            foreach ($this->quoteStrategy->getIdentifierColumnNames($class, $this->platform) as $columnName) {
+                $sqlParts[] = $baseTableAlias . '.' . $columnName . ' = ' . $tableAlias . '.' . $columnName;
             }
 
             // Add filters on the root class
@@ -665,21 +661,11 @@ public function walkPathExpression($pathExpr)
                 $dqlAlias = $pathExpr->identificationVariable;
                 $class = $this->queryComponents[$dqlAlias]['metadata'];
 
-                // Fix for bug GH-8229 (id column from parent class renamed in child class):
-                // Use the correct name for the id column as named in the inherited class.
-                $mapping = $class->fieldMappings[$fieldName];
-                if (isset($mapping['inherited'])) {
-                    $inheritedClass   = $this->em->getClassMetadata($mapping['inherited']);
-                    $quotedColumnName = $this->quoteStrategy->getColumnName($fieldName, $inheritedClass, $this->platform);
-                } else {
-                    $quotedColumnName = $this->quoteStrategy->getColumnName($fieldName, $class, $this->platform);
-                }
-
                 if ($this->useSqlTableAliases) {
                     $sql .= $this->walkIdentificationVariable($dqlAlias, $fieldName) . '.';
                 }
 
-                $sql .= $quotedColumnName;
+                $sql .= $this->quoteStrategy->getColumnName($fieldName, $class, $this->platform);
                 break;
 
             case AST\PathExpression::TYPE_SINGLE_VALUED_ASSOCIATION:
@@ -1419,19 +1405,13 @@ public function walkSelectExpression($selectExpression)
                         continue;
                     }
 
-                    // Fix for bug GH-8229 (id column from parent class renamed in child class):
-                    // Use the correct name for the id column as named in the inherited class.
-                    if (isset($mapping['inherited'])) {
-                        $inheritedClass   = $this->em->getClassMetadata($mapping['inherited']);
-                        $tableName        = $inheritedClass->getTableName();
-                        $quotedColumnName = $this->quoteStrategy->getColumnName($fieldName, $inheritedClass, $this->platform);
-                    } else {
-                        $tableName        = $class->getTableName();
-                        $quotedColumnName = $this->quoteStrategy->getColumnName($fieldName, $class, $this->platform);
-                    }
+                    $tableName = (isset($mapping['inherited']))
+                        ? $this->em->getClassMetadata($mapping['inherited'])->getTableName()
+                        : $class->getTableName();
 
-                    $sqlTableAlias = $this->getSQLTableAlias($tableName, $dqlAlias);
-                    $columnAlias   = $this->getSQLColumnAlias($mapping['columnName']);
+                    $sqlTableAlias    = $this->getSQLTableAlias($tableName, $dqlAlias);
+                    $columnAlias      = $this->getSQLColumnAlias($mapping['columnName']);
+                    $quotedColumnName = $this->quoteStrategy->getColumnName($fieldName, $class, $this->platform);
 
                     $col = $sqlTableAlias . '.' . $quotedColumnName;
 

lib/Doctrine/ORM/Tools/SchemaTool.php

@@ -31,7 +31,6 @@
 use Doctrine\ORM\ORMException;
 use Doctrine\ORM\Tools\Event\GenerateSchemaTableEventArgs;
 use Doctrine\ORM\Tools\Event\GenerateSchemaEventArgs;
-use function array_intersect_key;
 
 /**
  * The SchemaTool is a tool to create/drop/update database schemas based on
@@ -249,20 +248,14 @@ function (ClassMetadata $class) use ($idMapping) : bool {
                     }
 
                     if ( ! empty($inheritedKeyColumns)) {
-                        // Fix for bug GH-8229 (id column from parent class renamed in child class):
-                        // Use the correct name for the id columns as named in the parent class.
-                        $parentClass          = $this->em->getClassMetadata($class->rootEntityName);
-                        $parentKeyColumnNames = $parentClass->getIdentifierColumnNames();
-                        $parentKeyColumns     = array_intersect_key($parentKeyColumnNames, $inheritedKeyColumns);
-
                         // Add a FK constraint on the ID column
                         $table->addForeignKeyConstraint(
                             $this->quoteStrategy->getTableName(
-                                $parentClass,
+                                $this->em->getClassMetadata($class->rootEntityName),
                                 $this->platform
                             ),
                             $inheritedKeyColumns,
-                            $parentKeyColumns,
+                            $inheritedKeyColumns,
                             ['onDelete' => 'CASCADE']
                         );
                     }

tests/Doctrine/Tests/Models/DDC3579/DDC3579User.php

@@ -81,6 +81,8 @@ public function getGroups()
 
     public static function loadMetadata($metadata)
     {
+        $metadata->isMappedSuperclass = true;
+
         $metadata->mapField(
             [
            'id'         => true,