Issue #1785086 follow-up by Jose Reyero: Some general clean-up for translation string API.

core/modules/locale/lib/Drupal/locale/LocaleLookup.php

@@ -55,13 +55,9 @@ public function __construct($langcode, $context, $stringStorage) {
    */
   protected function resolveCacheMiss($offset) {
     $translation = $this->stringStorage->findTranslation(array(
-      // These are the search conditions.
       'language' => $this->langcode,
       'source' => $offset,
       'context' => $this->context
-    ), array(
-      // Search options. We just need this limited set of fields.
-      'fields' => array('lid', 'version', 'translation'),
     ));
 
     if ($translation) {

core/modules/locale/lib/Drupal/locale/StringBase.php

@@ -9,6 +9,9 @@
 
 /**
  * Defines the locale string base class.
+ *
+ * This is the base class to be used for locale string objects and contains
+ * the common properties and methods for source and translation strings.
  */
 abstract class StringBase implements StringInterface {
   /**
@@ -108,6 +111,13 @@ public function setPlurals($plurals) {
     return $this;
   }
 
+  /**
+   * Implements Drupal\locale\StringInterface::getStorage().
+   */
+  public function getStorage() {
+    return isset($this->storage) ? $this->storage : NULL;
+  }
+
   /**
    * Implements Drupal\locale\StringInterface::setStorage().
    */
@@ -145,7 +155,14 @@ public function getValues(array $fields) {
    * Implements Drupal\locale\LocaleString::save().
    */
   public function save() {
-    $this->getStorage()->save($this);
+    if ($storage = $this->getStorage()) {
+      $storage->save($this);
+    }
+    else {
+      throw new StringStorageException(format_string('The string cannot be saved because its not bound to a storage: @string', array(
+        '@string' => $string->getString()
+      )));
+    }
     return $this;
   }
 
@@ -154,26 +171,16 @@ public function save() {
    */
   public function delete() {
     if (!$this->isNew()) {
-      $this->getStorage()->delete($this);
-    }
-    return $this;
-  }
-
-  /**
-   * Gets the storage to which this string is bound.
-   *
-   * @throws Drupal\locale\StringStorageException
-   *   In case the string doesn't have an storage set, an exception is thrown.
-   */
-  protected function getStorage() {
-    if (isset($this->storage)) {
-      return $this->storage;
-    }
-    else {
-      throw new StringStorageException(format_string('The string cannot be saved nor deleted because its not bound to an storage: @string', array(
+      if ($storage = $this->getStorage()) {
+        $storage->delete($this);
+      }
+      else {
+        throw new StringStorageException(format_string('The string cannot be deleted because its not bound to a storage: @string', array(
           '@string' => $string->getString()
-      )));
+        )));
+      }
     }
+    return $this;
   }
 
 }

core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.php

@@ -49,7 +49,6 @@ public function __construct(Connection $connection, array $options = array()) {
    * Implements Drupal\locale\StringStorageInterface::getStrings().
    */
   public function getStrings(array $conditions = array(), array $options = array()) {
-    $options += array('source' => TRUE);
     return $this->dbStringLoad($conditions, $options, 'Drupal\locale\SourceString');
   }
 
@@ -57,16 +56,14 @@ public function getStrings(array $conditions = array(), array $options = array()
    * Implements Drupal\locale\StringStorageInterface::getTranslations().
    */
   public function getTranslations(array $conditions = array(), array $options = array()) {
-    $options += array('source' => TRUE, 'translation' => TRUE);
-    return $this->dbStringLoad($conditions, $options, 'Drupal\locale\TranslationString');
+    return $this->dbStringLoad($conditions, array('translation' => TRUE) + $options, 'Drupal\locale\TranslationString');
   }
 
   /**
    * Implements Drupal\locale\StringStorageInterface::findString().
    */
-  public function findString(array $conditions, array $options = array()) {
-    $options += array('source' => TRUE);
-    $string = $this->dbStringSelect($conditions, $options)
+  public function findString(array $conditions) {
+    $string = $this->dbStringSelect($conditions)
     ->execute()
     ->fetchObject('Drupal\locale\SourceString');
     if ($string) {
@@ -78,9 +75,8 @@ public function findString(array $conditions, array $options = array()) {
   /**
    * Implements Drupal\locale\StringStorageInterface::findTranslation().
    */
-  public function findTranslation(array $conditions, array $options = array()) {
-    $options += array('source' => TRUE, 'translation' => TRUE);
-    $string = $this->dbStringSelect($conditions, $options)
+  public function findTranslation(array $conditions) {
+    $string = $this->dbStringSelect($conditions, array('translation' => TRUE))
     ->execute()
     ->fetchObject('Drupal\locale\TranslationString');
     if ($string) {
@@ -324,34 +320,16 @@ protected function dbStringLoad(array $conditions, array $options, $class) {
    *   An associative array of additional options. It may contain any of the
    *   options used by Drupal\locale\StringStorageInterface::getStrings() and
    *   these additional ones:
-   *   - 'source', TRUE for selecting all source fields.
-   *   - 'translation', TRUE for selecting all translation fields.
-   *   - 'fields', Optional array of exact fields to get. Overrides the
-   *     previous 'source' and 'translation' options. Defaults to none.
+   *   - 'translation', Whether to include translation fields too. Defaults to
+   *     FALSE.
    * @return SelectQuery
    *   Query object with all the tables, fields and conditions.
    */
   protected function dbStringSelect(array $conditions, array $options = array()) {
-    // Check the fields we are going to select and to which table they belong.
-    $fields = array();
-    if (isset($options['fields'])) {
-      foreach ($options['fields'] as $field) {
-        $fields[$this->dbFieldTable($field)][] = $field;
-      }
-    }
-    else {
-      if (!empty($options['source'])) {
-        $fields['s'] = array();
-      }
-      if (!empty($options['translation'])) {
-        // If we've got translation fields, we leave out the lid field to avoid clashes.
-        $fields['t'] = isset($fields['s']) ? array('language', 'translation', 'customized') : array();
-      }
-    }
-
-    // Start building the query with source table and fields and check whether
-    // we need to join the target table too.
-    $query = $this->connection->select('locales_source', 's');
+    // Start building the query with source table and check whether we need to
+    // join the target table too.
+    $query = $this->connection->select('locales_source', 's', $this->options)
+      ->fields('s');
 
     // Figure out how to join and translate some options into conditions.
     if (isset($conditions['translated'])) {
@@ -368,7 +346,7 @@ protected function dbStringSelect(array $conditions, array $options = array()) {
       unset($conditions['translated']);
     }
     else {
-      $join = isset($fields['t']) ? 'leftJoin' : FALSE;
+      $join = !empty($options['translation']) ? 'leftJoin' : FALSE;
     }
 
     if ($join) {
@@ -383,11 +361,12 @@ protected function dbStringSelect(array $conditions, array $options = array()) {
         // Since we don't have a language, join with locale id only.
         $query->$join('locales_target', 't', "t.lid = s.lid");
       }
+      if (!empty($options['translation'])) {
+        // We cannot just add all fields because 'lid' may get null values.
+        $query->fields('t', array('language', 'translation', 'customized'));
+      }
     }
-    // Add fields for both tables, it may be a query without fields.
-    foreach ($fields as $table_alias => $table_fields) {
-      $query->fields($table_alias, $table_fields);
-    }
+
     // Add conditions for both tables.
     foreach ($conditions as $field => $value) {
       $table_alias = $this->dbFieldTable($field);
@@ -447,8 +426,8 @@ protected function dbStringInsert($string) {
     if (($table = $this->dbStringTable($string)) && ($fields = $this->dbStringValues($string, $table))) {
       $this->dbStringDefaults($string, $table);
       return $this->connection->insert($table, $this->options)
-      ->fields($fields)
-      ->execute();
+        ->fields($fields)
+        ->execute();
     }
     else {
       throw new StringStorageException(format_string('The string cannot be saved: @string', array(

core/modules/locale/lib/Drupal/locale/StringInterface.php

@@ -90,6 +90,14 @@ public function getPlurals();
   */
   public function setPlurals($plurals);
 
+  /**
+   * Gets the string storage.
+   *
+   * @return Drupal\locale\StringStorageInterface
+   *   The storage used for this string.
+   */
+  public function getStorage();
+
   /**
    * Sets the string storage.
    *

core/modules/locale/lib/Drupal/locale/StringStorageInterface.php

@@ -28,7 +28,6 @@ interface StringStorageInterface {
    *   any of the following optional keys:
    *   - 'filters': Array of string filters indexed by field name.
    *   - 'pager limit': Use pager and set this limit value.
-   *   - 'fields', Array with the exact fields to load. Defaults to all.
    *
    * @return array
    *   Array of Drupal\locale\StringInterface objects matching the conditions.
@@ -61,14 +60,11 @@ public function getTranslations(array $conditions = array(), array $options = ar
    * @param array $conditions
    *   (optional) Array with conditions that will be used to filter the strings
    *   returned and may include all of the conditions defined by getStrings().
-   * @param array $options
-   *   (optional) An associative array of additional options. It may contain
-   *   any of the options defined by getStrings().
    *
    * @return Drupal\locale\SourceString|null
    *   Minimal TranslationString object if found, NULL otherwise.
    */
-  public function findString(array $conditions, array $options = array());
+  public function findString(array $conditions);
 
   /**
    * Loads a string translation object, fast query.
@@ -76,14 +72,11 @@ public function findString(array $conditions, array $options = array());
    * @param array $conditions
    *   (optional) Array with conditions that will be used to filter the strings
    *   returned and may include all of the conditions defined by getStrings().
-   * @param array $options
-   *   (optional) An associative array of additional options. It may contain
-   *   any of the options defined by getStrings().
    *
    * @return Drupal\locale\TranslationString|null
    *   Minimal TranslationString object if found, NULL otherwise.
    */
-  public function findTranslation(array $conditions, array $options = array());
+  public function findTranslation(array $conditions);
 
   /**
    * Checks whether the string version matches a given version, fix it if not.

core/modules/locale/lib/Drupal/locale/Tests/LocaleStringTest.php

@@ -74,7 +74,7 @@ function testStringCRUDAPI() {
     $this->assertEqual($source->version, 'none', 'String originally created without version.');
     $this->storage->checkVersion($source, VERSION);
     $string = $this->storage->findString(array('lid' => $source->lid));
-    $this->assertEqual($source->version, VERSION, 'Checked and updated string version to Drupal version.');
+    $this->assertEqual($string->version, VERSION, 'Checked and updated string version to Drupal version.');
 
     // Create translation and find it by lid and source.
     $langcode = 'es';
@@ -99,7 +99,7 @@ function testStringCRUDAPI() {
     $lid = $source->lid;
     $translations = $this->createAllTranslations($source);
     $search = $this->storage->getTranslations(array('lid' => $source->lid));
-    $this->assertEqual(count($search), 3 , 'Created and retrieved all translations for our source string.');
+    $this->assertEqual(count($search), 3, 'Created and retrieved all translations for our source string.');
 
     $source->delete();
     $string = $this->storage->findString(array('lid' => $lid));
@@ -123,11 +123,11 @@ function testStringSearchAPI() {
     $source3 = $this->buildSourceString()->save();
     // Load all source strings.
     $strings = $this->storage->getStrings(array());
-    $this->assertEqual(count($strings), 3  , 'Found 3 source strings in the database.');
+    $this->assertEqual(count($strings), 3, 'Found 3 source strings in the database.');
     // Load all source strings matching a given string
     $filter_options['filters'] = array('source' => $prefix);
     $strings = $this->storage->getStrings(array(), $filter_options);
-    $this->assertEqual(count($strings), 2  , 'Found 2 strings using some string filter.');
+    $this->assertEqual(count($strings), 2, 'Found 2 strings using some string filter.');
 
     // Not customized translations.
     $translate1 = $this->createAllTranslations($source1);
@@ -144,24 +144,24 @@ function testStringSearchAPI() {
 
     // Load all translations. For next queries we'll be loading only translated strings.    $only_translated = array('untranslated' => FALSE);
     $translations = $this->storage->getTranslations(array('translated' => TRUE));
-    $this->assertEqual(count($translations), 2 * $language_count  , 'Created and retrieved all translations for source strings.');
+    $this->assertEqual(count($translations), 2 * $language_count, 'Created and retrieved all translations for source strings.');
 
     // Load all customized translations.
     $translations = $this->storage->getTranslations(array('customized' => LOCALE_CUSTOMIZED, 'translated' => TRUE));
-    $this->assertEqual(count($translations), $language_count  , 'Retrieved all customized translations for source strings.');
+    $this->assertEqual(count($translations), $language_count, 'Retrieved all customized translations for source strings.');
 
     // Load all Spanish customized translations
     $translations = $this->storage->getTranslations(array('language' => 'es', 'customized' => LOCALE_CUSTOMIZED, 'translated' => TRUE));
-    $this->assertEqual(count($translations), 1  , 'Found only Spanish and customized translations.');
+    $this->assertEqual(count($translations), 1, 'Found only Spanish and customized translations.');
 
     // Load all source strings without translation (1).
     $translations = $this->storage->getStrings(array('translated' => FALSE));
-    $this->assertEqual(count($translations), 1  , 'Found 1 source string without translations.');
+    $this->assertEqual(count($translations), 1, 'Found 1 source string without translations.');
 
     // Load Spanish translations using string filter.
     $filter_options['filters'] = array('source' => $prefix);
     $translations = $this->storage->getTranslations(array('language' => 'es'), $filter_options);
-    $this->assertEqual(count($strings), 2  , 'Found 2 translations using some string filter.');
+    $this->assertEqual(count($strings), 2, 'Found 2 translations using some string filter.');
 
   }
 

core/modules/locale/locale.module

@@ -935,7 +935,6 @@ function _locale_rebuild_js($langcode = NULL) {
   // Construct the array for JavaScript translations.
   // Only add strings with a translation to the translations array.
   $options['filters']['location'] = '.js';
-  $options['fields'] = array('lid', 'context', 'source', 'translation');
   $conditions['language'] = $language->langcode;
   $translations = array();
   foreach (locale_storage()->getTranslations($conditions, $options) as $data) {

core/modules/locale/locale.pages.inc

@@ -406,6 +406,7 @@ function locale_translate_edit_form_submit($form, &$form_state) {
   foreach ($form_state['values']['strings'] as $lid => $translations) {
     // Get target string, that may be NULL if there's no translation.
     $target = locale_storage()->findTranslation(array('language' => $langcode, 'lid' => $lid));
+
     // No translation when all strings are empty.
     $has_translation = FALSE;
     foreach ($translations['translations'] as $string) {