1.17.2 breaks queries when an alternate database prefix is specified

[MegaphoneJon]

Description of the bug

I keep CiviCRM in a separate database from Backdrop. The 1.17.2 release breaks Views of CiviCRM data.

Steps To Reproduce

My settings.php has an array that looks like this:

$databases['default']['default']['prefix']= array(
  'default' => '',
  'civicrm_acl'                              => '`acd_civi`.',
  'civicrm_acl_cache'                        => '`acd_civi`.',
  'civicrm_acl_contact_cache'                => '`acd_civi`.',
  'civicrm_acl_entity_role'                  => '`acd_civi`.',
  'civicrm_action_log'                       => '`acd_civi`.',
  'civicrm_action_mapping'                   => '`acd_civi`.',
  'civicrm_action_schedule'                  => '`acd_civi`.',
  'civicrm_activity'                         => '`acd_civi`.',
  'civicrm_activity_contact'                 => '`acd_civi`.',
  'civicrm_address'                          => '`acd_civi`.',
<snip>
);

Any View built on CiviCRM data fails.

Actual behavior

Views based on this database fail with:

SQLSTATE[42000]: Syntax error or access violation: 1103 Incorrect table name '' 

Expected behavior

View displays.

Additional information

The issue is in the new function DatabaseConnection_mysql::prepareQuery(), which looks like it's adding backticks for MySQL 8 compatibility.

However, this causes extra backticks, something like:

SELECT display_name from ``acd_civi`.civicrm_contact`

I wrote a patch which is almost certainly wrong, but can hopefully illustrate the problem well enough to facilitate discussion.

---

PR: backdrop/backdrop#3417

[MegaphoneJon]

Well, after some thought, I realize that the CiviCRM Views integration may fall into the category of "dirty hack", since I don't think the intended use of prefixes is to specify an alternative database.

If that's the case, I don't mind changing the dirty hack to something equally dirty. I figured out I could account for the extra backticks by changing a line like this:

  'civicrm_address'                          => '`acd_civi`.',

to this:

  'civicrm_address'                          => 'acd_civi`.`',

Hopefully the Backdrop/Civi experts can chime in on that.

[stpaultim]

One of the main items in this latest release was an update to provide support for MySQL 8. We need to consider whether or not this issue is any indication that this update might cause problems for other users in other situations.

#4238

[indigoxela]

I figured out I could account for the extra backticks by changing a line like this

@MegaphoneJon did that actually fix the queries for you? I agree, Civi does some hackish stuff, and I don't think that prefixes in settings.php need or should have any backticks.

Hopefully the Backdrop/Civi experts can chime in on that.

@stpaultim already pinged the CiviCRM users in the Zulip chat.

[herbdool]

Yeah I hadn't thought about it being a hack. There is a proper way to specify an alternative database but then we might lose the integration with views. Not sure.

Seems that we may need to update the docs either way.

[MegaphoneJon]

@indigoxela Yes, that change fixed the Views integration for me.

If it's decided that the 1.17.2 code stays as-is, we should probably implement a Civi check. It wouldn't occur to me to check the Views integration docs on a point upgrade. I'll wait until folks have a chance to look this over.

[indigoxela]

@MegaphoneJon many thanks for providing a pull request. It has some issues, though.

1. Two related tests are failing
2. Removing function prepareQuery() breaks MySQL 8 compatibility (see MySQL 8 support #4238)

[MegaphoneJon]

Understood @indigoxela - the PR was more raised to better illustrate the issue. At this point, I'm guessing that it's best to assume there's no easy solution in code that supports both scenarios, and I'm going to update my sites' settings.php as noted in my comment above.

[jlfranklin]

This was fixed in a later version of the Drupal 7 patch than what we used in #4238. In the last 24 hours, the Drupal patch has been tagged "pending Drupal 7 commit."

To reduce drift between D7 and BD, I suggest we update our MySQL 8 patch to better match their final patch.

[quicksketch]

Thanks @MegaphoneJon for the detailed information on how this is affecting CiviCRM and Backdrop integration. I agree with @indigoxela that backticks shouldn't be necessary in the settings.php database configuration.

I filed a PR at backdrop/backdrop#3417 that might help in this situation, but I don't think it would work without modification based on your previous configuration. The approach in that PR makes it so that you can specify databases as prefixes, but they wouldn't require (or even work) with backticks already specified. So it should work if your database was specified like this:

$databases['default']['default']['prefix']= array(
  'default' => '',
  'civicrm_acl'                              => 'acd_civi.',
  'civicrm_acl_cache'                        => 'acd_civi.',
  'civicrm_acl_contact_cache'                => 'acd_civi.',
<snip>
);

I would say using MySQL's ability to query across databases is generally a "hack", but it's been one that Drupal/Backdrop have been capable of for a very long time. I've seen this approach used not only on CiviCRM but also in some cases where some tables are separated out to different database servers or shared across multiple sites. So while it's not common, it's long been possible to share or split data across databases.

[laryn]

WFM on a Civi site that broke with the latest few updates that @quicksketch's PR fixes things (after I remove backticks in the settings.php integration).

[klonos]

Would it be possible (and make sense) to detect backticks in settings.php, remove them, and throw a warning that they need to be fixed?

[klonos]

...never mind. I just checked @quicksketch's PR 👍

[klonos]

...the Drupal patch has been tagged "pending Drupal 7 commit."

To reduce drift between D7 and BD, I suggest we update our MySQL 8 patch to better match their final patch.

I agree @jlfranklin 👍 ...and in the meantime, the patch was committed to D7 core a few days ago: https://git.drupalcode.org/project/drupal/commit/e189264

[seamuslee001]

It looks like this was how Drupal 7 fixed the same sort of issue https://git.drupalcode.org/project/drupal/commit/6b541fb https://www.drupal.org/i/3186120 not sure if its portable to Backdrop or not

[jenlampton]

Code in the PR looks great. Love the inline comments 👍

[quicksketch]

We had a failing test in the PR, which turned up an actual issue that we have a few places in core that use this sequence of calls:

  $tables = db_find_tables(Database::getConnection()->prefixTables('{table_name}') . '%');

This was a problem with the addition of back-ticks, because it would result in a LIKE query such as this:

SELECT table_name FROM information_schema.tables WHERE table_name LIKE "`table_name`%"

I pushed an additional commit that strips back-ticks when the DatabaseSchema::findTables() method is called.

We need code review after the new additions.

[klonos]

Thanks @quicksketch 🙏 ...code still looks good 👍

[quicksketch]

With the confirmations and code reviews, I've merged backdrop/backdrop#3417 into 1.x and 1.17.x. Thanks everyone for the reviewing and testing!

[herbdool]

This dropped off my radar. So in the end the database prefix specified can not have any backticks, correct? In my testing at least it doesn't like backticks anymore. So neither

'abc_civi`.`',

nor the original

'`abc_civi`.',

will work. Only this works:

'abc_civi.',

@MegaphoneJon are you aware of an issue in a Civi repo for this? I can look in the backdrop Civi repo.

[herbdool]

Here's an issue https://github.com/civicrm/civicrm-backdrop/issues/82

[herbdool]

I created an issue in civicrm-core: https://lab.civicrm.org/dev/core/-/issues/2352