Skip to content

Add tests for column collation to prove it works#2919

Merged
Ocramius merged 1 commit intodoctrine:masterfrom
Tobion:test-column-collation
Mar 30, 2018
Merged

Add tests for column collation to prove it works#2919
Ocramius merged 1 commit intodoctrine:masterfrom
Tobion:test-column-collation

Conversation

@Tobion
Copy link
Copy Markdown
Contributor

@Tobion Tobion commented Nov 18, 2017

Fixes #2551

The main point is that one must use the platform or the custom option for the collation and not the standard option for columns. The reasoning I guess is that those options are not cross-RDBMS compatible. There are two things that are inconsistent and causing alot of the confusion:

  1. platform vs custom schema option (both result in the same outcome)
  2. on the column level the option is called collation but on the table level the option is called collate (only relevant for mysql, see https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php#L499 )

@Tobion
Copy link
Copy Markdown
Contributor Author

Tobion commented Dec 26, 2017

@Ocramius this is ready

Copy link
Copy Markdown
Contributor

@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

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

Retriggered the continuousphp build, looks ok now.

self::assertNotContains('DATABASE()', $sql);
}

public function testSupportsColumnCollation()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add : void for all new test methods?

Copy link
Copy Markdown
Contributor Author

@Tobion Tobion Dec 26, 2017

Choose a reason for hiding this comment

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

Sure but shouldn't that be changed everywhere in another PR? Just doing it here is inconsistent.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

True, CS is currently not enforced on project level, except checking for some basic rules for new PRs, But soon will be - if we do it like this now, it won't need further fixes in few weeks. 👍

$table->addColumn('column_collation', 'string')->setPlatformOption('collation', 'ascii_general_ci');

self::assertSame(
array('CREATE TABLE foo (no_collation VARCHAR(255) NOT NULL, column_collation VARCHAR(255) NOT NULL COLLATE ascii_general_ci) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB'),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Short array syntax should be used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as above

@Tobion
Copy link
Copy Markdown
Contributor Author

Tobion commented Jan 1, 2018

@Majkl578 fixed CS.

@Tobion
Copy link
Copy Markdown
Contributor Author

Tobion commented Feb 11, 2018

@Majkl578 @Ocramius please merge

@Tobion
Copy link
Copy Markdown
Contributor Author

Tobion commented Mar 30, 2018

Rebased. Please merge

@Ocramius Ocramius added this to the 2.7.0 milestone Mar 30, 2018
@Ocramius Ocramius self-assigned this Mar 30, 2018
@Ocramius
Copy link
Copy Markdown
Member

👍

@Ocramius
Copy link
Copy Markdown
Member

Had to restart some builds, but otherwise all is green now :) Thanks @Tobion!

@Ocramius Ocramius merged commit 46069b6 into doctrine:master Mar 30, 2018
@Tobion Tobion deleted the test-column-collation branch March 30, 2018 22:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2022
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.

3 participants