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

fix for comparator so it can correctly see added indexes. #2807

Closed
wants to merge 1 commit into from
Closed

fix for comparator so it can correctly see added indexes. #2807

wants to merge 1 commit into from

Conversation

arminneman
Copy link

@arminneman arminneman commented Aug 2, 2017

starting point:

CREATE TABLE `someTable` (
	`datum` DATE NOT NULL,
	PRIMARY KEY (`datum`),
	INDEX `i_system_date_changed` (`system_date_changed`),
	INDEX `datum` (`datum`)
)
COLLATE='utf8_unicode_ci'
ENGINE=InnoDB
;

new situation:

CREATE TABLE `someTable` (
	`datum` DATE NOT NULL,
	`id` INT(10) UNSIGNED NOT NULL AUTO_INCREMENT,
	PRIMARY KEY (`id`),
	INDEX `i_system_date_changed` (`system_date_changed`),
	INDEX `datum` (`datum`)
)
COLLATE='utf8_unicode_ci'
ENGINE=InnoDB
;

The current comparison in comparator will not see a change here because the index name will return primary for both tables. Causing it not to trigger addedIndexes, which in turn will lead to an incorrect statement where the id key and the primary tag get added separately. MySQL will not allow this because AUTOINCREMENT can only be applied on the primary key.

ALTER TABLE someTable ADD id INT UNSIGNED AUTO_INCREMENT NOT NULL;
ALTER TABLE someTable ADD PRIMARY KEY (id);
SQLSTATE[42000]: Syntax error or access violation: 1075 Incorrect table definition; there can be only one auto column and it must be defined as a key

This, however, is accepted.

ALTER TABLE someTable ADD id INT UNSIGNED AUTO_INCREMENT NOT NULL, ADD PRIMARY KEY (id); 

@Ocramius
Copy link
Member

Ocramius commented Aug 2, 2017

Please target master in a new patch.

Also, tests have to be written beforehand.

Closing as incomplete

@arminneman
Copy link
Author

Thanks for the feedback, so I create a pull request on the master branch? Regarding the tests, I have not added any new features, will the current tests not suffice?

Btw is there a way that allows me to run tests through the CI without polluting the project?

@Ocramius
Copy link
Member

Ocramius commented Aug 3, 2017

Thanks for the feedback, so I create a pull request on the master branch?

Yep!

Regarding the tests, I have not added any new features, will the current tests not suffice?

No, every unexpected behavior needs to be tested to ensure that the fix is correct, and that we don't introduce regressions by changing the code later on.

Btw is there a way that allows me to run tests through the CI without polluting the project?

You can enable travis-ci for your local repository by going to https://travis-ci.org/ and enabling it there.
I still suggest you run your tests locally. See https://github.com/doctrine/dbal/blob/399ab63ddf3db48a4befb7d4f6bcf699e7796d5f/tests/README.markdown

@arminneman
Copy link
Author

Okay, I'll look into it, thanks again!

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.

None yet

2 participants