Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GH-8229] Prevent Illegal Inheritance Override #8348

Merged
merged 10 commits into from
Nov 25, 2020
Merged

[GH-8229] Prevent Illegal Inheritance Override #8348

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
75c4ded
[GH-8229] Prevent AttributeOverride on fields from entities, only all…
beberlei Nov 24, 2020
e8ab2dd
[GH-8229] Prevent AssociationOverride on fields from entities, only a…
beberlei Nov 24, 2020
d85efb0
Revert "Fix SQL alias generation regression for simple inheritance (#…
beberlei Nov 24, 2020
52b780a
[GH-8229] Finalize checks for illegal attribute/assocation overrides.
beberlei Nov 24, 2020
d70f502
[GH-8229] Revert ccae8f717684dcd871b418444d7ca7155c677169 PR #8234
beberlei Nov 24, 2020
3431b24
[GH-8229] Update documentation to clarify only mapped superclass or t…
beberlei Nov 24, 2020
1e98b3d
[GH-8229] Fix style violations introduced by revert
beberlei Nov 24, 2020
0193517
[GH-8229] Fix style violations introduced by revert
beberlei Nov 24, 2020
81aa5a9
[GH-8229] Temporarily disable the exception until 2.8.
beberlei Nov 25, 2020
5238c88
Make phpcs happy
beberlei Nov 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions docs/en/reference/inheritance-mapping.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 13 additions & 7 deletions lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
use ReflectionClass;
use ReflectionProperty;
use RuntimeException;
use function array_key_exists;
use function count;
use function explode;

/**
Expand Down Expand Up @@ -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);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should the parts that are about throwing be contributed to 2.8 instead? If we think there are people currently using this, we should probably not have it blow up in their face in a patch

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, i should remove them for 2.7, and re-introduce them for 2.8


if (isset($overrideMapping['joinColumns'])) {
$mapping['joinColumns'] = $overrideMapping['joinColumns'];
}
Expand Down Expand Up @@ -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'];
}
Expand All @@ -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']]);
Expand Down
14 changes: 14 additions & 0 deletions lib/Doctrine/ORM/Mapping/MappingException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

namespace Doctrine\ORM\Mapping;

use function sprintf;

/**
* A MappingException indicates that something is wrong with the mapping setup.
*
Expand Down Expand Up @@ -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
)
);
}
}
44 changes: 6 additions & 38 deletions lib/Doctrine/ORM/Persisters/Entity/BasicEntityPersister.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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'];
Expand Down Expand Up @@ -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])) {
Expand Down
71 changes: 28 additions & 43 deletions lib/Doctrine/ORM/Persisters/Entity/JoinedSubclassPersister.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
38 changes: 9 additions & 29 deletions lib/Doctrine/ORM/Query/SqlWalker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;

Expand Down
11 changes: 2 additions & 9 deletions lib/Doctrine/ORM/Tools/SchemaTool.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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']
);
}
Expand Down
2 changes: 2 additions & 0 deletions tests/Doctrine/Tests/Models/DDC3579/DDC3579User.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ public function getGroups()

public static function loadMetadata($metadata)
{
$metadata->isMappedSuperclass = true;

$metadata->mapField(
[
'id' => true,
Expand Down
Loading