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

CRM/Logging - Fix log table exceptions #13675

Merged

Conversation

pfigel
Copy link
Contributor

@pfigel pfigel commented Feb 22, 2019

Overview

Fixes a bug with log table exceptions not being observed.

Before

Log table exceptions defined in CRM_Logging_Schema or via hook_civicrm_alterLogTables are not applied unless they're provided with backticks surrounding the column names.

After

Log table exceptions are applied for columns provided with and without backticks.

Comments

Not sure if this never worked or was dependent on a certain version/fork of MySQL. This fix should work with all sane MySQL versions.

This fixes a bug that caused columns that were excluded from being
considered "log-worthy" changes to be logged anyway. That happened
because columns were extracted with backticks but compared to strings
without backticks. To preserve compatibility with exceptions set by
alterLogTables which could contain backticks, the comparison is
performed against the column name with and without backticks.
@civibot
Copy link

civibot bot commented Feb 22, 2019

(Standard links)

@pfigel
Copy link
Contributor Author

pfigel commented Feb 22, 2019

ping @mattwire @michaelmcandrew

@eileenmcnaughton
Copy link
Contributor

this makes sense & test cover works for me :-)

Merging

@eileenmcnaughton eileenmcnaughton merged commit 8aea1e8 into civicrm:master Feb 23, 2019
@pfigel pfigel deleted the fix-logtable-exceptions branch February 23, 2019 10:25
@michaelmcandrew
Copy link
Contributor

thanks @pfigel! I am half way through some paternity leave but I will check this out when I am back, though I see it has been merged in any case :)

@michaelmcandrew
Copy link
Contributor

Just a random update from me since I went back to check on the log_civicrm_group table today. Amount of churn vastly reduced 👍

@eileenmcnaughton
Copy link
Contributor

@michaelmcandrew good to hear!

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

Successfully merging this pull request may close these issues.

3 participants