(dev/core#183) Temporary tables should follow consistent naming convention

[totten]

Overview

This provides incremental progress towards stronger standardization of temporary table names, with the expectation that all temp tables will match these prefixes:

• civicrm_tmp_e_% matches any true temp table (i.e. "temporary-during-this-connection"; CREATE TEMPORARY TABLE). The "e" stands for "ephemeral".
• civicrm_tmp_d_% matches any quasi-temp table (i.e. "temporary-for-a-few-minutes-or-hours"; CREATE TABLE). The "d" stands for "durable".
• civicrm_tmp_% matches any temp table, whether ephemeral or durable.

This PR adds a new utility-class and updates a handful of use-cases based on the utility-class. The use-cases were chosen arbitrarily. Subsequent PRs will be needed to update more use-cases.

See: https://lab.civicrm.org/dev/core/issues/183

Before

• The following use-cases generate temp tables like:
  • Search-tasks (e.g civicrm_task_action_temp_abcd1234 per DAO::createTempTableName())
  • Exports search-tasks (e.g. civicrm_export_temp_abcd1234 per DAO::createTempTableName())
  • LYBUNT report (e.g. civicrm_report_temp_lybunt_c_abcd1234)
• CRM_Core_Config::clearTempTables() truncates a handful of random prefixes.

After

• DAO::createTempTableName() is deprecated in favor of CRM_Utils_SQL_TempTable
• The following use-cases generate temp tables like:
  • Search-tasks (e.g civicrm_tmp_d_tskact_abcd1234 per TempTable)
  • Exports search-tasks (e.g. civicrm_tmp_d_export_abcd1234 per TempTable)
  • LYBUNT report (e.g. civicrm_tmp_d_rptlybunt_abcd1234 per TempTable)
• CRM_Core_Config::clearTempTables() truncates fewer random prefixes.

Technical Details

The contracts for CRM_Utils_SQL_TempTable and CRM_Core_DAO::createTempTableName() are different in that:

• DAO::createTempTableName()does not differentiate between ephemeral and durable temp-tables. TempTable does.
• DAO::createTempTableName() does not provide a true standard prefix. It only standardizes an infix (_temp_) which is not a very good way to define replication policies. By contrast, TempTable provides a standard+distinctive prefix. A naive patch to DAO::createTempTableName() would be hard to QA -- because it affects too many bespoke use-cases at once. Hence the transition to a new interface.
• TempTable->getName() can be dropped-in as a replacement for DAO::createTempTableName(). This is ideal for updating existing code safely.
• TempTable->create*() and TempTable->drop() can be used as an "improved interface" that captures some issues Eileen mentioned (eg use utf8; allow local-devs to force temp tables to be durable while debugging; also, ensure that create/drop notations match).

For testing the updates, I generally followed this process:

• Identify a use-case that makes a temp table -- and specifically identify the line which names the table.
• Change the line to do something silly (throw new Exception("Foozaaggla baagle")). Run it and ensure our use-case actually hits the line.
• Change the line to do what we want. Run it and ensure there's no error.

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

[totten]

jenkins, test this please

[eileenmcnaughton]

Given that the most common syntax above seems to be setCategory the omission of setCategory (& possibly setId) from these examples seems worth fixing

[totten]

Agree we should give an example of that in the docblock, so I'll add it.

But I'd also keep the existing examples. The setCategory() and setId() should be optional -- most cases should work without them. But I did use them in refactoring because... I'm conservative. To wit: if someone previously called DAO::createTempTableName() and set the equivalent options, then I suppose they may have had a reason, and imitating them reduces our risk.

Digging in a little:

• With the predecessor createTempTableName(), about ~70% (9/13) specified a prefix (equiv to setting category) and ~7% (1/13) specified an id.
• The main benefit of setting a category is for debugging -- it makes it easier to read a query.
• The main benefit of setting an ID is reproducibility. Usually, we don't care if temptables have reproducible IDs (and we often explicitly don't want them to be reproduced). In the six cases I've updated, the only example which smelled like it mattered was CRM/Contact/Form/Task.php. (But I didn't really test -- I just kept the old ID to be safe.)

[eileenmcnaughton]

test this please

[eileenmcnaughton]

I checked the places where the table is changed in here & they appear correct. No fatals when testing!