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 #17 - Virtual Blob Columns were not treated as virtual #19

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

amenk
Copy link

@amenk amenk commented Nov 15, 2022

  • Virtual columns have a special handling
  • If there are such columns, we need to switch to complete inserts for the table and omit the virtual values in the INSERTs
  • If a column is virtual and blob, due to an error in the if-priority it was no longer treated as virtual
  • Additionally the complete-insert switching also affected consequent tables, this PR isolates this to affect only the current table with the virtual columns

* Backup and restore the complete-insert settings, so it affects only tables with
virtual columns and not all following ones
@back-2-95
Copy link
Member

Tests pass.

So this is only for 5.7 ?

Could you provide the background on the PR description what is done here and why.

@amenk
Copy link
Author

amenk commented Nov 23, 2022

Actually I am not sure if it is only for 5.7 -- should probably apply to all versions with virtual fields

Comment updated

@back-2-95
Copy link
Member

Aah, I just read the diffs and there was 5 and 7 used for ifs so I assumed.

@amenk
Copy link
Author

amenk commented Nov 23, 2022

okay, looks like my tests just would run for >= 5.7 but I think that is okay, I copied that from the other tests with virtual columns

@amenk
Copy link
Author

amenk commented Nov 28, 2022

@back-2-95 can it be merged or do you have any other comments?

@back-2-95 back-2-95 merged commit 5850725 into druidfi:main Nov 30, 2022
@back-2-95
Copy link
Member

@amenk It's now merged. I'm not gonna do a release yet until I have tested this in real projects.

@amenk
Copy link
Author

amenk commented Dec 5, 2022

@back-2-95 okay, cool, keep me posted :-)

@amenk
Copy link
Author

amenk commented Jan 6, 2023

Did you have a chance to test in a real project, yet?
Also now with the PHP8.2 support a tagged release would be awesome :-)

See also Smile-SA/gdpr-dump#79

@back-2-95
Copy link
Member

Hi @amenk not yet but I created a patch release https://github.com/druidfi/mysqldump-php/releases/tag/1.0.2

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

Successfully merging this pull request may close these issues.

2 participants