Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

fisharebest · 2024-04-29T22:07:09Z

Q	A
Type	bug
Fixed issues	#6146

Summary

In MySQL (and MariaDB), utf8 and utf8mb3 are aliases. Depending on the database server version, the information schema may contain one or the other. Similarly, column definitions may contain one or the other.

When performing schema-comparisons, a mismatch between utf8 and utf8mb3 causes an ALTER TABLE statement to be generated - even when columns are identical.

The code already contains a function to normalize the charset/collation options:

dbal/src/Platforms/MySQL/Comparator.php

Line 83 in 61d79c6

private function normalizeOptions(array $options): array

My solution is to extend this function so that it also replaces utf8 with utf8mb3 (or vice-versa), to match the current database connection.

To detect the prefered character set for the connection, I've added:
AbstractMySQLPlatform::informationSchemaUsesUtf8mb3()
MariaDBPlatform::informationSchemaUsesUtf8mb3()

The obvious place for the new normalization logic is the CharsetMetadataProvider and CollationMetadataProvider classes. These are interfaces, so this is technically a BC break, however, they are marked @internal.

I've added tests which should cover all the new/modified code. I've run it against MySQL. I don't have MariaDB to hand.

…tected Make internal Comparator methods protected

Remove property-based schema comparison APIs

…ySQL

…ftovers Remove leftovers of handling foreign keys on non-InnoDB engines for MySQL

…traint-name Introspect default constraint name

Remove conditional comments for MySQL older than 5.1.16

Remove support for "unique" and "check" column properties

…-schema Remove SchemaDiff::$fromSchema

Remove support for renaming tables via TableDiff

…lt-constraint-leftovers Remove leftovers of renaming default contraints

Remove TableDiff::$name and getName()

…perties Mark ColumnDiff properties as private and read-only

…handling-workaround Remove workaround for PHP bug in PostgreSQL exception handler

To reduce Algolia operations and indexes older versions get removed

Remove older versions from the docs

* 3.8.x: Add MariaDB 11.3 to the test matrix (doctrine#6342) Spell which properly Fix double quote used wrongly for literal value in SqliteSchemaManager (doctrine#6325)

* 3.8.x: Avoid calling deprecated Type::getName() (doctrine#6359) Ensure correct json default value normalization (doctrine#6358) Document how to run integration tests locally Remove unused script

fisharebest · 2024-04-30T08:20:43Z

Just pushed a change to fix the psalm issues

greg0ire · 2024-04-30T08:26:42Z

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

greg0ire · 2024-04-30T08:57:01Z

ಠ_ಠ

…changes

greg0ire · 2024-05-01T08:29:02Z

I've added tests which should cover all the new/modified code. I've run it against MySQL. I don't have MariaDB to hand.

Codecov says that some of the new code is untested, most notably the parts using version_compare. Please add functional tests for this change. As you can see, our test suite features, among other platforms MariaDB 10.5 and MariaDB 10.6, so you should be able to test that schema comparisons on either version of MariaDB indeed doesn't cause ALTER TABLE statements to be generated. Same goes for MySQL.

You can learn more about such tests at https://www.doctrine-project.org/projects/doctrine-dbal/en/stable/reference/testing.html

fisharebest · 2024-05-01T13:40:46Z

Codecov says that some of the new code is untested, most notably the parts using version_compare. Please add functional tests for this change.

Running the tests locally says the code is covered. (Ad hoc testing confirms this).

XDEBUG_MODE=coverage vendor/bin/phpunit -c ci/github/phpunit/pdo_mysql.xml --coverage-html=coverage

However, Codecov says it isn't.

greg0ire · 2024-05-01T14:29:57Z

I think I found the reason: [2024-05-01T09:48:28.648Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 429 - {'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected available in 1935 seconds.', code='throttled')}

Let me retry this.

In a recent PR, we got reports from CodeCov about several lines not being covered. After further inspection, I found an error message in the coverage file upload saying we hit a rate limit. This will make it more likely we spot such issues in the future. The PR: doctrine#6370

greg0ire · 2024-05-01T14:42:05Z

From the Codecov UI:

Coverage data is based on head 9c87c57(2 uploads) compared to base 7fb00fd(5 uploads)

… we currently send 69 uploads, so there is no way we are going to be below the upload limit. I will have to look into this separately.

greg0ire

Looks good overall, my feedback is not a blocker.

greg0ire · 2024-05-01T14:46:50Z

src/Platforms/MySQL/CollationMetadataProvider/ConnectionCollationMetadataProvider.php

+    public function normalizeCollation(string $collation): string
+    {
+        if ($this->useUtf8mb3 && str_starts_with($collation, 'utf8_')) {
+            return 'utf8mb3' . substr($collation, 4);


I'd make things more obvious here:

Suggested change

return 'utf8mb3' . substr($collation, 4);

return 'utf8mb3' . substr($collation, strlen('utf8'));

greg0ire · 2024-05-01T14:47:07Z

src/Platforms/MySQL/CollationMetadataProvider/ConnectionCollationMetadataProvider.php

+        }
+
+        if (! $this->useUtf8mb3 && str_starts_with($collation, 'utf8mb3_')) {
+            return 'utf8' . substr($collation, 7);


Suggested change

return 'utf8' . substr($collation, 7);

return 'utf8' . substr($collation, strlen('utf8mb3'));

greg0ire · 2024-05-01T14:51:26Z

Ah there is one issue though: since this is a fix, you should target 3.8.x

fisharebest · 2024-05-01T15:46:12Z

Ah there is one issue though: since this is a fix, you should target 3.8.x

I can create another PR for this branch.

greg0ire · 2024-05-01T16:35:57Z

Please don't, we usually do merges up. Instead, let me retarget your PR, and please rebase your commits on 3.8.x and force push.

The base branch was changed.

In a recent PR, we got reports from CodeCov about several lines not being covered. After further inspection, I found an error message in the coverage file upload saying we hit a rate limit. This will make it more likely we spot such issues in the future. The PR: doctrine#6370

fisharebest · 2024-05-02T15:35:37Z

Please don't, we usually do merges up. Instead, let me retarget your PR, and please rebase your commits on 3.8.x and force push.

The PR uses Connection::getServerVersion() to select the prefered alias for utf8/utf8mb3.
In 3.8.x, this method isn't available, so I'll need to find another approach.

I'm going to be offline for the next week or so, and will look at this on my return.

greg0ire · 2024-05-02T17:48:33Z

Enjoy your time off 👍

morozov added 30 commits September 3, 2022 10:24

Merge branch '3.5.x' into 4.0.x

9cee7b7

Make internal Comparator methods protected

a8f939a

Merge pull request doctrine#5652 from morozov/comparator-internal-pro…

075848c

…tected Make internal Comparator methods protected

Remove property-based schema comparison APIs

e7b326c

Merge pull request doctrine#5649 from morozov/remove-changed-properties

1f4ca72

Remove property-based schema comparison APIs

Remove leftovers of handling foreign keys on non-InnoDB engines for M…

f84cf59

…ySQL

Merge branch '3.5.x' into 4.0.x

b4e4c74

Rework functional tests for default values on SQL Server

2f562d6

Introspect default constraint name

c8d2c3d

Merge pull request doctrine#5664 from morozov/remove-non-innodb-fk-le…

06732b8

…ftovers Remove leftovers of handling foreign keys on non-InnoDB engines for MySQL

Merge pull request doctrine#5662 from morozov/sql-server-default-cons…

0e15462

…traint-name Introspect default constraint name

Merge branch '3.5.x' into 4.0.x

039b945

Remove conditional comments for MySQL older than 5.1.16

58a47c8

Remove support for "unique" and "check" column properties

d1a5672

Remove SchemaDiff::$fromSchema

50ede2d

Merge pull request doctrine#5675 from morozov/remove-mysql-5.0-leftovers

774af88

Remove conditional comments for MySQL older than 5.1.16

Merge pull request doctrine#5655 from morozov/remove-unique-check

b4c294b

Remove support for "unique" and "check" column properties

Merge pull request doctrine#5676 from morozov/remove-schema-diff-from…

4f009c1

…-schema Remove SchemaDiff::$fromSchema

Remove support for renaming tables via TableDiff

e35aef9

Merge pull request doctrine#5677 from morozov/remove-table-diff-new-name

350149d

Remove support for renaming tables via TableDiff

Remove leftovers of renaming default contraints

598feec

Merge pull request doctrine#5679 from morozov/remove-sql-server-defau…

85a631b

…lt-constraint-leftovers Remove leftovers of renaming default contraints

Merge branch '3.5.x' into 4.0.x

b24280d

Remove TableDiff::$name and getName()

6381aca

Merge pull request doctrine#5683 from morozov/remove-table-diff-name

76635d3

Remove TableDiff::$name and getName()

Mark ColumnDiff properties as private and read-only

2373c42

Merge pull request doctrine#5684 from morozov/column-diff-private-pro…

06243b7

…perties Mark ColumnDiff properties as private and read-only

Remove workaround for PHP bug in PostgreSQL exception handler

a71d512

Merge pull request doctrine#5696 from morozov/remove-pgsql-exception-…

e652aab

…handling-workaround Remove workaround for PHP bug in PostgreSQL exception handler

Merge branch '3.5.x' into 4.0.x

07f9202

SenseException and others added 5 commits March 19, 2024 20:58

Remove older versions from the docs

aec34a9

To reduce Algolia operations and indexes older versions get removed

[Documentation] Fixing markup (doctrine#6340)

e0219f8

Merge pull request doctrine#6339 from SenseException/remove-docs

d9109cd

Remove older versions from the docs

Merge branch '3.8.x' into 4.0.x

c70bae9

* 3.8.x: Add MariaDB 11.3 to the test matrix (doctrine#6342) Spell which properly Fix double quote used wrongly for literal value in SqliteSchemaManager (doctrine#6325)

Merge branch '3.8.x' into 4.0.x

7fb00fd

* 3.8.x: Avoid calling deprecated Type::getName() (doctrine#6359) Ensure correct json default value normalization (doctrine#6358) Document how to run integration tests locally Remove unused script

fisharebest force-pushed the fix-6146 branch from 793f09e to fa707e2 Compare April 30, 2024 08:33

Fix: doctrine#6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE …

83f4788

…changes

fisharebest force-pushed the fix-6146 branch from fa707e2 to 83f4788 Compare April 30, 2024 09:04

Add functional tests

9c87c57

fisharebest force-pushed the fix-6146 branch from 61de19b to 9c87c57 Compare May 1, 2024 09:39

greg0ire previously approved these changes May 1, 2024

View reviewed changes

greg0ire requested review from SenseException and derrabus May 1, 2024 14:50

greg0ire mentioned this pull request May 1, 2024

Set fail_ci_if_error flag to true #6372

Merged

greg0ire changed the base branch from 4.0.x to 3.8.x May 1, 2024 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

fisharebest commented Apr 29, 2024 •

edited

fisharebest commented Apr 30, 2024

greg0ire commented Apr 30, 2024

greg0ire commented Apr 30, 2024

greg0ire commented May 1, 2024 •

edited

fisharebest commented May 1, 2024

greg0ire commented May 1, 2024 •

edited

greg0ire commented May 1, 2024

greg0ire left a comment

greg0ire May 1, 2024

greg0ire May 1, 2024

greg0ire commented May 1, 2024

fisharebest commented May 1, 2024

greg0ire commented May 1, 2024 •

edited

fisharebest commented May 2, 2024

greg0ire commented May 2, 2024

	return 'utf8mb3' . substr($collation, 4);
	return 'utf8mb3' . substr($collation, strlen('utf8'));

	return 'utf8' . substr($collation, 7);
	return 'utf8' . substr($collation, strlen('utf8mb3'));

Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

Are you sure you want to change the base?

Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

Conversation

fisharebest commented Apr 29, 2024 • edited

Summary

fisharebest commented Apr 30, 2024

greg0ire commented Apr 30, 2024

greg0ire commented Apr 30, 2024

greg0ire commented May 1, 2024 • edited

fisharebest commented May 1, 2024

greg0ire commented May 1, 2024 • edited

greg0ire commented May 1, 2024

greg0ire left a comment

Choose a reason for hiding this comment

greg0ire May 1, 2024

Choose a reason for hiding this comment

greg0ire May 1, 2024

Choose a reason for hiding this comment

greg0ire commented May 1, 2024

fisharebest commented May 1, 2024

greg0ire commented May 1, 2024 • edited

fisharebest commented May 2, 2024

greg0ire commented May 2, 2024

fisharebest commented Apr 29, 2024 •

edited

greg0ire commented May 1, 2024 •

edited

greg0ire commented May 1, 2024 •

edited

greg0ire commented May 1, 2024 •

edited