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

SQLite: Fix wrongly detected reference constraint name on schema change #4246

Merged
merged 1 commit into from
Sep 3, 2020
Merged

Conversation

MyIgel
Copy link
Contributor

@MyIgel MyIgel commented Sep 2, 2020

Q A
Type bug
BC Break no?
Fixed issues #4243

Summary

This fixes #4243 as other parts of the code expect the constraint name to be null, for example \Doctrine\DBAL\Schema\ForeignKeyConstraint::__construct if no actual name exists

@greg0ire
Copy link
Member

greg0ire commented Sep 2, 2020

I'd expect #4243 (comment) to be summarized in the commit message body, right now it's quite hard to understand what is going on here from the commit message alone.

@morozov
Copy link
Member

morozov commented Sep 2, 2020

Thank you for the PR, @MyIgel. Please make sure there's a functional test that reproduces the problem. The changes in the existing test are clear enough but insufficient.

If there is no constraint name defined the listTableForeignKeys should not invent a new one as a numerical id has no deeper meaning and makes schema changes adding information that was not there before.
Using `null` is an apropriate value as its already in use when handling not existing fk names.
@MyIgel
Copy link
Contributor Author

MyIgel commented Sep 2, 2020

Please make sure there's a functional test that reproduces the problem.

@morozov Do you have an example how such a test would look like? #4244 should be a good start but as i have no overview about the dbal tests some hints would be nice here :)

@MyIgel
Copy link
Contributor Author

MyIgel commented Sep 2, 2020

I'd expect #4243 (comment) to be summarized in the commit message body

I added a more verbose explanation, i hope its more understandable now

@morozov
Copy link
Member

morozov commented Sep 3, 2020

@MyIgel sorry, I didn't notice that the test being changed is a functional one. The change looks good to me given the explanation in #4243 (comment).

@greg0ire
Copy link
Member

greg0ire commented Sep 3, 2020

i hope its more understandable now

It's a great commit message, you can be proud of it!

@greg0ire greg0ire merged commit cd2ddda into doctrine:2.10.x Sep 3, 2020
@greg0ire
Copy link
Member

greg0ire commented Sep 3, 2020

Thanks @MyIgel !

@MyIgel MyIgel deleted the patch-1 branch September 3, 2020 08:36
@morozov morozov removed this from the 2.10.4 milestone Sep 12, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 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