Skip to content

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

Closed
arminneman wants to merge 1 commit intodoctrine:2.6from
arminneman:2.6
Closed

fix for comparator so it can correctly see added indexes.#2807
arminneman wants to merge 1 commit intodoctrine:2.6from
arminneman:2.6

Conversation

@arminneman
Copy link
Copy Markdown

@arminneman arminneman commented Aug 2, 2017 •
edited
Loading

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);

@arminneman
9f23877 
fix for comparator so it can correctly see added indexes.
@Ocramius
Copy link
Copy Markdown
Member

Ocramius commented Aug 2, 2017

Please target master in a new patch.

Also, tests have to be written beforehand.

Closing as incomplete

@Ocramius Ocramius closed this Aug 2, 2017
@Ocramius Ocramius self-assigned this Aug 2, 2017
@arminneman
Copy link
Copy Markdown
Author

arminneman commented Aug 2, 2017

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
Copy Markdown
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
Copy Markdown
Author

arminneman commented Aug 4, 2017

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

This was referenced Oct 4, 2018
Closed
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@Ocramius Ocramius

Labels

Bug Indexes Missing Tests MySQL Schema Comparison

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@arminneman @Ocramius