Update index name quoting in MySQL table creation #730
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-1051 We use Jira to track the state of pull requests and the versions they got |
| } | ||
| } | ||
|
|
||
| // add all indexes | ||
| if (isset($options['indexes']) && ! empty($options['indexes'])) { | ||
| foreach ($options['indexes'] as $index => $definition) { | ||
| $queryFields .= ', ' . $this->getIndexDeclarationSQL($index, $definition); | ||
| $queryFields .= ', ' . $this->getIndexDeclarationSQL($definition->getQuotedName($this), $definition); |
There was a problem hiding this comment.
Is this only for the MySql platform? Can you check if other platforms have similar leaks?
|
@Ocramius MySQL seems to be the only engine that creates indexes inline with table creation. All the other platform need a different query to create the index that go through the I did however move the quoting to the abstract so that it brings it all to one location. |
|
Looking good. Assigning to @deeky666 for merge. |
|
hmm not sure if it is a good idea to totally ignore the |
|
Also maybe related? #723 |
|
I would argue that this PR is doing the same as #723 but I do see how quoting the index name earlier in the process maintains the documentation and interface around the Is #723 likely to get merged in any time soon, if so I am happy to close this one, or if any changes are needed I am happy to push them through. I'm currently supporting a forked version of the code base to support my requirements, starting to generate our schema using the schema-tool. Unfortunately we already have an indexed names |
|
@garoevans I think that other PR is complementary. The idea behind your patch is not wrong as those methods are public API and can be used separately, it's just that I think the implementation is not correct. I would rather go for quoting the $name = new Identifier($name);
$name->getQuotedName($this);This is a workaround we are already utilizing in several code paths. |
|
@deeky666 that looks good to me. I'll try and make the change later today and then leave it in your capable hands! |
|
@deeky666 does this need any more input from me now? |
| @@ -26,6 +26,20 @@ public function testGenerateMixedCaseTableCreate() | |||
| $this->assertEquals('CREATE TABLE Foo (Bar INT NOT NULL) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB', array_shift($sql)); | |||
| } | |||
|
|
|||
| public function testReservedKeywordInIndexGeneratedTableSql() | |||
There was a problem hiding this comment.
Can you please move that test to AbstractPlatformTestCase to have all platforms execute that test? Thanks.
There was a problem hiding this comment.
#730 (comment) mentioned that MySQL is the only engine that creates engines along with a table create statement. I'm not sure the test would be relevant for other engines?
I did have the test higher up the abstraction at one point but it didn't seem right?
There was a problem hiding this comment.
@garoevans the two methods you modified is public API! This means it can be used in any context and is not only trigger by getCreateTableSQL().
Creating indexes and unique constraints is supported by all of our platforms AFAIK. So putting the tests into AbstractPlatformTestCase makes perfect sense.
There was a problem hiding this comment.
Ah, my first iteration of code quoted the index name within the MySQL platform file. Now that it is in the 'AbstractPlatform' class you are quite right that the test should be updated to reflect this.
Bit slow there, will take a look when I get some time.
When creating a table that includes an index named with a reserved keyword the index name isn't quoted. When creating the index specifically the name is quote. This bring the table generation SQL inline with the index generation.