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.

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

Infer omitted column charset from collation #5471

Merged
merged 1 commit into from Jul 2, 2022

Conversation

morozov
Copy link
Member

@morozov morozov commented Jul 2, 2022

Q A
Type bug

Fixes #5338.

@morozov morozov added this to the 3.3.8 milestone Jul 2, 2022
@morozov morozov marked this pull request as ready for review July 2, 2022 03:30
@morozov morozov requested review from derrabus and greg0ire July 2, 2022 03:45
*/
interface CollationMetadataProvider
{
public function getCollationCharset(string $collation): ?string;
Copy link
Member

Choose a reason for hiding this comment

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

What does null mean here? "This collation has no charset"? "We don't know about this collation"? Both? If both, should the second case result in an exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

What does null mean here?

"This collation has no charset" is impossible, so it's "We don't know about this collation".

[…] should the second case result in an exception?

It doesn't have to. Should strpos() throw an exception if the substring is not found? The calling code handles null by not updating options, so if the collation is invalid, the DDL will eventually fail once it hits the database.

If we decide to throw an exception here, how do we unit-test the comparator without the database connection? Right now, we supply it with a provider that always returns null, which is fine because no unit tests rely on this feature.

Copy link
Member

Choose a reason for hiding this comment

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

It's clearer, thanks for the explanation.

@morozov morozov merged commit 82fa2f1 into doctrine:3.3.x Jul 2, 2022
@morozov morozov deleted the issues/5338-charset-from-collation branch July 2, 2022 19:11
@Ocramius
Copy link
Member

Ocramius commented Sep 28, 2022

Note to self: this will do something alike:

$column->setPlatformOptions([
    'collation' => 'utf8_bin',
    'charset' => 'utf8',
]);

What I found while testing locally (couldn't investigate further, so this is mostly a note to self, in order to figure out how to debug + open an issue about it), is that the order of the platform options may be scrambled.

Sometimes, 'collation' comes first, other times, 'charset' comes first.

This seems to trip the whole schema comparison somewhere, although I couldn't pinpoint it yet.

EDIT: errata: it's just the table charset being diffed with my own column definition. Very confused about it, need to check deeper.

EDIT2: possibly found the culprit, and it's possibly a weird edge case that doesn't really need fixing. Given a table like this:

CREATE TABLE a_table
(
   a_column VARCHAR(255) COLLATE utf8_bin NULL,
)
    charset = utf8;

The following table definition seems to always produce a diff:

new Table(
    'a_table',
    new Column(
        'a_column',
        Type::getType(Types::STRING),
        ['platformOptions' => ['collation' => 'utf8_bin, 'charset' => 'utf8']]
)

This makes sense, because we don't want to "remove" the charset from columns that have an utf8 charset, and are already part of a table with utf8 charset: we mostly want to ignore that.

This will produce ALTER statements like:

ALTER TABLE a_table CHANGE a_column a_column VARCHAR(255) CHARACTER SET utf8 DEFAULT NULL COLLATE `utf8_bin`;

Next up for me is dusting off the DBAL test suite and making it reproducible (plus new issue)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema validation error (and diff error) when collation is set
3 participants