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

Address deprecations from doctrine/dbal #11809

Draft
wants to merge 7 commits into
base: 3.3.x
Choose a base branch
from
Draft

Address deprecations from doctrine/dbal #11809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
939394d
Address DBAL deprecations
greg0ire Jan 25, 2025
2cdebdd
Address removeForeignKey() deprecation
greg0ire Jan 25, 2025
de2eb07
Address quoteIdentifier() deprecations in tests
greg0ire Jan 25, 2025
da457c8
to be reverted
greg0ire Jan 25, 2025
f8fc8ec
Address deprecation of getQuotedName()
greg0ire Jan 25, 2025
a6dacb1
Move to quoteSingleIdentifier() in src
greg0ire Jan 26, 2025
593210a
Add and use compatibility layer
greg0ire Jan 26, 2025
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
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"composer-runtime-api": "^2",
"ext-ctype": "*",
"doctrine/collections": "^2.2",
"doctrine/dbal": "^3.8.2 || ^4",
"doctrine/dbal": "4.3.x-dev",
"doctrine/deprecations": "^0.5.3 || ^1",
"doctrine/event-manager": "^1.2 || ^2",
"doctrine/inflector": "^1.4 || ^2.0",
Expand Down
59 changes: 59 additions & 0 deletions src/DbalCompatibility/ForeignKey.php
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

@morozov am I on the right track with this? It seems a bit complicated.

Copy link
Member

Choose a reason for hiding this comment

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

As I understand, you want to create wrapper classes that will expose some unified interface for DBAL 4 and 5. That looks about right.

Copy link
Member

Choose a reason for hiding this comment

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

As a second thought, is it really necessary for ORM to support DBAL 4.2.x and 5.x? If it bumps DBAL requirement to 4.3.x, then it can switch to the new API without the need to implement the wrapers.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue isn't that 4.2.x needs to be supported (otherwise indeed I would bump), the issue is that 3.x needs to be supported 😢

Copy link
Member

@morozov morozov Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think there's something fundamentally wrong with this requirement.

DBAL deprecates its old APIs and provides a forward compatibility layer exactly in order to help its consumers migrate from one major release to the other (two in total) w/o building any compatibility layers themselves. ORM, on the other hand, aims at supporting 3 major DBAL releases (3 through 5) and thus has to maintain its own compatibility layer.

Maybe instead of building a compatibility layer, this effort could be spent on dropping support for DBAL 3?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see that @derrabus has done this: #11216

@derrabus should it be backported or what?

Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php

declare(strict_types=1);

namespace Doctrine\ORM\DbalCompatibility;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Schema\ForeignKeyConstraint;

use function method_exists;

/** @internal */
final readonly class ForeignKey
{
public function __construct(private ForeignKeyConstraint $foreignKey)
{
}

/** @return array<string> */
public function getReferencingColumns(AbstractPlatform $platform): array
{
if (! method_exists($this->foreignKey, 'getReferencingColumns')) {

Check failure on line 22 in src/DbalCompatibility/ForeignKey.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Call to function method_exists() with Doctrine\DBAL\Schema\ForeignKeyConstraint and 'getReferencingColum…' will always evaluate to false.
return $this->foreignKey->getLocalColumns();

Check failure on line 23 in src/DbalCompatibility/ForeignKey.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Call to deprecated method getLocalColumns() of class Doctrine\DBAL\Schema\ForeignKeyConstraint: Use {@see getReferencingColumnNames()} instead.
}

$namesAsStrings = [];

foreach ($this->foreignKey->getReferencingColumnNames() as $name) {

Check failure on line 28 in src/DbalCompatibility/ForeignKey.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (3.8.2, phpstan-dbal3.neon) 

Call to an undefined method Doctrine\DBAL\Schema\ForeignKeyConstraint::getReferencingColumnNames().
$namesAsStrings[] = $name->toSQL($platform);
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I maybe be using toString here? I fear that the platform won't be always available in places where I should use these methods, for instance here:

orm/src/Mapping/Driver/DatabaseDriver.php

Line 210 in cf39e00

$localColumn = current($myFk->getLocalColumns());

Copy link
Member

Choose a reason for hiding this comment

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

If you want to represent names as strings for compatibility with DBAL 4, then toString() is fine. The contract is: toSQL(AbstractPlatform) will render the name so that it can be parsed by the platform in question; toString() will render the name so that it can be parsed by the corresponding name parser (i.e. converted back to object).

}

return $namesAsStrings;
}

/** @return array<string> */
public function getReferencedColumns(AbstractPlatform $platform): array
{
if (! method_exists($this->foreignKey, 'getReferencedColumns')) {

Check failure on line 38 in src/DbalCompatibility/ForeignKey.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Call to function method_exists() with Doctrine\DBAL\Schema\ForeignKeyConstraint and 'getReferencedColumns' will always evaluate to false.
return $this->foreignKey->getForeignColumns();

Check failure on line 39 in src/DbalCompatibility/ForeignKey.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Call to deprecated method getForeignColumns() of class Doctrine\DBAL\Schema\ForeignKeyConstraint: Use {@see getReferencedColumnNames()} instead.  Returns the names of the referenced table columns the foreign key constraint is associated with.
}

$namesAsStrings = [];

foreach ($this->foreignKey->getReferencedColumnNames() as $name) {

Check failure on line 44 in src/DbalCompatibility/ForeignKey.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (3.8.2, phpstan-dbal3.neon) 

Call to an undefined method Doctrine\DBAL\Schema\ForeignKeyConstraint::getReferencedColumnNames().
$namesAsStrings[] = $name->toSQL($platform);
}

return $namesAsStrings;
}

public function getReferencedTableName(AbstractPlatform $platform): string
{
if (! method_exists($this->foreignKey, 'getReferencedTableName')) {

Check failure on line 53 in src/DbalCompatibility/ForeignKey.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Call to function method_exists() with Doctrine\DBAL\Schema\ForeignKeyConstraint and 'getReferencedTableN…' will always evaluate to true.
return $this->foreignKey->getForeignTableName();

Check failure on line 54 in src/DbalCompatibility/ForeignKey.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Call to deprecated method getForeignTableName() of class Doctrine\DBAL\Schema\ForeignKeyConstraint: Use {@see getReferencedTableName()} instead.  Returns the name of the referenced table the foreign key constraint is associated with.
}

return $this->foreignKey->getReferencedTableName()->toSQL($platform);
}
}
27 changes: 27 additions & 0 deletions src/DbalCompatibility/Table.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

declare(strict_types=1);

namespace Doctrine\ORM\DbalCompatibility;

use Doctrine\DBAL\Schema\ForeignKeyConstraint;
use Doctrine\DBAL\Schema\Table as DoctrineTable;

use function array_map;

/** @internal */
final readonly class Table
{
public function __construct(private DoctrineTable $table)
{
}

/** @return array<string, ForeignKey> */
public function getForeignKeys(): array
{
return array_map(
static fn (ForeignKeyConstraint $foreignKey) => new ForeignKey($foreignKey),
$this->table->getForeignKeys(),
);
}
}
14 changes: 7 additions & 7 deletions src/Mapping/DefaultQuoteStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class DefaultQuoteStrategy implements QuoteStrategy
public function getColumnName(string $fieldName, ClassMetadata $class, AbstractPlatform $platform): string
{
return isset($class->fieldMappings[$fieldName]->quoted)
? $platform->quoteIdentifier($class->fieldMappings[$fieldName]->columnName)
? $platform->quoteSingleIdentifier($class->fieldMappings[$fieldName]->columnName)
: $class->fieldMappings[$fieldName]->columnName;
}

Expand All @@ -42,7 +42,7 @@ public function getTableName(ClassMetadata $class, AbstractPlatform $platform):
}

return isset($class->table['quoted'])
? $platform->quoteIdentifier($tableName)
? $platform->quoteSingleIdentifier($tableName)
: $tableName;
}

Expand All @@ -52,14 +52,14 @@ public function getTableName(ClassMetadata $class, AbstractPlatform $platform):
public function getSequenceName(array $definition, ClassMetadata $class, AbstractPlatform $platform): string
{
return isset($definition['quoted'])
? $platform->quoteIdentifier($definition['sequenceName'])
? $platform->quoteSingleIdentifier($definition['sequenceName'])
: $definition['sequenceName'];
}

public function getJoinColumnName(JoinColumnMapping $joinColumn, ClassMetadata $class, AbstractPlatform $platform): string
{
return isset($joinColumn->quoted)
? $platform->quoteIdentifier($joinColumn->name)
? $platform->quoteSingleIdentifier($joinColumn->name)
: $joinColumn->name;
}

Expand All @@ -69,7 +69,7 @@ public function getReferencedJoinColumnName(
AbstractPlatform $platform,
): string {
return isset($joinColumn->quoted)
? $platform->quoteIdentifier($joinColumn->referencedColumnName)
? $platform->quoteSingleIdentifier($joinColumn->referencedColumnName)
: $joinColumn->referencedColumnName;
}

Expand All @@ -87,7 +87,7 @@ public function getJoinTableName(
$tableName = $association->joinTable->name;

if (isset($association->joinTable->quoted)) {
$tableName = $platform->quoteIdentifier($tableName);
$tableName = $platform->quoteSingleIdentifier($tableName);
}

return $schema . $tableName;
Expand All @@ -113,7 +113,7 @@ public function getIdentifierColumnNames(ClassMetadata $class, AbstractPlatform
$joinColumns = $assoc->joinColumns;
$assocQuotedColumnNames = array_map(
static fn (JoinColumnMapping $joinColumn) => isset($joinColumn->quoted)
? $platform->quoteIdentifier($joinColumn->name)
? $platform->quoteSingleIdentifier($joinColumn->name)
: $joinColumn->name,
$joinColumns,
);
Expand Down
15 changes: 10 additions & 5 deletions src/Tools/SchemaTool.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use Doctrine\DBAL\Schema\Index;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\DBAL\Schema\Table;
use Doctrine\ORM\DbalCompatibility\Table as CompatibilityTable;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\Mapping\AssociationMapping;
use Doctrine\ORM\Mapping\ClassMetadata;
Expand All @@ -37,6 +38,7 @@
use function implode;
use function in_array;
use function is_numeric;
use function method_exists;
use function strtolower;

/**
Expand Down Expand Up @@ -723,13 +725,16 @@
&& ($foreignTableName !== $addedFks[$compositeName]['foreignTableName']
|| 0 < count(array_diff($foreignColumns, $addedFks[$compositeName]['foreignColumns'])))
) {
foreach ($theJoinTable->getForeignKeys() as $fkName => $key) {
$theCompatibilityTable = new CompatibilityTable($theJoinTable);
foreach ($theCompatibilityTable->getForeignKeys() as $fkName => $key) {
if (
count(array_diff($key->getLocalColumns(), $localColumns)) === 0
&& (($key->getForeignTableName() !== $foreignTableName)
|| 0 < count(array_diff($key->getForeignColumns(), $foreignColumns)))
count(array_diff($key->getReferencingColumns($this->platform), $localColumns)) === 0
&& (($key->getReferencedTableName($this->platform) !== $foreignTableName)
|| 0 < count(array_diff($key->getReferencedColumns($this->platform), $foreignColumns)))
) {
$theJoinTable->removeForeignKey($fkName);
method_exists($theJoinTable, 'dropForeignKey')

Check failure on line 735 in src/Tools/SchemaTool.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Call to function method_exists() with Doctrine\DBAL\Schema\Table and 'dropForeignKey' will always evaluate to true.
? $theJoinTable->dropForeignKey($fkName)
: $theJoinTable->removeForeignKey($fkName);

Check failure on line 737 in src/Tools/SchemaTool.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Call to deprecated method removeForeignKey() of class Doctrine\DBAL\Schema\Table: Use {@link dropForeignKey()} instead.
break;
}
}
Expand All @@ -739,8 +744,8 @@
$addedFks[$compositeName] = ['foreignTableName' => $foreignTableName, 'foreignColumns' => $foreignColumns];
$theJoinTable->addForeignKeyConstraint(
$foreignTableName,
$localColumns,

Check failure on line 747 in src/Tools/SchemaTool.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Parameter #2 $localColumnNames of method Doctrine\DBAL\Schema\Table::addForeignKeyConstraint() expects non-empty-list<string>, list<string> given.
$foreignColumns,

Check failure on line 748 in src/Tools/SchemaTool.php

View workflow job for this annotation

GitHub Actions / Static Analysis with PHPStan (default, phpstan.neon) 

Parameter #3 $foreignColumnNames of method Doctrine\DBAL\Schema\Table::addForeignKeyConstraint() expects non-empty-list<string>, list<string> given.
$fkOptions,
);
}
Expand Down
18 changes: 16 additions & 2 deletions tests/Tests/ORM/Functional/Ticket/DDC2138Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

use function assert;
use function reset;
use function sprintf;

class DDC2138Test extends OrmFunctionalTestCase
{
Expand All @@ -42,8 +43,8 @@ public function testForeignKeyOnSTIWithMultipleMapping(): void

$table = $schema->getTable('users_followed_objects');
assert($table instanceof DbalTable);
self::assertTrue($table->columnsAreIndexed(['object_id']));
self::assertTrue($table->columnsAreIndexed(['user_id']));
self::assertColumnIsIndexed($table, 'object_id');
self::assertColumnIsIndexed($table, 'user_id');
$foreignKeys = $table->getForeignKeys();
self::assertCount(1, $foreignKeys, 'user_id column has to have FK, but not object_id');

Expand All @@ -55,6 +56,19 @@ public function testForeignKeyOnSTIWithMultipleMapping(): void
self::assertContains('user_id', $localColumns);
self::assertCount(1, $localColumns);
}

private static function assertColumnIsIndexed(DbalTable $table, string $columnName): void
{
$columnsIsIndexed = false;
foreach ($table->getIndexes() as $index) {
if ($index->spansColumns([$columnName])) {
$columnsIsIndexed = true;
break;
}
}

self::assertTrue($columnsIsIndexed, sprintf('Column %s should be indexed.', $columnName));
}
}


Expand Down
10 changes: 5 additions & 5 deletions tests/Tests/ORM/Functional/Ticket/DDC832Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ public function tearDown(): void
$platform = $this->_em->getConnection()->getDatabasePlatform();

$sm = $this->createSchemaManager();
$sm->dropTable($platform->quoteIdentifier('TREE_INDEX'));
$sm->dropTable($platform->quoteIdentifier('INDEX'));
$sm->dropTable($platform->quoteIdentifier('LIKE'));
$sm->dropTable($platform->quoteSingleIdentifier('TREE_INDEX'));
$sm->dropTable($platform->quoteSingleIdentifier('INDEX'));
$sm->dropTable($platform->quoteSingleIdentifier('LIKE'));

// DBAL 3
if ($platform instanceof PostgreSQLPlatform && method_exists($platform, 'getIdentitySequenceName')) {
$sm->dropSequence($platform->quoteIdentifier('INDEX_id_seq'));
$sm->dropSequence($platform->quoteIdentifier('LIKE_id_seq'));
$sm->dropSequence($platform->quoteSingleIdentifier('INDEX_id_seq'));
$sm->dropSequence($platform->quoteSingleIdentifier('LIKE_id_seq'));
}
}

Expand Down
27 changes: 20 additions & 7 deletions tests/Tests/ORM/Tools/SchemaToolTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace Doctrine\Tests\ORM\Tools;

use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\DbalCompatibility\ForeignKey;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\Entity;
Expand Down Expand Up @@ -65,7 +66,16 @@ public function testAddUniqueIndexForUniqueFieldAttribute(): void
$schema = $schemaTool->getSchemaFromMetadata($classes);

self::assertTrue($schema->hasTable('cms_users'), 'Table cms_users should exist.');
self::assertTrue($schema->getTable('cms_users')->columnsAreIndexed(['username']), 'username column should be indexed.');

$usernameIsIndexed = false;
foreach ($schema->getTable('cms_users')->getIndexes() as $index) {
if ($index->spansColumns(['username'])) {
$usernameIsIndexed = true;
break;
}
}

self::assertTrue($usernameIsIndexed, 'username column should be indexed.');
}

public function testAttributeOptionsArgument(): void
Expand Down Expand Up @@ -285,17 +295,20 @@ public function testDerivedCompositeKey(): void
self::assertCount(2, $childTableForeignKeys);

$expectedColumns = [
'joined_derived_identity' => [['keyPart1_id'], ['id']],
'joined_derived_root' => [['keyPart1_id', 'keyPart2'], ['keyPart1_id', 'keyPart2']],
'"JOINED_DERIVED_IDENTITY"' => [['keyPart1_id'], ['id']],
'"JOINED_DERIVED_ROOT"' => [['keyPart1_id', 'keyPart2'], ['keyPart1_id', 'keyPart2']],
];

$platform = $em->getConnection()->getDatabasePlatform();

foreach ($childTableForeignKeys as $foreignKey) {
self::assertArrayHasKey($foreignKey->getForeignTableName(), $expectedColumns);
$compatForeignKey = new ForeignKey($foreignKey);
self::assertArrayHasKey($compatForeignKey->getReferencedTableName($platform), $expectedColumns);

[$localColumns, $foreignColumns] = $expectedColumns[$foreignKey->getForeignTableName()];
[$localColumns, $foreignColumns] = $expectedColumns[$compatForeignKey->getReferencedTableName($platform)];

self::assertSame($localColumns, $foreignKey->getLocalColumns());
self::assertSame($foreignColumns, $foreignKey->getForeignColumns());
self::assertSame($localColumns, $compatForeignKey->getReferencingColumns($platform));
self::assertSame($foreignColumns, $compatForeignKey->getReferencedColumns($platform));
}
}

Expand Down
22 changes: 12 additions & 10 deletions tests/Tests/OrmFunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,7 @@ protected function tearDown(): void
}

if (isset($this->_usedModelSets['directorytree'])) {
$conn->executeStatement('DELETE FROM ' . $platform->quoteIdentifier('file'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteSingleIdentifier('file'));
// MySQL doesn't know deferred deletions therefore only executing the second query gives errors.
$conn->executeStatement('DELETE FROM Directory WHERE parentDirectory_id IS NOT NULL');
$conn->executeStatement('DELETE FROM Directory');
Expand Down Expand Up @@ -707,17 +707,17 @@ protected function tearDown(): void
$conn->executeStatement(
sprintf(
'UPDATE %s SET %s = NULL',
$platform->quoteIdentifier('quote-address'),
$platform->quoteIdentifier('user-id'),
$platform->quoteSingleIdentifier('quote-address'),
$platform->quoteSingleIdentifier('user-id'),
),
);

$conn->executeStatement('DELETE FROM ' . $platform->quoteIdentifier('quote-users-groups'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteIdentifier('quote-group'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteIdentifier('quote-phone'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteIdentifier('quote-user'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteIdentifier('quote-address'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteIdentifier('quote-city'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteSingleIdentifier('quote-users-groups'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteSingleIdentifier('quote-group'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteSingleIdentifier('quote-phone'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteSingleIdentifier('quote-user'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteSingleIdentifier('quote-address'));
$conn->executeStatement('DELETE FROM ' . $platform->quoteSingleIdentifier('quote-city'));
}

if (isset($this->_usedModelSets['vct_onetoone'])) {
Expand Down Expand Up @@ -1108,7 +1108,9 @@ protected function dropAndCreateTable(Table $table): void
{
$schemaManager = $this->createSchemaManager();
$platform = $this->_em->getConnection()->getDatabasePlatform();
$tableName = $table->getQuotedName($platform);
$tableName = method_exists($table, 'getObjectName') ?
$table->getObjectName($platform)->toSQL($platform)
: $table->getQuotedName($platform);

$this->dropTableIfExists($tableName);
$schemaManager->createTable($table);
Expand Down
Loading