Introduce a generic API for interface translation strings

Tagging for interface translation.

First, great abstraction, I like the string => translation | source abstraction. I also like the lots of custom code removed :) This has been an old issue and lots of work went into #361597: CRUD API for locale source and locale target strings, so it would be great to have a solid solution for this. I have mostly minor remarks:

+++ b/core/modules/locale/lib/Drupal/locale/LocaleSource.phpundefined
@@ -0,0 +1,91 @@
+    $result =$query->execute();

Whitespace after =

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,128 @@
+  /**
+   * The locale ID.
+   *
+   * @var integer
+   */
+  public $lid;

"The string identifier" instead? :)

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,128 @@
+}
\ No newline at end of file

No newline.

+++ b/core/modules/locale/lib/Drupal/locale/PoDatabaseReader.phpundefined
@@ -109,47 +111,47 @@ class PoDatabaseReader implements PoReaderInterface {
+    // User our query builder with fields and conditions. The option 'not_translated'
+    // will be handled by the query builder itself.
+    $query = LocaleTranslation::buildQuery($fields, $conditions, $options);

First comment line too long.

Also:
- Are you not applying this to the locale() lookup due to the speed required there?
- Do we have an idea of how much slower does this make importing a big .po file for example?
- Do you think we can have tests for the new classes (without the whole Drupal system being tested like we do now with the .po imports/exports and edits)?

Want to post a patch at #1789090: Warnings when exporting templates: addcslashes() expects parameter 1 to be string as well for that bug specifically or mark it a duplicate in favor of this bigger improvement?

Below are the notes that I made while reviewing this patch. I am unable to roll a patch at this time so I am unable to re-roll a patch that incorporates these comments. Hopefully, someone else will be able to.

Aside from the comments below, consistently throughout this code the one line descriptions need to be changed to active tense (e.g 'Gets' instead of 'Get').

+++ b/core/modules/locale/lib/Drupal/locale/LocaleSource.phpundefined
@@ -0,0 +1,105 @@
+  /**
+   * Load by locale id.
+   */
+  public static function loadById($lid) {

Perhaps 'Loads strings by locale ID.' Also this docblock needs type hinted @param and @return directives.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleSource.phpundefined
@@ -0,0 +1,105 @@
+  /**
+   * Load by context, source.
+   */
+  public static function loadBySource($source, $context = '') {

Perhaps 'Loads strings by source and optional context.' Needs @param and @return directives too.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+   * @param $values
+   *   Object or array with initial values.

Can we add type hint 'array|object'?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Set an array of values as object properties ignoring null values.

Needs to be active verb.. 'Sets'

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+   * @param array $values
+   *   Array with values indexed by property name.

Missing type hinted @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Set an array of values as object properties if not set before.

Ibid. Occurs elsewhere in patch too.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Get field values excluding keys.

Missing @param and @return directives.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Check whether this string version matches a given version, fix it if not.
+   *
+   * @param string $version
+   *   Drupal version to check against.

s/Check/Checks/ and needs @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Load multiple string objects, fast query.
+   *
+   * In order to produce a query as fast as possible we must
+   * pass the exact fields we need to load only that ones.
+   *
+   * @param $conditions
+   *   Array with simple field conditions.
+   * @param $fields
+   *   Fields to load, defaults to all.
+   * @param $options
+   *   Array of options used for building the query and one aditional option:
+   *   - 'fetch class', Optional class to fetch the results into.

Should be rewrapped for 80 characters. Add type hinting to each @param. Add '(optional)' to start of descriptions for $fields and $options. Add missing @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+   * @param $conditions
+   *   Array with field => value conditions that may include NULL values. If a
+   *   language condition is included we use it for the join.
+   * @param $fields
+   *   Array with the fields to select. By default we select all source fields
+   *   but none of the target fields. See 'target fields' and 'source fields'
+   *   options.
+   * @param $options
+   *   Array of options that may contain the following values:
+   *   - 'translated': Whether to include translated strings, defaults to TRUE.

Please add type hinting and '(optional)' at start of descriptions of $fields and $options.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Get table alias for field.

Missing @param and @return directives.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Save string object to database.
+   */
+  public function save() {

Missing @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Split string to work with plural values.
+   */
+  public function getPlurals() {
+    return explode(LOCALE_PLURAL_DELIMITER, $this->getString());

Missing @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,348 @@
+  /**
+   * Set this string using array of plural values.
+   *
+   * Serialize plural variants in one string by LOCALE_PLURAL_DELIMITER.
+   */
+  public function setPlurals($plurals) {
+    $this->setString(implode(LOCALE_PLURAL_DELIMITER, $plurals));

Missing @param and @return directives.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,184 @@
+  /**
+   * Boolean indicating whether this string is customized.
+   */
+  public $customized;

Missing @var directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,184 @@
+  /**
+   * Boolean indicating whether the target object exists.
+   */
+  protected $is_new = TRUE;

Ibid.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,184 @@
+   *
+   * @return
+   */
+  public static function loadBySource($langcode, $source, $context = '', $fields = array()) {
+    $conditions = array('language' => $langcode, 'source' => $source, 'context' => $context);

Can we add @return type hint here?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,184 @@
+   * @param array $fields
+   *   Array containing the name of the fields to load, defaults to all fields.
+   */
+  public static function loadById($langcode, $lid, $fields = array()) {
+    $conditions = array('language' => $langcode, 'lid' => $lid);

Missing @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,184 @@
+   * @param $is_new
+   *   True if the string is new, false otherwise.

Perhaps rewrite as '@param bool $is_new' and '(optional) TRUE if the string is new; otherwise, FALSE. Defaults to TRUE.'

Also needs @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,184 @@
+   * @param $customized
+   *   Boolean that determines whether the string is new

Needs @return directive and type hint as well as all descriptions should end in a period.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleStringTest.phpundefined
@@ -0,0 +1,185 @@
+use Drupal\simpletest\WebTestBase;
+use Drupal\locale\LocaleSource;
+use Drupal\locale\LocaleTranslation;
+use Drupal\Core\Language\Language;

Not sure about the ordering here... does not seem 'sane' according to #1624564: Coding standards for "use" statements.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleStringTest.phpundefined
@@ -0,0 +1,185 @@
+  /**
+   * A user able to create languages and export translations.
+   */
+  protected $admin_user = NULL;

Missing @var directive.

Here are my comments from reviewing the patch in #15. In general, this looks much better than before. Hopefully, these can be incorporated in the next re-roll.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleSource.phpundefined
@@ -0,0 +1,122 @@
+   *
+   * @return Drupal\locale\LocaleSource
+   *   LocaleSource object if found, NULL otherwise.

I think this should be 'Drupal\locale\LocaleSource|null'.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleSource.phpundefined
@@ -0,0 +1,122 @@
+   * @param string $context
+   *   (defaults to the empty context) String context.
+   *
+   * @return Drupal\locale\LocaleSource
+   *   LocaleSource object if found, NULL otherwise.

Should start with '(optional)' and then include what happens in the default case.

@return needs '|null' appended.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+  /**
+   * Sets an array of values as object properties ignoring null values.
+   *
+   * @param array $values
+   *   Array with values indexed by property name.
+   */
+  public function setValues(array $values = array()) {
+    foreach ($values as $key => $value) {

Missing @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+  /**
+   * Sets an array of values as object properties if not set before.
+   *
+   * @param array $values
+   *   Array with values indexed by property name.
+   */
+  public function setDefaultValues(array $values = array()) {
+    foreach ($values as $key => $value) {

Ibid.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+  /**
+   * Gets field values that are set for given field names.
+   *
+   * @param array $field_names
+   *   Array of field names.
+   */
+  public function getFieldValues(array $field_names) {
+    $values = array();

Ibid.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+  /**
+   * Checks whether this string version matches a given version, fix it if not.
+   *
+   * @param string $version
+   *   Drupal version to check against.
+   */
+  public function checkVersion($version) {
+    if (isset($this->lid) && isset($this->version) && $this->version != $version) {

Ibid.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+   * @param array $conditions
+   *   (optional) Array with simple field conditions.

What happens in default condition?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+   * @param array $options
+   *   (optional) An associative array of additional options, with the

Ibid.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+   *   - Additional $options elements used by the getQuery() method.
+   * @return array
+   *   Array of string objects matching the conditions.

Needs a blank line before @return.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+   *   (optional) An associative array of additional options, with the
+   *   following elements:
+   *   - 'translated' (defaults to TRUE): Whether to include translated

Suggest 'with any of the following optional keys:'.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+   * @param string $field_name
+   *
+   * @return string

Missing @param description.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+   *
+   * @return bool

Missing @return description.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+  /**
+   * Saves string object to database.
+   */
+  public function save() {
+    if ($this->isNew()) {

Missing @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,371 @@
+   * @param array $plurals
+   *   Array of strings with plural variants.
+   */
+  public function setPlurals($plurals) {
+    $this->setString(implode(LOCALE_PLURAL_DELIMITER, $plurals));

Missing @return directive.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,207 @@
+   * @param string $context
+   *   (defaults to the empty context) String context.
+   * @param array $fields

Should start with '(optional) '.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,207 @@
+   * Load translation by string identifier, fast query.

s/Load/Loads/

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,207 @@
+   * Overrides Drupal\locale\LocaleString::loadMultiple().
+   *
+   * @return array
+   *   Array of Drupal\locale\LocaleTranslation objects.

I don't think this @return is needed.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,207 @@
+   * Marks the string as new / not new.
+   *
+   * @param bool $is_new
+   *   TRUE if the string is new, FALSE otherwise.

Missing @return directive. Also needs to start with '(optional)'.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,207 @@
+   * Sets the string as customized / not customized.
+   *
+   * @param bool $customized
+   *   Sets the string as customized if TRUE, not customized otherwise.

Ibid.

+++ b/core/modules/locale/lib/Drupal/locale/PoDatabaseWriter.phpundefined
@@ -240,66 +237,44 @@ class PoDatabaseWriter implements PoWriterInterface {
+        ))
+        ->save();

This looks strange. Perhaps '$target->save()'?

+++ b/core/modules/locale/locale.pages.incundefined
@@ -18,42 +20,38 @@ function locale_translate_page() {
 /**
- * Build a string search query.
+ * Build a string search query and return array of string objects.

s/Build/Builds/. also needs @return directive.

I think the latest patch looks like a good general improvement, and would indeed unblock further work on #1777070: Refactor and clean up source string location handling which is a major piece. So Jose, if you have time to do the cleanups suggested by Lars, I don't see any other missing pieces that we'd need to deal with anymore. The patch looks pretty good :)

Ha, that is pretty unfortunate, although I kind of expected it :/ While doing these changes, can you also add a couple line of comments above where the direct queries are done to something like:

// These queries run as part of a critical path to look up translations from t() and
// benchmarks found using db_select() would be significantly slower to build the
// query, so using a direct db_query() instead.

Something like that (wrapped to 80 chars properly :).

Jose, there's issue explaining db_select performance #1067802: Compare $connection->query() and $connection->select() performance
Also there's a comment in DrupalDatabaseCache::getMultimple()

I think it's abd idea to allow hook_execution in t() it could be used in early bootstrap and probably could lead to unpredictable results

Thanks for brining the performance of this up to what was there before. I think this is a great cleanup and would serve as well in Drupal 8. It also serves as a base API for #1777070: Refactor and clean up source string location handling which is a major task and targeted at the D8 feature freeze.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleSource.phpundefined
@@ -0,0 +1,143 @@
+  /**
+   * Implements Drupal\locale\LocaleString::delete().
+   */
+  public function delete() {
+    if (!$this->isNew()) {
+      db_delete('locales_target')
+        ->condition('lid', $this->lid)
+        ->execute();
+      db_delete('locales_source')
+        ->condition('lid', $this->lid)
+        ->execute();
+      unset($this->lid);
+    }
+    return $this;

The database connection should be injected here, rather than using the procedural helpers.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,358 @@
+
+/**
+ * Defines the locale string class.
+ *
+ * This is the base class for LocaleSource and LocaleTranslation.
+ */
+abstract class LocaleString {
+  /**
+   * The string identifier.
+   *
+   * @var integer
+   */

If it's a base class it should have a 'Base' suffix - so LocaleStringBase.

There's no interface here at all but this looks like a public API that people will be using. Why not?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleString.phpundefined
@@ -0,0 +1,358 @@
+   * @return SelectQuery
+   *   Query object with all the tables, fields and conditions.
+   */

The static functions make this harder to dependency inject. I assume the reason they're static is because they have nothing to do with the state in this class (string IDs etc.). With entities we've handled this by having storage controllers separate from the classed object itself - why not the same pattern here? At the moment this feels like it's doing too many things at once in the same class.

+++ b/core/modules/locale/lib/Drupal/locale/LocaleTranslation.phpundefined
@@ -0,0 +1,246 @@
+  public static function findTranslation($langcode, $source, $context, $fields) {

Same with this one. This also has the same issue that the database isn't injected.

Since we cache locale strings, I'm fine for the change to the select query builder from a performance standpoint, but thanks very, very much for actually checking the difference :)

@catch: thanks for the review. To put this issue into perspective, we bumped into needing to touch locale tables in #1777070: Refactor and clean up source string location handling and Jose decided to finally tackle a more general API for locale string management instead of just copy-pasting a couple specific locale queries to the new place scattering that even more. I don't think we have a vested interest per-say in introducing this now and it does not give us any features we don't have now, it merely cleans up code. So if you prefer the current one-off queries and do not consider the patch an improvement, then we can go and postpone this to after December 1st (or Drupal 9) and just keep doing one-off direct locale table queries to speed up the actual features we are working on. Those are subject to the December 1st cut-off, so instead of perfecting this spin-off with storage controllers and dependency injection, we can focus on the real problems we are trying to solve.

I don't think we need to tackle dependency injection here, that's not properly happening in core yet anywhere but people are starting to push for it when new stuff goes in so I brought it up.

But splitting the class into storage vs. data feels necessary prior to commit and should not be much work either.

Also indicating you'll drop work on refactoring patches every time I push back doesn't incline me to want to bother reviewing things in a timely fashion at all, nor to commit new features if they're going to introduce maintenance overhead.

Fact is, there are 5 months of refactoring planned into the release timeline and 9 weeks before that in which we can still introduce new features, so I'm focusing on these 9 weeks, not those 5 months at this point.

All right, discussed this with @catch on IRC, since #1777070: Refactor and clean up source string location handling only touches 2 locale queries and adds 2 new ones (and it is a new feature vs. this patch), it would make sense to get that done instead and have this be done (possibly even after December 1st). Moving off of the sprint for that. Let's return to our real feature at #1777070: Refactor and clean up source string location handling instead of this side-track unless @Jose Reyero wants to rework this ASAP.

(Moving off of the sprint for that).

I agree with catch that for normal priority feature requests that aren't blockers for other issues, patches need to incorporate legitimate feedback before being committed. I also agree with Gabor that Jose and other D8MI leaders/experts have higher priority things to work on before feature freeze. That does create an unfortunate situation that good (but not fully refined) patches can sit in limbo for a while or not make it in to D8 at all. To combat this, I'd like to try out an "Across the finish line" tag (thanks to webchick for coming up with that name). I'm hoping that there are existing or new core contributors out there who want to help get cool improvements like this in, but who don't yet have the expertise (or time) to work on issues like this from scratch, and who want to build up their expertise by helping to finish these kinds of issues. Note that this is different from the Novice tag, as these kinds of issues may require more Drupal experience or time than what we typically use the Novice tag for.

Please help spread the word about this tag, and apply it to other similar issues.

Restoring tag.

Restoring correct tag :)

So the current class/interface hierarchy is:

StringStorageInterface ---- implemented by ----> StringDatabaseStorage
StringInterface ---- implemented by ----> StringBase ---- extended by ----> both LocaleSource and LocaleTranslation

This is quite a few levels of abstraction added compared to the direct queries we had before the patch, but it certainly provides for more flexibility and looks logical as-is.

Given that all of these are in the locale namespace anyway, would it make sense to rename LocaleSource to SourceString and LocaleTranslation to TranslationString? Or just Source and Translation? They do not have any naming relation to the string interfaces / base classes they extend anyway.

Plus one minor thing, noticed that the StringBase file level comment says "Definition of Drupal\locale\LocaleSource". Should be fixed.

Took care of concerns from catch, looks good to me.

Putting back on sprint merely as a tracking helper. Also, for some reason lost the language-ui tag.

Assigning this to catch as he did an in-depth review in #30. I'll leave it to him to commit (if ready). Thanks!

I think this has just been broken by cache prefix clears removal (because the parameters to __construct() for CacheArray changed), sending for re-test.

#44: locale_string_object-1785086-43.patch queued for re-testing.

OK a few more comments. I saw several issues with documentation when reading through again, but I left those for now.

I'm not sure why the patch continues to pass despite the __construct() arguments being incompatible.

+++ b/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.phpundefined
@@ -0,0 +1,467 @@
+    $args = array();
+    $values = $conditions;
+    // Prefix fields with table alias and eliminate duplicates.
+    foreach ($fields as $index => $field) {
+      $fields[$index] = $this->getFieldTableAlias($field) . '.' . $field;
+    }
+    $sql = 'SELECT ' . implode(', ', $fields) . ' FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid';
+    if (isset($conditions['language'])) {
+      $sql .= ' AND t.language = :langcode';
+      $args[':langcode'] = $conditions['language'];
+      unset($conditions['language']);
+    }
+    if ($conditions) {
+      foreach ($conditions as $field => $value) {
+        $conditions[$field] = $this->getFieldTableAlias($field) . '.' . $field . ' = :' . $field;
+        $args[':' . $field] =  $value;
+      }
+      $sql .= ' WHERE ' . implode(' AND ', $conditions);
+    }
+    $string = $this->connection->query($sql, $args, $this->options)
+      ->fetchObject('Drupal\locale\TranslationString');
+    if ($string) {
+      // Since we don't load all values, fill from conditions and set storage.
+      $string->setValues($values + array('storage' => $this), FALSE);
+    }
+    return $string;

Why isn't this using db_select()?

+++ b/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.phpundefined
@@ -0,0 +1,467 @@
+  /**
+   * Implements Drupal\locale\StringStorageInterface::countStrings().
+   */
+  public function countStrings() {
+    return $this->selectQuery('locales_source')
+      ->countQuery()
+      ->execute()
+      ->fetchField();

This one could just be a straight db_query() though since it's not dynamic at all.

+++ b/core/modules/locale/lib/Drupal/locale/StringDatabaseStorage.phpundefined
@@ -0,0 +1,467 @@
+  /**
+   * Implements Drupal\locale\StringStorageInterface::countTranslations().
+   */
+  public function countTranslations() {
+    $query = $this->selectQuery('locales_target');
+    $query->addExpression('language, COUNT(lid)', 'translations');
+    $query->groupBy('language');
+    return $query->execute()->fetchAllKeyed();
+  }

Same question here.

+++ b/core/modules/locale/lib/Drupal/locale/StringStorageInterface.phpundefined
@@ -0,0 +1,182 @@
+  /**
+   * Loads multiple source string objects.
+   *
+   * In order to produce a query as fast as possible we must pass the exact
+   * fields we need to load only that ones.
+   *
+   * @param array $conditions
+   *   (optional) Array with simple field conditions. Defaults to no conditions

Why is it performance critical how many fields are loaded? Since strings are cached anyway it feels like it's probably not, but would be good to know why this optimization is here if that's not the case.

I think usage of db_select is required only in places where query actually requires to be altered, all other usage of db_select is just a performance overhead and dependency on hook subsystem

Resolved or explained reasons for all of @catch's concerns. Should be back there then.

That produces the performance critical query mentioned above (#20), comments about that are in the interface definition.
Similar queries happen to be used too from other parts of the module (js translation, po imports) with different parameters, that's why we are using an ad-hoc query builder here.

Sorry I don't really think this is performance critical, the results are cached and it's not a slow query (assuming it hasn't changed when converted to the query build), just PHP overhead from db_select(). We should try to optimize db_select() but working around it should only be used in extreme cases (like the database cache backend where it completely destroyed cached page performance).

I'm wondering a bit if we could optimize some of the query building a bit - i.e. create a stub select query object then fill it in with the specifics each call to that method, would that help at all? If it's OK let's put the db_select() back in here than open a follow-up to try to either optimize db_select() or see if there's a way to avoid duplicating some of the specific query building.

Adding/changing translations should be quite rare, especially if they're done in a staging environment then imported. Additionally if we wanted to optimize translation cache clears we could potentially write through to the cache object with the updated translation rather than clearing the whole lot.

With only selecting some of the fields, I was asking why it's performance critical that only some fields are selected - again when the results of the most performance-critical query are cached anyway. Have any benchmarks been done on whether excluding fields actually affects performance measurably here? Right now it's introducing inconsistency in return results (i.e. "the results of this may not be suitable for CRUD operation" with no tangible benefit. We might even be able to save some time in the query builder by just doing a SELECT * instead of individually adding fields, should be less function calls at least.

The changes from ->select() to ->query() the other way look great, thanks for those.

So it needs benchmarks?
As I pointer above - there's no way to make db_select() none-alterable, so it makes whole translation system dependent on full bootstrap and hook sub-system

@Jose I think I had the original change the wrong way 'round. I thought the patch had changed from building the query with concatenation to using the query builder, because despite being slower the strings are cached anyway. Sorry for any misunderstanding but despite the query builder being slower, we absolutely have to use that for dynamic queries.

seen the translators work right on the live site (just because we don't have a workflow to really stage translation updates other than manual export / import).
So basically we are facing the scenario of the site being way slower the week the translation team is reviewing the translations (yes, believe it or not, translations being updated on the live site every few minutes).

That's a poor workflow on the site then, or we need to find a way to flush translation caches less often like writing back to them.

@catch: that sounds like you suggest we take the performance regression here in favor of better looking code then.

Yes, we have db_select() and that's mandated for dynamic queries. We have one or two highly commented exceptions to this where there's an otherwise static query with a dynamic table name (i.e. in the dbtng cache backend), because it's right in the critical path even on cached pages and was such a bad performance regression without it. However actual dynamic query building there's really no option - we might as well rip out db_select() altogether if we're going to start doing that. Also this query isn't in the critical path at all - it only runs (albeit loads of times) on a cache miss.

I'd be interested in:

- does the db_select() overhead reduce if the field optimization is taken out.
- it sounds like Jose tried the stub query and that helped a bit.

Also this issue reminds me a bit of sun's attempt to rewrite t() - if t() instantiated an object with the original string, then didn't try to get any translations until a __toString() method was called, then potentially we could try to query all the translations on a page at once the first time that happens. That's not strictly related to this issue but in short there's potentially other ways to optimize this rather than avoiding db_select().

One more thing with the performance issue and db_select() again. Apologies for not seeing all these things at once...

Here's all the usage of findTranslation I could find:

+    $translation = $this->stringStorage->findTranslation(array(
+      'language' => $this->langcode,
+      'source' => $offset,
+      'context' => $this->context
+    ),

+    $string = locale_storage()->findTranslation(array(
+      'language' => $this->_langcode,
+      'source' => $source,
+      'context' => $context
+    ), array('customized'));

+    $string1 = $this->storage->findTranslation(array('language' => $langcode, 'lid' => $source->lid));

+    $string2 = $this->storage->findTranslation(array('language' => $langcode, 'source' => $source->source, 'context' => $source->context));

I don't see any problem with adding an optimized method to the interface (i..e for returning a translated string given a source string, language and context), and using db_query() for that, then keeping the query builder for rarer operations where it saves code duplication. We have similar helpers for entities like user_load_by_name() or whatever. Would that help here?

Had a hard time reviewing the changes since no interdiff was posted, however there are no db_query()'s left at all in the code (only in the removed hunks :), and the remaining feedback was also taken care of as much as I can tell. We could use the improvements here in the major CMI multilingual tasks and time is pressing for us to get these improvements in, so if there are no more issues with the patch, getting it land would be great!

Thanks all!

Gabor asked me to take a look at this again in irc. I don't see anything pressing at first glance - looks like most or all of my concerns were addressed, but don't have enough time at the minute to do a solid review prior to commit.

Unassigning me since anyone can commit at this point, I'll try to get this in next time I'm doing a commit spree if no-one beats me to it.

I had a look through this patch with Jose and Gábor on IRC. In general this seems to clean up quite a bit of stuff and make the result much, much easier to read.

Here is some feedback from my review. The only one that needs to be resolved pre-commit is the docs for SourceString and TranslationString, but Jose and I are working on that in IRC right now. The rest are small nitpicks and could be a novice follow-up.

+++ b/core/modules/locale/lib/Drupal/locale/SourceString.phpundefined
@@ -0,0 +1,53 @@
+/**
+ * Defines the locale source string object.
+ */
+class SourceString extends StringBase {

+++ b/core/modules/locale/lib/Drupal/locale/TranslationString.phpundefined
@@ -0,0 +1,123 @@
+/**
+ * Defines the locale translation string object.
+ */
+class TranslationString extends StringBase {

These are the two biggest new vocabulary words added with this patch; it'd be really nice to see more explanation here as to what they are, how they relate, how to know if my module needs to work with them.

+++ b/core/modules/locale/lib/Drupal/locale/StringBase.phpundefined
@@ -0,0 +1,179 @@
+/**
+ * Defines the locale string base class.
+ */
+abstract class StringBase implements StringInterface {

This one too. Why else might we have to implement this class?

+++ b/core/modules/locale/lib/Drupal/locale/StringBase.phpundefined
@@ -0,0 +1,179 @@
+  /**
+   * Implements Drupal\locale\StringInterface::setStorage().
+   */
+  public function setStorage($storage) {
+    $this->storage = $storage;
+    return $this;
+  }

(nitpick) Can we move this below getStorage()? I almost missed it in my review.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleStringTest.phpundefined
@@ -0,0 +1,199 @@
+    $this->assertEqual(count($search), 3 , 'Created and retrieved all translations for our source string.');
...
+    $this->assertEqual(count($strings), 3  , 'Found 3 source strings in the database.');
...
+    $this->assertEqual(count($strings), 2  , 'Found 2 strings using some string filter.');
...
+    $this->assertEqual(count($translations), 2 * $language_count  , 'Created and retrieved all translations for source strings.');
...
+    $this->assertEqual(count($translations), $language_count  , 'Retrieved all customized translations for source strings.');
...
+    $this->assertEqual(count($translations), 1  , 'Found only Spanish and customized translations.');
...
+    $this->assertEqual(count($translations), 1  , 'Found 1 source string without translations.');
...
+    $this->assertEqual(count($strings), 2  , 'Found 2 translations using some string filter.');

(nitpick) Can we remove those two spaces after the count?

+++ b/core/modules/locale/locale.moduleundefined
@@ -288,6 +289,20 @@ function locale_reset() {
+function locale_storage() {
+  $storage = &drupal_static(__FUNCTION__);
+  if (!isset($storage)) {
+    $options = array('target' => 'default');
+    $storage = new StringDatabaseStorage(Database::getConnection($options['target']), $options);
+  }
+  return $storage;

We talked a bit about whether 'target' should be passed in to locale_storage() as a parameter, but in the end decided that if someone needs a non-default target, they could create their own implementation. I was confused about how said implementation even gets *into* this function, given that there is no $storage_class parameter (or what have you), but Jose said it happens via the DIC. I'm not totally up to speed on that stuff, so I'm going to assume this is true and move on. :D

Ok, Jose and I worked on the docs for SourceString/TranslatedString on IRC, and came up with the following:

/**
 * Defines the locale source string object.
 *
 * This class represents a module-defined string value that is to be translated.
 * This string must at least contain a 'source' field, which is the raw source
 * value, and is assumed to be in English language.
 */

...

/**
 * Defines the locale translation string object.
 *
 * This class represents a translation of a source string to a given language,
 * thus it must have at least a 'language' which is the language code and a
 * 'translation' property which is the translated text of the the source string 
 * in the specified language.
 */

These aren't perfect descriptions, but at least a bit more info so someone coming into this brand new will understand the difference between them.

With that, I Committed and pushed this to 8.x. Yay! The remaining feedback in #70 (the stuff prefixed with "nitpick") is Novice material, so tagging for that and re-categorizing accordingly. Since this is just internal Locale module refactoring, Jose didn't think we needed a change notice for this. I did ask him to start a follow-up about that 'target' stuff, though... we should be consistent about that in core.

+++ b/core/modules/locale/lib/Drupal/locale/StringBase.phpundefined
@@ -145,7 +155,14 @@ public function getValues(array $fields) {
+      throw new StringStorageException(format_string('The string cannot be saved because its not bound to an storage: @string', array(

@@ -154,26 +171,16 @@ public function save() {
+        throw new StringStorageException(format_string('The string cannot be deleted because its not bound to an storage: @string', array(

"its not bound to an storage" => "it is not bound to a storage"

Small nit if this gets re-rolled again. Otherwise, this looks good from a documentation perspective.

+++ b/core/modules/locale/lib/Drupal/locale/StringBase.phpundefined
@@ -145,7 +155,14 @@ public function getValues(array $fields) {
    * Implements Drupal\locale\LocaleString::save().
    */
   public function save() {
...
+    else {
+      throw new StringStorageException(format_string('The string cannot be saved because its not bound to a storage: @string', array(
+        '@string' => $string->getString()
+      )));

I believe that this method needs to document the exception possibility with a @throws directive.

Great cleanup, even including whitespace fixes in the tests :) Thanks!

Awesome, thanks!

Committed and pushed to 8.x.

Restoring issue properties.

Great, thanks all!