OptionValue in install.sql breaks installation on multi-lingual environments

[mlutfy]

The installer creates a new OptionValue for the CiviVolunteer report using an SQL query.

INSERT IGNORE INTO `civicrm_option_value` (`option_group_id`, `label`, `value`, `name`, `grouping`, `filter`, [...]
(@opGId, 'Volunteer Report', 'volunteer', 'CRM_Volunteer_Form_VolunteerReport', NULL, 0, 0, [...]

However, in CiviCRM, SQL queries that affect multi-lingual tables must be wrapped in a {localize} block (ex: a table with a "label", "title", "description", etc field, where the value is user-configurable and language-dependant). Furthermore, the localize is a smarty tag, so the .sql must be processed in Smarty. (in other words, the civicrm-way of localizing .sql files is kind of clunky.)

I'll commit a patch in a second, to show how extensions can enable that, but given that it's just for an OptionValue, it might be better to use the API for that?

PS: CiviVolunteer is awsome! (and we are installing it on the FR demo site) bgm

[ginkgomzd]

Matt, help with localization is much appreciated.

I would much prefer an additional function to the boolean flag. I think the flag hurts readability both where it is called and in the function code.

executeSqlFile() and executeSqlSmartyFile() or executeSqlFileWithSmarty()

I'll be interested in what others have to say about how to set the template directory. Is there a better way than the parent-directory references? Is that CMS agnostic?

Should we open a core issue for the localize token issue? Would be nice for it be easy to tell when that code is no longer needed. https://github.com/mlutfy/civivolunteer/blob/issue180-smarty-sql/CRM/Volunteer/Upgrader/Base.php#L111

...more questions here than answers.

[totten]

1. Smarty-SQL initially feels like an edge-case for me (because I focus on English-only sites), but... once multi-lingual comes into the picture, the Smarty helpers become mandatory, and it makes more sense to use Smarty-SQL by default. It's probably a good include Smarty-SQL support by default in the upgrader.
2. The "Upgrader" comes as two classes -- CRM_Foo_Upgrader and CRM_Foo_Upgrader_Base. The intention is that "CRM_Foo_Upgrader" is hand-crafted while "Base" is autogenerated (and maybe destroyed/recreated/modified by civix). If the patch is really intended for CiviVolunteer (and not other extensions), then all the changes should go into CRM_Foo_Upgrader. (Note that you can override inherited functions.) However, if the intention is to apply to all extensions, then we should patch civix's upgrader-base.php.php.
3. When evaluating multiple Smarty-SQL files, there may be an issue with the current patch adding extraneous entries to the template-search-path.
4. To keep backward compatibility while also making it easy use Smarty, how about this convention: "sql/FOO_install.sql" files are auto-evaluated without Smarty, and "sql/FOO_install.sql.tpl" files are auto-evaluated with Smarty. So the onInstall function contains:

public function onInstall() {
  $files = glob($this->extensionDir . '/sql/*_install.sql');
  if (is_array($files)) {
    foreach ($files as $file) {
      $file = makeRelative($file, $this->extensionDir);
      $this->executeSqlFile($file);
    }
  }
  $files = glob($this->extensionDir . '/sql/*_install.sql.tpl');
  if (is_array($files)) {
    foreach ($files as $file) {
      $file = makeRelative($file, $this->extensionDir);
      $this->executeSqlSmarty($file);
    }
  }
  ...
}

[universalhandle]

I think I agree with everything everyone has said :-)

@mlutfy, thanks for identifying this issue. I should be more diligent about testing in at least a Spanish-language environment, as I am bilingual after all. Does the hardcoded English actually break the install process (what kind of error is thrown?), or do you mean that it's broken in that it fails to respect language settings?

I think it makes sense to avoid SQL where an API alternative is available. As far as CiviVolunteer is concerned, I think the right thing to do is to port the last few lines of volunteer_install.sql to API calls. That is, unless translating the table comments is important to you... but that seems on par with translating code comments, which is going a bit far.

@totten's suggestions about updating Civix seem right to me. Sometimes using the API won't be an option and one will have to resort to writing SQL, in which case @mlutfy's patch (or something like it) will be quite handy. Patching civix would address the issue for all extensions.

[mlutfy]

Yep, I guess I wanted to raise the issue that civix currently does not pass sql through smarty, so custom queries often fail in multi-lingual environments. The better solution in this specific case is to use the API to add new OptionValues.

@GinkgoFJG : the bug is specific to multi-lingual (ex: English + Spanish), rather than localized instances of CiviCRM (ex: Spanish only). In civi multi-lingual, the DB schema is modified to allow multiple values of some internationalized fields, such as titles, labels, descriptions.

[colemanw]

Note this issue will no longer be accessible because I'm removing the github issue tracking option from our repos since it duplicates Jira. Please continue discussions on jira or the forum.