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.

Sign up for GitHub

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

fixes incorrect database migration for quoted columns #3395

Closed
wants to merge 6 commits into from
Closed

fixes incorrect database migration for quoted columns #3395

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b3096b2
fixes #3387 does not intersect columns if one is quoted and another i…
svycka Dec 7, 2018
8401951
updated tests to test agains queted
svycka Dec 7, 2018
77f349a
Extracted into separate test
svycka Dec 10, 2018
f58443d
removed improvement from not related test :)
svycka May 20, 2019
806ec1e
I still have to change this test and keeping mock makes harder to tes…
svycka May 20, 2019
6d592fd
Merge branch 'master' into patch-2
svycka Nov 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/Doctrine/DBAL/Schema/ForeignKeyConstraint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ private function onEvent($event)
*/
public function intersectsIndexColumns(Index $index)
{
foreach ($index->getColumns() as $indexColumn) {
foreach ($index->getUnquotedColumns() as $indexColumn) {
foreach ($this->_localColumnNames as $localColumn) {
if (strtolower($indexColumn) === strtolower($localColumn->getName())) {
return true;
Expand Down
25 changes: 18 additions & 7 deletions tests/Doctrine/Tests/DBAL/Schema/ForeignKeyConstraintTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,31 @@

class ForeignKeyConstraintTest extends TestCase
{
/**
* @param string[] $indexColumns
*
* @dataProvider getIntersectsIndexColumnsData
*/
public function testIntersectsIndexEscapedColumns(array $indexColumns, bool $expectedResult)
{
$foreignKey = new ForeignKeyConstraint(['`foo`', '`bar`'], 'foreign_table', ['fk_foo', 'fk_bar']);

$index = new Index('INDEX_NAME', $indexColumns);

self::assertSame($expectedResult, $foreignKey->intersectsIndexColumns($index));
}

/**
* @param string[] $indexColumns
*
* @group DBAL-1062
* @dataProvider getIntersectsIndexColumnsData
*/
public function testIntersectsIndexColumns(array $indexColumns, $expectedResult)
public function testIntersectsIndexColumns(array $indexColumns, bool $expectedResult)
{
$foreignKey = new ForeignKeyConstraint(['foo', 'bar'], 'foreign_table', ['fk_foo', 'fk_bar']);

$index = $this->getMockBuilder(Index::class)
->disableOriginalConstructor()
->getMock();
$index->expects($this->once())
->method('getColumns')
->will($this->returnValue($indexColumns));
$index = new Index('INDEX_NAME', $indexColumns);
Copy link
Member

@Ocramius Ocramius May 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm vary of test changes here: what was broken that required getting rid of the mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing, just thought maybe better to use an actual object instead of mock no?

Copy link
Member

Choose a reason for hiding this comment

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

If the previous code works without changes, let's avoid changing it at all. Let's put test refactoring in separate patches, to avoid hiding potential BC breaks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ocramius as you can see tests fail and now I remember why. Because I use getUnquotedColumns instead of getColumns and I added some columns with quotes into data provider. And if I mock here can't test quoted names. I mean this:

$index->expects($this->once())
            ->method('getUnquotedColumns')
            ->will($this->returnValue($indexColumns));

should always return unquoted names and I can't add quoted names in data provider but maybe I don't have to?
I still think actual implementation would be better here and anyway I have to change mocked method anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ocramius if you want I can still leave this mock but will have to change mocked method and will also have to duplicate this test and test data to actually test quoted names.


self::assertSame($expectedResult, $foreignKey->intersectsIndexColumns($index));
}
Expand All @@ -37,6 +46,8 @@ public function getIntersectsIndexColumnsData()
[['baz'], false],
[['baz', 'bloo'], false],

[['`foo`'], true],
[['`bar`'], true],
[['foo'], true],
[['bar'], true],

Expand Down