Skip to content

Detect changes on the column collation (Issue #2400)#2401

Closed
j-d wants to merge 2 commits intodoctrine:4.0.xfrom
j-d:issue_2400
Closed

Detect changes on the column collation (Issue #2400)#2401
j-d wants to merge 2 commits intodoctrine:4.0.xfrom
j-d:issue_2400

Conversation

@j-d
Copy link
Copy Markdown

@j-d j-d commented May 20, 2016

I cannot run the unit tests on my current environment so I would appreciate if someone could make sure that it is ok.

I have replicated the logic on the Comparator that was already there for the platform options.


foreach (array_keys(array_intersect_key($platformOptions1, $platformOptions2)) as $key) {
if ($properties1[$key] !== $properties2[$key]) {
foreach (array_merge(array_keys($platformOptions1), array_keys($platformOptions2)) as $key) {
Copy link
Copy Markdown
Member

@deeky666 deeky666 Jun 2, 2016

Choose a reason for hiding this comment

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

Hmmm this does not seem to be working (see Travis). IMO the logic should be as follows: Do an array_diff_key() and mark all key differences as changed, do an array_intersect_key() and mark all keys as changed where the values do not match. However I am not sure how this will behave with "default values" like collation which DBAL sets implicitly in the platform (MySQL's default collation for tables for example) if not explicitly set. This could lead to undesired diffs, too. So you might have to play around a bit and get a working test environment going on your machine.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for your feedback @deeky666 . I will try to get it working in my environment. Do you think there is also a problem here https://github.com/j-d/dbal/blob/fba875af209772b5ce0446d4adaa59da8c4fe6c2/lib/Doctrine/DBAL/Schema/Comparator.php#L480-L486 ? I basically copy/pasted from there and adjusted it.
Thanks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking at this again, it seems that it should be doing already what I was suggesting, even though a lit bit more complicated. So you might just add some more tests to reveal any issues.

@morozov
Copy link
Copy Markdown
Member

morozov commented Jul 9, 2021

Closing as stale. The issue should be addressed by #4659.

@morozov morozov closed this Jul 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 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