Skip to content

Commit

Permalink
(civicrm-setup/1) CRM_Core_I18n - Loosen coupling to DB layer
Browse files Browse the repository at this point in the history
Overview
--------

The `ts()`/`{ts}` feature from `CRM_Core_I18n` has an option to escape output
(e.g.  `{ts escape="sql"}Activities{/ts}`).  However, SQL encoding is subjective
(depending on the connection details). For `{ts}` to support this feature, it
must have a dependency on the DB subsystem (e.g. it eventually calls `CRM_Core_DAO`).

For civicrm/civicrm-setup#1, we have an issue
where expressions like `{ts escape="sql"}Activities{/ts}` fail during
installation because `CRM_Core_DAO` is not fully available.

This change loosens the coupling, so that we can use `{ts escape="sql"}Activities{/ts}`
without needing `CRM_Core_DAO` per se.

Before
------

`ts`/`CRM_Core_I18n` is tightly coupled to `CRM_Core_DAO`. There is no way
to use `{ts escape=sql}Activities{/ts}` without spinning-up `CRM_Core_DAO`.

After
-----

`ts`/`CRM_Core_I18n` *defaults* to calling `CRM_Core_DAO`.  However, this can
be overriden by manipulating a property.

Comments
--------

* I feel a little dirty keeping any coupling between i18n and the DB.
  However, changing this would mean removing support for the
  `{ts escape=sql}` option, and that would be a clear compatibility-break.
* Arguably, there may be a microsecond-level penalty in using
  `call_user_func($SQL_ESCAPER)` rather than a specific class/function.
  However, it's only incurred if you actually call `{ts escape=sql}`
  while setting `$SQL_ESCAPER`, and that's pretty rare circumstance.
  The typical runtime use-cases for `ts()` are unaffected.
  • Loading branch information
totten committed Feb 20, 2018
1 parent 159a0a9 commit 2c4bba5
Showing 1 changed file with 30 additions and 2 deletions.
32 changes: 30 additions & 2 deletions CRM/Core/I18n.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,34 @@ class CRM_Core_I18n {
*/
const NONE = 'none', AUTO = 'auto';

/**
* @var callable|NULL
* A callback function which handles SQL string encoding.
* Set NULL to use the default, CRM_Core_DAO::escapeString().
* This is used by `ts(..., [escape=>sql])`.
*
* This option is not intended for general consumption. It is only intended
* for certain pre-boot/pre-install contexts.
*
* You might ask, "Why on Earth does string-translation have an opinion on
* SQL escaping?" Good question!
*/
public static $SQL_ESCAPER = NULL;

/**
* Encode a string for use in SQL.
*
* @param string $text
* @return string
*/
protected static function escapeSql($text) {
if (self::$SQL_ESCAPER == NULL) {
return CRM_Core_DAO::escapeString($text);
}
else {
return call_user_func(self::$SQL_ESCAPER, $text);
}
}

/**
* A PHP-gettext instance for string translation;
Expand Down Expand Up @@ -289,7 +317,7 @@ public function crm_translate($text, $params = array()) {
// in such cases we return early, only doing SQL/JS escaping
if (isset($params['skip']) and $params['skip']) {
if (isset($escape) and ($escape == 'sql')) {
$text = CRM_Core_DAO::escapeString($text);
$text = self::escapeSql($text);
}
if (isset($escape) and ($escape == 'js')) {
$text = addcslashes($text, "'");
Expand Down Expand Up @@ -351,7 +379,7 @@ public function crm_translate($text, $params = array()) {

// escape SQL if we were asked for it
if (isset($escape) and ($escape == 'sql')) {
$text = CRM_Core_DAO::escapeString($text);
$text = self::escapeSql($text);
}

// escape for JavaScript (if requested)
Expand Down

0 comments on commit 2c4bba5

Please sign in to comment.