Skip to content

Commit

Permalink
Add deprecations for current parser behavior
Browse files Browse the repository at this point in the history
  • Loading branch information
morozov committed Nov 12, 2024
1 parent 98617a6 commit 358c6ea
Show file tree
Hide file tree
Showing 3 changed files with 241 additions and 4 deletions.
22 changes: 22 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,28 @@ awareness about deprecated code.

# Upgrade to 4.3

## Deprecated relying on the current implementation of the database object name parser

The current object name parser implicitly quotes identifiers in the following cases:

1. If the object name is a reserved keyword (e.g., `select`).
2. If an unquoted identifier is preceded by a quoted identifier (e.g., `"inventory".product`).

As a result, the original case of such identifiers is preserved on platforms that respect the SQL-92 standard (i.e.,
identifiers are not upper-cased on Oracle and IBM DB2, and not lower-cased on PostgreSQL). This behavior is deprecated.

If preserving the original case of an identifier is required, please explicitly quote it (e.g., `select``"select"`).

Additionally, the current parser exhibits the following defects:

1. It ignores a missing closing quote in a quoted identifier (e.g., `"inventory`).
2. It allows names with more than two identifiers (e.g., `warehouse.inventory.product`) but only uses the first two,
ignoring the remaining ones.
3. If a quoted identifier contains a dot, it incorrectly treats the part before the dot as a qualifier, despite the
identifier being quoted.

Relying on the above behaviors is deprecated.

## Deprecated `AbstractPlatform::quoteIdentifier()` and `Connection::quoteIdentifier()`

The `AbstractPlatform::quoteIdentifier()` and `Connection::quoteIdentifier()` methods have been deprecated.
Expand Down
130 changes: 126 additions & 4 deletions src/Schema/AbstractAsset.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,17 @@
namespace Doctrine\DBAL\Schema;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Schema\Name\Parser;
use Doctrine\DBAL\Schema\Name\Parser\Identifier;
use Doctrine\Deprecations\Deprecation;

use function array_map;
use function count;
use function crc32;
use function dechex;
use function explode;
use function implode;
use function sprintf;
use function str_contains;
use function str_replace;
use function strtolower;
Expand All @@ -35,11 +40,18 @@ abstract class AbstractAsset

protected bool $_quoted = false;

/** @var list<Identifier> */
private array $identifiers = [];

private bool $validateFuture = false;

/**
* Sets the name of this asset.
*/
protected function _setName(string $name): void
{
$input = $name;

if ($this->isIdentifierQuoted($name)) {
$this->_quoted = true;
$name = $this->trimQuotes($name);
Expand All @@ -52,6 +64,81 @@ protected function _setName(string $name): void
}

$this->_name = $name;

$this->validateFuture = false;

if ($input !== '') {
$parser = new Parser();

try {
$identifiers = $parser->parse($input);
} catch (Parser\Exception $e) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/6592',
'Unable to parse object name: %s.',
$e->getMessage(),
);

return;
}
} else {
$identifiers = [];
}

switch (count($identifiers)) {
case 0:
$this->identifiers = [];

return;
case 1:
$namespace = null;
$name = $identifiers[0];
break;

case 2:
/** @psalm-suppress PossiblyUndefinedArrayOffset */
[$namespace, $name] = $identifiers;
break;

default:
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/6592',
'An object name may consist of at most 2 identifiers (<namespace>.<name>), %d given.',
count($identifiers),
);

return;
}

$this->identifiers = $identifiers;
$this->validateFuture = true;

$futureName = $name->getValue();
$futureNamespace = $namespace?->getValue();

if ($this->_name !== $futureName) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/6592',
'Instead of "%s", this name will be interpreted as "%s" in 5.0',
$this->_name,
$futureName,
);
}

if ($this->_namespace === $futureNamespace) {
return;
}

Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/6592',
'Instead of %s, the namespace in this name will be interpreted as %s in 5.0.',
$this->_namespace !== null ? sprintf('"%s"', $this->_namespace) : 'null',
$futureNamespace !== null ? sprintf('"%s"', $futureNamespace) : 'null',
);
}

/**
Expand Down Expand Up @@ -129,12 +216,47 @@ public function getName(): string
public function getQuotedName(AbstractPlatform $platform): string
{
$keywords = $platform->getReservedKeywordsList();
$parts = explode('.', $this->getName());
foreach ($parts as $k => $v) {
$parts[$k] = $this->_quoted || $keywords->isKeyword($v) ? $platform->quoteSingleIdentifier($v) : $v;
$parts = $normalizedParts = [];

foreach (explode('.', $this->getName()) as $identifier) {
$isQuoted = $this->_quoted || $keywords->isKeyword($identifier);

if (! $isQuoted) {
$parts[] = $identifier;
$normalizedParts[] = $platform->normalizeUnquotedIdentifier($identifier);
} else {
$parts[] = $platform->quoteSingleIdentifier($identifier);
$normalizedParts[] = $identifier;
}
}

$name = implode('.', $parts);

if ($this->validateFuture) {
$futureParts = array_map(static function (Identifier $identifier) use ($platform): string {
$value = $identifier->getValue();

if (! $identifier->isQuoted()) {
$value = $platform->normalizeUnquotedIdentifier($value);
}

return $value;
}, $this->identifiers);

if ($normalizedParts !== $futureParts) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/6592',
'Relying on implicitly quoted identifiers preserving their original case is deprecated. '
. 'The current name %s will become %s in 5.0. '
. 'Please quote the name if the case needs to be preserved.',
$name,
implode('.', array_map([$platform, 'quoteSingleIdentifier'], $futureParts)),
);
}
}

return implode('.', $parts);
return $name;
}

/**
Expand Down
93 changes: 93 additions & 0 deletions tests/Schema/AbstractAssetTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Tests\Schema;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\MySQLPlatform;
use Doctrine\DBAL\Platforms\OraclePlatform;
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
use Doctrine\DBAL\Schema\Identifier;
use Doctrine\Deprecations\PHPUnit\VerifyDeprecations;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;

class AbstractAssetTest extends TestCase
{
use VerifyDeprecations;

#[DataProvider('nameParsingDeprecationProvider')]
public function testNameParsingDeprecation(string $name, AbstractPlatform $platform): void
{
$this->expectDeprecationWithIdentifier('https://github.com/doctrine/dbal/pull/6592');

$identifier = new Identifier($name);
$identifier->getQuotedName($platform);
}

/** @return iterable<array{string, AbstractPlatform}> */
public static function nameParsingDeprecationProvider(): iterable
{
return [
// unquoted keywords not in normal case
['select', new OraclePlatform()],
['SELECT', new PostgreSQLPlatform()],

// unquoted name not in normal case qualified by quoted name
['"_".id', new OraclePlatform()],
['"_".ID', new PostgreSQLPlatform()],

// name with more than one qualifier
['i.am.overqualified', new MySQLPlatform()],

// parse error
['table.', new MySQLPlatform()],
['"table', new MySQLPlatform()],
['table"', new MySQLPlatform()],
[' ', new MySQLPlatform()],

// incompatible parser behavior
['"example.com"', new MySQLPlatform()],
];
}

#[DataProvider('noNameParsingDeprecationProvider')]
public function testNoNameParsingDeprecation(string $name, AbstractPlatform $platform): void
{
$identifier = new Identifier($name);

$this->expectNoDeprecationWithIdentifier('https://github.com/doctrine/dbal/pull/XXXX');
$identifier->getQuotedName($platform);
}

/** @return iterable<array{string, AbstractPlatform}> */
public static function noNameParsingDeprecationProvider(): iterable
{
return [
// empty name
['', new MySQLPlatform()],

// name with one qualifier
['schema.table', new MySQLPlatform()],

// quoted keywords
['"select"', new OraclePlatform()],
['"SELECT"', new PostgreSQLPlatform()],

// unquoted keywords in normal case
['SELECT', new OraclePlatform()],
['select', new PostgreSQLPlatform()],

// unquoted keywords in any case on a platform that does not force a case
['SELECT', new MySQLPlatform()],
['select', new MySQLPlatform()],

// non-keywords in any case
['id', new OraclePlatform()],
['ID', new OraclePlatform()],
['id', new PostgreSQLPlatform()],
['ID', new PostgreSQLPlatform()],
];
}
}

0 comments on commit 358c6ea

Please sign in to comment.