Convert locale.inc to dbtng.

We will need a test for the equivalent of #353886: Too many arguments.

Here is an extension of the .po import test to cover the bug uncovered in #353886: Too many arguments.

And the correct patch.

Visual review of #5:

- The first db_update() should have its fields array split across multiple lines.

-  if (!empty($form_state['values']['domain']) && $duplicate = db_fetch_object(db_query("SELECT language FROM {languages} WHERE domain = '%s' AND language <> '%s'", $form_state['values']['domain'], $form_state['values']['langcode']))) {
+
+  $duplicate = db_query("SELECT COUNT(language) FROM {languages} WHERE domain = :domain AND language <> :language", array(':domain' => $form_state['values']['domain'], ':language' => $form_state['values']['langcode']))->fetchField();
+  if (!empty($form_state['values']['domain']) && $duplicate) {

That snippet introduces a logic change, by moving the query that uses $form_state['values']['domain'] before the check to see if that value exists. I do not know this part of the code base well enough to say if that is a problem or not. That snippet appears twice, in fact, the second one for langcode.

-  db_query("UPDATE {languages} SET name = '%s', native = '%s', domain = '%s', prefix = '%s', direction = %d WHERE language = '%s'", $form_state['values']['name'], $form_state['values']['native'], $form_state['values']['domain'], $form_state['values']['prefix'], $form_state['values']['direction'], $form_state['values']['langcode']);
+  db_update('languages')
+    ->fields(array(
+               'name' => $form_state['values']['name'],
+               'native' => $form_state['values']['native'],
+               'domain' => $form_state['values']['domain'],
+               'prefix' => $form_state['values']['prefix'],
+               'direction' => $form_state['values']['direction'])
+             )
+    ->condition('language', $form_state['values']['langcode'])
+    ->execute();

The array lines should only be indented 2 spaces from the ->fields line, not all the way to 2 spaces after the array( keyword.

@@ -500,7 +524,7 @@ function locale_translate_overview_scree
   // Collect summaries of all source strings in all groups.
   $sums = db_query("SELECT COUNT(*) AS strings, textgroup FROM {locales_source} GROUP BY textgroup");
   $groupsums = array();
-  while ($group = db_fetch_object($sums)) {
+  foreach ($sums as $group) {
     $groupsums[$group->textgroup] = $group->strings;
   }

That whole block can now be condensed into:

$groupsums = db_query("SELECT textgroup, COUNT(*) as strings ...")->fetchAllKeyed();

-    $translation = db_result(db_query("SELECT translation FROM {locales_target} WHERE lid = %d AND language = '%s'", $lid, $key));
+    $translation = db_query("SELECT translation FROM {locales_target} WHERE lid = :lid AND language = :language", array(
+                              ':lid' => $lid,
+                              ':language' => $key))->fetchField();

Excessive indentation again. There are a couple of others, too, that I'll skip over for now.

-  db_query("INSERT INTO {languages} (language, name, native, direction, domain, prefix, enabled) VALUES ('%s', '%s', '%s', %d, '%s', '%s', %d)", $langcode, $name, $native, $direction, $domain, $prefix, $enabled);
+  db_insert('languages')
+    ->fields(array('language' => $langcode, 'name' => $name, 'native' => $native, 'direction' => $direction, 'domain' => $domain, 'prefix' => $prefix, 'enabled' => $enabled))
+    ->execute();

That definitely needs to broken out to multiple lines.

-  $result = db_query("SELECT s.lid, s.source, t.plid, t.plural, t.translation
-    FROM {locales_source} s
-      LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language
-    WHERE s.location LIKE '%.js%'
-      AND s.textgroup = 'default'
-    ORDER BY t.plural DESC", array(':language' => $language->language));
+  $result = db_query("SELECT s.lid, s.source, t.plid, t.plural, t.translation FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.location LIKE '%.js%' AND s.textgroup = :textgroup ORDER BY t.plural DESC", array(':language' => $language->language, ':textgroup' => 'default'));

Let's leave the query string itself broken across multiple lines. It's FAR easier to read that way.

Here we are. This depends on #343999: No obvious way to do "IS (NOT) NULL" in a built query?.

Now that the dependent patch is in, let's retry.

There are a few spots where things should be broken up onto separate lines. The basic rule is that if a query has more than 1 replacement, it should be broken up onto multiple lines. If it's in a conditional statement, it should be moved outside the conditional for clarity. This occurs in at least a few spots.

You should also be able to simplify the following because db_insert should return the lid value (saving you a query in this case)

      db_insert('locales_source')
        ->fields(array('location' => $location, 'source' => $source, 'textgroup' => $textgroup))
        ->execute();
      $lid = db_query("SELECT lid FROM {locales_source} WHERE source = :source AND textgroup = :textgroup", array(
                        ':source' => $source, ':textgroup' => $textgroup))

Those are the issues that jump out at me on first glance. I'll try and take a deeper look if time permits.

There are a few spots where things should be broken up onto separate lines. The basic rule is that if a query has more than 1 replacement, it should be broken up onto multiple lines. If it's in a conditional statement, it should be moved outside the conditional for clarity. This occurs in at least a few spots.

There is no such code style rule. On the contrary, we always put all parameters in the same line.

You should also be able to simplify the following because db_insert should return the lid value (saving you a query in this case)

Good catch. I have no idea why it was so crappy in D6 ;)

Here is a reroll of the patch.

There is no such code style rule. On the contrary, we always put all parameters in the same line.

On the contrary, we have been breaking the associative array out into multiple lines, the same as most associative arrays. We're doing that all over core at this point, and it's much easier to read than a 200 character long line. :-)

On the contrary, we have been breaking the associative array out into multiple lines, the same as most associative arrays. We're doing that all over core at this point, and it's much easier to read than a 200 character long line. :-)

Since when? Where is this documented?

http://groups.drupal.org/node/17906 - not really documentation but afaik that's the best we've got.

No query has more than 2 replacements. I believe this is perfectly correct formatting.

Looks fine to me as well.

I'm fine with whatever standard we want to set as long as it is a standard. Right now, it looks like we're being heavily inconsistent with these patches for core. Here we're doing multiple replacements inline, in the book module, we're breaking out single replacements, and in comment module we're breaking out multiple replacements into multiple lines. We can move this discussion for sure, but we should define and follow a standard moving forward so we all know what we should do and it doesn't devolve into a battle of wills.

Committed to CVS HEAD. Thanks!

csevb10 - agreed, that groups discussion has gone a bit dead, should probably revive it.

Oups. One call to fields() was named field(), and the test bot was unable to catch that, because it still can't properly catch fatal errors in the parent side (I think we have an issue for that, but I can't find it anymore).

Looks good, nice catch!

this is breaking any attempt to run tests...

Thanks, committed!