Allow updating ENGINE_CONFIG for System.updatelogtables

[mattwire]

Overview

Allow you to update the ENGINE_CONFIG via System.updatelogtables if you are not changing the actual storage engine.
I needed to go from InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=4 to InnoDB ROW_FORMAT=DYNAMIC and you can't do it via this API function without changing the engine too.

Also return the number of tables we updated as a result instead of "1" in the API call.

Before

Cannot modify SQL engine config for log tables.

After

Can modify SQL engine config for log tables.

Technical Details

Just follows the same principal as existing code. Enabled via parameter so no change by default.

Comments

@seamuslee001 @mfb @pfigel

[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).

[seamuslee001]

Test failure relates @mattwire https://test.civicrm.org/job/CiviCRM-Core-PR/26463/testReport/junit/(root)/CRM_Logging_SchemaTest/testColumnInfo/

[pfigel]

I'm wondering if we really need a new API parameter or if just checking for a difference is good enough. Seems like it would be a sane default behaviour, unless we have good reason to be more conservative. Thoughts?

[mattwire]

@pfigel I think it makes sense to be conservative (and add the param) when we first add it, just in case it should cause any problems.

[pfigel]

@mattwire LGTM!

• General standards
  • (r-explain) PASS
  • (r-user) PASS: No change in default behaviour - hidden behind new parameter with appropriate docs.
  • (r-doc) PASS
  • (r-run) PASS: Confirmed engine config is changed with updateChangedEngineConfig=1 and not changed otherwise.
• Developer standards
  • (r-tech) PASS: Minor (useful) change in return value, unlikely to have ecosystem impact.
  • (r-code) PASS
  • (r-maint) PASS
  • (r-test) PASS

[pfigel]

Whoops, noticed the test failure was already resolved, so definitely LGTM now.

[monishdeb]

@mattwire @pfigel is there anything remaining or I can merged it given that @pfigel has already reviewed and approved it?

[mattwire]

@monishdeb I'm happy!

[monishdeb]

Great .. Let us wait for @pfigel reply and I'll then merge this PR after his approval.

[pfigel]

@monishdeb I'm happy with this!

[monishdeb]

Merged. Thanks guys for all your effort.