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

#7841 SchemaTool generates extra diff for platforms without FK support #7842

Merged
merged 1 commit into from
Oct 2, 2019

Conversation

vpArth
Copy link
Contributor

@vpArth vpArth commented Sep 30, 2019

Add breaking test for #7841

@vpArth
Copy link
Contributor Author

vpArth commented Sep 30, 2019

assertEmpty()

ok, changed

@vpArth
Copy link
Contributor Author

vpArth commented Oct 1, 2019

Commits are squashed after both(scrutinizer, travis) checks passed

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

@vpArth thanks for sending this patch! Here are a few comments 👍

I also believe that this was intended as a bug fix, right? If so, it must use 2.6 as base instead of `master.

@vpArth
Copy link
Contributor Author

vpArth commented Oct 2, 2019

it must use 2.6 as base instead of `master.

vendor/bin/phpcs fails every single file on 2.6 branch. is it ok?

@lcobucci
Copy link
Member

lcobucci commented Oct 2, 2019

it must use 2.6 as base instead of `master.

vendor/bin/phpcs fails every single file on 2.6 branch. is it ok?

@vpArth we have configured it to validate the diff in PRs because of that. You can focus on your changes only and ensure they're correct.

@vpArth
Copy link
Contributor Author

vpArth commented Oct 2, 2019

Should I migrate to new PR to switch target branch to 2.6?

Upd: found option to switch the base =)

@vpArth vpArth changed the base branch from master to 2.6 October 2, 2019 08:29
Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Awesome 🎉
Thanks!

@lcobucci lcobucci merged commit 4b0d86e into doctrine:2.6 Oct 2, 2019
@lcobucci lcobucci self-assigned this Oct 2, 2019
@lcobucci lcobucci added the Bug label Oct 2, 2019
@lcobucci lcobucci added this to the 2.6.5 milestone Oct 2, 2019
@vpArth vpArth deleted the gh-7841 branch October 2, 2019 09:01
@beberlei
Copy link
Member

beberlei commented Dec 1, 2019

This is causing a regression in 2.6.5 and 2.7 for SQLite users: #7930

@beberlei
Copy link
Member

beberlei commented Dec 1, 2019

This needs to be reverted. It fixes the symptom and not the problem.

  1. SQLite does not support foreign keys, but we emulate them. A check for supportsForeignKeyConstraints() is the wrong place, because we interact with DBAL SchemaManager API in SchemaTool that abstracts the platform and if foriegn keys are emulated or supported.

  2. Debugging into the code, the culprit is Comparator::diffTable() detects an "added foreign key", even though it already exists. That means that the SQLite abstraction does not populate Table#foreignKeys with the emulated keys.

  3. The real fix is probably that we let SQLitePlatform#supportsForeignKeyConstraints() return true.

@beberlei
Copy link
Member

beberlei commented Dec 1, 2019

Fix in DBAL: doctrine/dbal#3762

@lcobucci
Copy link
Member

@vpArth we had to revert the changes as @beberlei explained. Unfortunately the fix can only be applied for ORM v2.8.0 since it requires us to bump up DBAL version (to 2.10).

@vpArth
Copy link
Contributor Author

vpArth commented Dec 18, 2019

But what about #7841?
The SchemaTool bug for all platforms with false in supportsForeignKeyConstraints is still here then.

Adding true into SqlitePlatform could fix the regression without reverting that pr, right?

Have we any platform with correct populating of Table#foreignKeys with emulated keys?

@beberlei
Copy link
Member

@vpArth please see doctrine/dbal#3762 - it ads the missing support in SQLite platform when it gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants