dev/core#2709 Enable logging for custom data tables with non-standard names

[eileenmcnaughton]

Overview

dev/core#2709 Enable logging for custom data tables with non-standard names

Before

When a custom group is created and the table_name is specified as 'something random' and then logging is enabled, logging is not enabled for the table

After

Logging behaviour follows normal standards. As with any other table the hook can be used to opt the table out of logging

Technical Details

One line change + comments + test

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

@eileenmcnaughton In what situation would a custom group table get a non-standard name?

This does happen with some extensions by the way (eg. civirule_xx) and I've thought for a long time that it would be really good if an extension had the ability to "declare" what tables it has to civicrm so that core "tables" functions such as logging just work. But I don't think that's the issue you're trying to solve here?

[seamuslee001]

@mattwire I would have thought that sort of information is available via the entityTypes hook but maybe its just not easily exposed to the logging system / logging system doesn't work with entityTypes

[eileenmcnaughton]

@mattwire we have non standard names for custom tables & custom fields in our custom code simply because we want to have the same table names across life & dev installs and to keep them simple and clean. This makes is a lot easier for integrations that interact directly with the database. Specifying the table name & field name is supported via unit tests - but there is no reason for there to be any implied 'magic' - which is what we currently have.

As @seamuslee001 says - you can declare entity tables via the hook (which is called if you use the xml + civix to generate your entity) - the missing piece is for our schema to have a parameter for 'logging' - or rather, it DOES but it maps to the change log not to the db logging which is what I mean. I do think that would be good in the schema and it would be used for core tables too - but it IS a separate issue

[mattwire]

@seamuslee001 @eileenmcnaughton Thanks for clarifying. I'm happy with the concept here. Do either of you have any idea why logging doesn't work with extension entity tables when they have non-standard names? .. and whether that could be fixed easily?

A good example is civirules where all tables start with civirule_xx and do not get included in db logging - there are other extensions that have the same problem. They declare the entity tables correctly.

[eileenmcnaughton]

@mattwire it comes from https://github.com/civicrm/civicrm-core/pull/20918/files#diff-38921d05e11c37ba518426a68b4d962a6df048cbfc87f41c0e67dd02dc9c86faR129

• That code could add in code from the entityTables hook - but I think we should define something in the schema eg
  <database_logging>FALSE</database_logging>
  that would allow the schema to influence it

• <logging></logging> is already used

Steps would be

1. agree the parameter name
2. add to GenCode - probably use TRUE as the default in gencode
3. update the function & test in this PR reflect that dao value
4. bonus - rip out all the exceptions in the function that exist because we don't have ^^ (eg. skipping civicrm_log)

[mattwire]

@eileenmcnaughton yes agree - database_logging is fine by me

[eileenmcnaughton]

I've added https://lab.civicrm.org/dev/core/-/issues/2712 for the logging issue - looks like I have a jenkins issue here

[eileenmcnaughton]

test passes locally - let's see if that works

[demeritcowboy]

Interesting: The underscore is a wildcard character with LIKE, so this would skip some intended tables (e.g. one called civicrmstuff), except because line 129 includes those tables when it didn't intend to, it works out. So 🤷

[mattwire]

Right, I think you need to use a "regex" here ie:

Suggested change

	where table_name NOT LIKE 'civicrm_%';
	where table_name NOT LIKE 'civicrm\_%';

(see api/v3/System.utf8conversion)

[demeritcowboy]

Yes just then a table like 'civicrm4ever' would get included twice but you could run array_unique on the results. Or could change line 129 to also do \_, but a problem would be that that would start excluding non-custom-field tables that were being logged before if they had names like civicrm4ever.

[eileenmcnaughton]

@demeritcowboy @mattwire that is the same WHERE as is used to include a few lines earlier so it's basically

• step one include all tables where table_name LIKE 'civicrm_%';
• step two include custom tables WHERE table_name NOT LIKE 'civicrm_%';

I tried doing it as a union but the charset didn't match when ci ran

[eileenmcnaughton]

(ie the WHERE clause only exists because we might as well not get ones we already have)

[eileenmcnaughton]

Currently if you have table called civicrm4eva then it will be included regardless of whether it is a custom table or not - I don't want to change that and I don't think this will.

[demeritcowboy]

Right that's why I gave the shrug - this PR as-is is the most consistent, with the downside the queries are technically wrong, but they complement each other.

[eileenmcnaughton]

@demeritcowboy yeah & changing would create a functional change - which I don't want to do