-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370
base: 4.3.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,13 +11,28 @@ | |
/** @internal */ | ||
final class ConnectionCharsetMetadataProvider implements CharsetMetadataProvider | ||
{ | ||
public function __construct(private readonly Connection $connection) | ||
public function __construct(private readonly Connection $connection, private bool $useUtf8mb3) | ||
{ | ||
} | ||
|
||
public function normalizeCharset(string $charset): string | ||
{ | ||
if ($this->useUtf8mb3 && $charset === 'utf8') { | ||
return 'utf8mb3'; | ||
} | ||
|
||
if (! $this->useUtf8mb3 && $charset === 'utf8mb3') { | ||
return 'utf8'; | ||
} | ||
|
||
return $charset; | ||
} | ||
Comment on lines
+18
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The more I think about it: This normalization logic belongs into the platform classes. And if you put it there, you don't need that detection logic of yours and you don't that |
||
|
||
/** @throws Exception */ | ||
public function getDefaultCharsetCollation(string $charset): ?string | ||
{ | ||
$charset = $this->normalizeCharset($charset); | ||
|
||
$collation = $this->connection->fetchOne( | ||
<<<'SQL' | ||
SELECT DEFAULT_COLLATE_NAME | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,16 +8,34 @@ | |||||
use Doctrine\DBAL\Exception; | ||||||
use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider; | ||||||
|
||||||
use function str_starts_with; | ||||||
use function substr; | ||||||
|
||||||
/** @internal */ | ||||||
final class ConnectionCollationMetadataProvider implements CollationMetadataProvider | ||||||
{ | ||||||
public function __construct(private readonly Connection $connection) | ||||||
public function __construct(private readonly Connection $connection, private bool $useUtf8mb3) | ||||||
{ | ||||||
} | ||||||
|
||||||
public function normalizeCollation(string $collation): string | ||||||
{ | ||||||
if ($this->useUtf8mb3 && str_starts_with($collation, 'utf8_')) { | ||||||
return 'utf8mb3' . substr($collation, 4); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd make things more obvious here:
Suggested change
|
||||||
} | ||||||
|
||||||
if (! $this->useUtf8mb3 && str_starts_with($collation, 'utf8mb3_')) { | ||||||
return 'utf8' . substr($collation, 7); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
return $collation; | ||||||
} | ||||||
|
||||||
/** @throws Exception */ | ||||||
public function getCollationCharset(string $collation): ?string | ||||||
{ | ||||||
$collation = $this->normalizeCollation($collation); | ||||||
|
||||||
$charset = $this->connection->fetchOne( | ||||||
<<<'SQL' | ||||||
SELECT CHARACTER_SET_NAME | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\DBAL\Tests\Functional\Driver; | ||
|
||
use Doctrine\DBAL\Schema\Column; | ||
use Doctrine\DBAL\Schema\Schema; | ||
use Doctrine\DBAL\Schema\Table; | ||
use Doctrine\DBAL\Tests\FunctionalTestCase; | ||
use Doctrine\DBAL\Tests\TestUtil; | ||
use Doctrine\DBAL\Types\StringType; | ||
use PHPUnit\Framework\Attributes\DataProvider; | ||
|
||
class DBAL6146Test extends FunctionalTestCase | ||
{ | ||
protected function setUp(): void | ||
{ | ||
parent::setUp(); | ||
|
||
if (TestUtil::isDriverOneOf('pdo_mysql', 'mysqli')) { | ||
return; | ||
} | ||
|
||
self::markTestSkipped('This test requires the pdo_mysql or the mysqli driver.'); | ||
} | ||
|
||
/** @return iterable<array{array<string,string>,array<string,string>}> */ | ||
public static function equivalentCharsetAndCollationProvider(): iterable | ||
{ | ||
yield [[], []]; | ||
yield [['charset' => 'utf8'], ['charset' => 'utf8']]; | ||
yield [['charset' => 'utf8'], ['charset' => 'utf8mb3']]; | ||
yield [['charset' => 'utf8mb3'], ['charset' => 'utf8']]; | ||
yield [['charset' => 'utf8mb3'], ['charset' => 'utf8mb3']]; | ||
yield [['collation' => 'utf8_unicode_ci'], ['collation' => 'utf8_unicode_ci']]; | ||
yield [['collation' => 'utf8_unicode_ci'], ['collation' => 'utf8mb3_unicode_ci']]; | ||
yield [['collation' => 'utf8mb3_unicode_ci'], ['collation' => 'utf8mb3_unicode_ci']]; | ||
yield [['collation' => 'utf8mb3_unicode_ci'], ['collation' => 'utf8_unicode_ci']]; | ||
yield [ | ||
['charset' => 'utf8', 'collation' => 'utf8_unicode_ci'], | ||
['charset' => 'utf8', 'collation' => 'utf8_unicode_ci'], | ||
]; | ||
|
||
yield [ | ||
['charset' => 'utf8', 'collation' => 'utf8_unicode_ci'], | ||
['charset' => 'utf8mb3', 'collation' => 'utf8mb3_unicode_ci'], | ||
]; | ||
|
||
yield [ | ||
['charset' => 'utf8mb3', 'collation' => 'utf8mb3_unicode_ci'], | ||
['charset' => 'utf8', 'collation' => 'utf8_unicode_ci'], | ||
]; | ||
|
||
yield [ | ||
['charset' => 'utf8mb3', 'collation' => 'utf8mb3_unicode_ci'], | ||
['charset' => 'utf8mb3', 'collation' => 'utf8mb3_unicode_ci'], | ||
]; | ||
} | ||
|
||
/** | ||
* @param array<string,string> $options1 | ||
* @param array<string,string> $options2 | ||
*/ | ||
#[DataProvider('equivalentCharsetAndCollationProvider')] | ||
public function testThereAreNoRedundantAlterTableStatements(array $options1, array $options2): void | ||
{ | ||
$column1 = new Column('bar', new StringType(), ['length' => 32, 'platformOptions' => $options1]); | ||
$table1 = new Table(name: 'foo6146', columns: [$column1]); | ||
|
||
$column2 = new Column('bar', new StringType(), ['length' => 32, 'platformOptions' => $options2]); | ||
$table2 = new Table(name: 'foo6146', columns: [$column2]); | ||
|
||
$this->dropAndCreateTable($table1); | ||
|
||
$schemaManager = $this->connection->createSchemaManager(); | ||
$oldSchema = $schemaManager->introspectSchema(); | ||
$newSchema = new Schema([$table2]); | ||
$comparator = $schemaManager->createComparator(); | ||
$schemaDiff = $comparator->compareSchemas($oldSchema, $newSchema); | ||
$alteredTables = $schemaDiff->getAlteredTables(); | ||
|
||
self::assertEmpty($alteredTables); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\DBAL\Tests\Platforms\MySQL\CollationMetadataProvider; | ||
|
||
use Doctrine\DBAL\Connection; | ||
use Doctrine\DBAL\Platforms\MySQL\CharsetMetadataProvider\ConnectionCharsetMetadataProvider; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class ConnectionCharsetMetadataProviderTest extends TestCase | ||
{ | ||
public function testNormalizeCharset(): void | ||
{ | ||
$connection = $this->createMock(Connection::class); | ||
|
||
$utf8Provider = new ConnectionCharsetMetadataProvider($connection, false); | ||
|
||
self::assertSame('utf8', $utf8Provider->normalizeCharset('utf8')); | ||
self::assertSame('utf8', $utf8Provider->normalizeCharset('utf8mb3')); | ||
self::assertSame('foobar', $utf8Provider->normalizeCharset('foobar')); | ||
|
||
$utf8mb3Provider = new ConnectionCharsetMetadataProvider($connection, true); | ||
|
||
self::assertSame('utf8mb3', $utf8mb3Provider->normalizeCharset('utf8')); | ||
self::assertSame('utf8mb3', $utf8mb3Provider->normalizeCharset('utf8mb3')); | ||
self::assertSame('foobar', $utf8mb3Provider->normalizeCharset('foobar')); | ||
Comment on lines
+23
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a separate test. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Doctrine\DBAL\Tests\Platforms\MySQL\CollationMetadataProvider; | ||
|
||
use Doctrine\DBAL\Connection; | ||
use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider\ConnectionCollationMetadataProvider; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class ConnectionCollationMetadataProviderTest extends TestCase | ||
{ | ||
public function testNormalizeCcollation(): void | ||
{ | ||
$connection = $this->createMock(Connection::class); | ||
|
||
$utf8Provider = new ConnectionCollationMetadataProvider($connection, false); | ||
|
||
self::assertSame('utf8_unicode_ci', $utf8Provider->normalizeCollation('utf8_unicode_ci')); | ||
self::assertSame('utf8_unicode_ci', $utf8Provider->normalizeCollation('utf8mb3_unicode_ci')); | ||
self::assertSame('foobar_unicode_ci', $utf8Provider->normalizeCollation('foobar_unicode_ci')); | ||
|
||
$utf8mb3Provider = new ConnectionCollationMetadataProvider($connection, true); | ||
|
||
self::assertSame('utf8mb3_unicode_ci', $utf8mb3Provider->normalizeCollation('utf8_unicode_ci')); | ||
self::assertSame('utf8mb3_unicode_ci', $utf8mb3Provider->normalizeCollation('utf8mb3_unicode_ci')); | ||
self::assertSame('foobar_unicode_ci', $utf8mb3Provider->normalizeCollation('foobar_unicode_ci')); | ||
Comment on lines
+23
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a separate test. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having my issues with this method.
First of all, it is highly unusual that our platform classes interact with the connection, in this case send a query to the server. Our platform classes are meant to collect and normalize differences between database systems and versions. Since you don't ever override this method, there's no reason why this method should exist on a platform class.
Moreover, you wrote that…
If this behavior really depends on the database version and taking into account that we know the database version: Why do we need to perform feature detection by running a query on the server?