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

Improve output triggers when logged to file #20472

Merged
merged 1 commit into from
Jun 18, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 2, 2021

Overview

When logging is enabled or disabled with the setting logging_no_trigger_permission set to TRUE the trigger sql is output to a file instead of being run. This is used by wmf & probably no-one else.

The issue we are having is the output sql has become a bit messy & inconsistent in terms of

  1. extraneous db names in it Fix Schema calculation of usePrefix to cope with rpow: #20471
  2. the sorting of the tables has somehow changed making a diff tricky
  3. duplication - the calculation of triggers to drop happens more than once - resulting in them all being written out once and then being interspersed into the create statements
  4. verbosity - the separators are unset & reset after each line.

This cleans it up by

  1. see Fix Schema calculation of usePrefix to cope with rpow: #20471
  2. adding an asort
  3. putting all the triggers into an array keyed by the statements (for deduping) and only outputting them on class destruct
  4. with them all being written at once we can open and close the delimiters just once

Before

drush cvapi Setting.create logging_no_trigger_permission=1
drush cvapi Setting.create logging=1

The output is to a file in config and log - it is overly verbose

image

After

Per above but logging is less verbose

Technical Details

Comments

@civibot
Copy link

civibot bot commented Jun 2, 2021

(Standard links)

@civibot civibot bot added the master label Jun 2, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the trigger_clean branch 3 times, most recently from 3f324de to a1840cc Compare June 2, 2021 07:18
@eileenmcnaughton
Copy link
Contributor Author

test this please

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jun 8, 2021
At some point these stopped being consistently alpha sorted - which doesn't matter
if you are just letting Civi run the trigger updates but if you output it
and diff it this inconsistency is a problem

Subset of civicrm#20472
in the hope of getting this merged

civicrm#20471 also grooms this output for diffing
albeit only in an edge case
@seamuslee001
Copy link
Contributor

O_o odd test failure lets see it if repeats

CRM_Utils_versionCheckTest::testCronFallback
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     '4.3' => Array (...)
+    '4.2' => Array (...)
+    '4.4' => Array (...)
+    '4.5' => Array (...)
+    '4.6' => Array (...)
 )

@seamuslee001
Copy link
Contributor

Jenkins re test this please

Hmm write on rebuild AND reap on destruct
@demeritcowboy
Copy link
Contributor

Is this the one you were looking for review? I can take a look.

@eileenmcnaughton
Copy link
Contributor Author

Thanks @demeritcowboy - there is another one too which can be tested in conjuntion - #20471 - note we discussed #24071 & you just need to test for no breakage - the rpow stuff itself doesn't need testing as it's obscure / only used by us & the change should be a clean up on it's own

@demeritcowboy
Copy link
Contributor

jenkins retest this please

@demeritcowboy
Copy link
Contributor

I reviewed both PRs at the same time. Looks good.

I wasn't aware this feature existed. The only differences in the final result (when imported) I see is that the Definer is different but that's a good thing.

@demeritcowboy demeritcowboy merged commit 16618bf into civicrm:master Jun 18, 2021
@eileenmcnaughton eileenmcnaughton deleted the trigger_clean branch June 19, 2021 23:35
@eileenmcnaughton
Copy link
Contributor Author

thanks @demeritcowboy !

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