Replace theme() with drupal_render() in locale module

We are working today with this issue during Code Sprint UA.

Needs some additional work because patch failed tests locally.
Will be work with this in few days.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -50,10 +50,15 @@ function locale_translate_import_form($form, &$form_state) {
+    '#description' => t('A Gettext Portable Object file.'),
+    '#upload_validators' => $validators
...
+++ b/core/modules/locale/locale.pages.incundefined
@@ -545,8 +549,12 @@ function locale_translation_status_form($form, &$form_state) {
+    '#theme' => 'locale_translation_last_check',
+    '#last' => $last_checked
+  );
...
+++ b/core/modules/locale/locale.pages.incundefined
@@ -725,13 +733,16 @@ function theme_locale_translate_edit_form_strings($variables) {
+    '#empty' => t('No strings available.'),
+    '#attributes' => array('class' => array('locale-translate-edit-table'))
+  );
...
+++ b/core/modules/locale/locale.pages.incundefined
@@ -759,7 +770,11 @@ function theme_locale_translation_update_info($variables) {
+      '#theme' => 'item_list',
+      '#items' => $releases
...
+++ b/core/modules/locale/locale.pages.incundefined
@@ -777,7 +792,11 @@ function theme_locale_translation_update_info($variables) {
+      '#theme' => 'item_list',
+      '#items' => $releases
+    );

https://drupal.org/coding-standards#array
Arrays should be ended with comma

Re-assigning to myself.

Re-rolling with trailing commas in render arrays.

I've removed the following three hunks, as they appear to have been fixed upstream.

@@ -545,8 +549,12 @@ function locale_translation_status_form($form, &$form_state) {
   }
 
   $last_checked = state()->get('locale.translation_last_checked');
+  $locale_translation_last_check = array(
+    '#theme' => 'locale_translation_last_check',
+    '#last' => $last_checked
+  );
   $form['last_checked'] = array(
-    '#markup' => '<p>' . theme('locale_translation_last_check', array('last' => $last_checked)) . '</p>',
+    '#markup' => '<p>' . drupal_render($locale_translation_last_check) . '</p>',
   );

@@ -759,7 +770,11 @@ function theme_locale_translation_update_info($variables) {
       }
     }
     $description = '<span class="text">' . t('Updates for: @modules', array('@modules' => implode(', ', $modules))) . '</span>';
-    $details = theme('item_list', array('items' => $releases));
+    $item_list = array(
+      '#theme' => 'item_list',
+      '#items' => $releases
+    );
+    $details = drupal_render($item_list);
   }
 
   // Build output for updates not found.
@@ -777,7 +792,11 @@ function theme_locale_translation_update_info($variables) {
     if ($details) {
       $details .= t('Missing translations for:');
     }
-    $details .= theme('item_list', array('items' => $releases));
+    $build = array(
+      '#theme' => 'item_list',
+      '#items' => $releases
+    );
+    $details .= drupal_render($build);
   }

Updating to needs review.

This is fixing related bugs, taking out arguments as it does not exist in hook_theme(). Also being explicit with the renderable array (#2006152: [meta] Don't call theme() directly anywhere outside drupal_render()).

I worked on this with @Carolyn at NYC Camp. Arguably we could move out the bug fixes to another issue as was done in #2009688: Replace theme() with drupal_render() in update system.

+++ b/core/modules/locale/locale.moduleundefined
@@ -253,7 +253,7 @@ function locale_theme() {
-      'arguments' => array('updates' => array(), 'not_found' => array()),
+      'variables' => array('updates' => array(), 'not_found' => array()),

This.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -771,14 +781,15 @@ function template_preprocess_locale_translation_update_info(&$variables) {
+    $releases = array();
     if ($variables['updates']) {
-      $releases = array();
       foreach ($variables['updates'] as $update) {
         $modules[] = $update['name'];
         $releases[] = t('@module (@date)', array('@module' => $update['name'], '@date' => format_date($update['timestamp'], 'html_date')));
       }
       $variables['modules'] = $modules;
     }

And this.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -51,10 +51,15 @@ function locale_translate_import_form($form, &$form_state) {
+  $file_upload_help = array(
+    '#theme' => 'file_upload_help',
+    '#description' => t('A Gettext Portable Object file.'),
+    '#upload_validators' => $validators,
+  );
   $form['file'] = array(
     '#type' => 'file',
     '#title' => t('Translation file'),
-    '#description' => theme('file_upload_help', array('description' => t('A Gettext Portable Object file.'), 'upload_validators' => $validators)),
+    '#description' => drupal_render($file_upload_help),

Let's not do this change here because we're doing #2030243: Remove theme_file_upload_help() from the theme system

quick update as per #14

This seems like it might be unrelated?

@@ -747,14 +757,15 @@ function template_preprocess_locale_translation_update_info(&$variables) {
 
   // Build output for available updates.
   if (isset($variables['updates'])) {
+    $releases = array();
     if ($variables['updates']) {
-      $releases = array();
       foreach ($variables['updates'] as $update) {
         $modules[] = $update['name'];
         $releases[] = t('@module (@date)', array('@module' => $update['name'], '@date' => format_date($update['timestamp'], 'html_date')));
       }
       $variables['modules'] = $modules;
     }
+
     $details['available_updates_list'] = array(
       '#theme' => 'item_list',
       '#items' => $releases,

@pplantinga, thanks! That change is to fix a PHP notice (undefined variable). It could be moved to another issue but it's a very small change. You can see in the context of the patch that $releases is used in $details['available_updates_list'] but might not be set.

+++ b/core/modules/locale/locale.pages.inc
@@ -520,15 +520,22 @@ function locale_translation_status_form($form, &$form_state) {
+

@@ -747,14 +757,15 @@ function template_preprocess_locale_translation_update_info(&$variables) {
+

However we could remove these extra blank lines added in the patch :)

…although looking at the whole function again, we might as well just set $releases = array() along with $details = array() at the very top, and remove both $releases = array() in the conditions below.

function template_preprocess_locale_translation_update_info(&$variables) {
  $details = array();

  // Build output for available updates.
  if (isset($variables['updates'])) {
    if ($variables['updates']) {
      $releases = array();
      foreach ($variables['updates'] as $update) {
        $modules[] = $update['name'];
        $releases[] = t('@module (@date)', array('@module' => $update['name'], '@date' => format_date($update['timestamp'], 'html_date')));
      }
      $variables['modules'] = $modules;
    }
    $details['available_updates_list'] = array(
      '#theme' => 'item_list',
      '#items' => $releases,
    );
  }

  // Build output for updates not found.
  if (isset($variables['not_found'])) {
    $variables['missing_updates_status'] = format_plural(count($variables['not_found']), 'Missing translations for one project', 'Missing translations for @count projects');
    if ($variables['not_found']) {
      $releases = array();
      foreach ($variables['not_found'] as $update) {
        $version = $update['version'] ? $update['version'] : t('no version');
        $releases[] = t('@module (@version).', array('@module' => $update['name'], '@version' => $version)) . ' ' . $update['info'];
      }
    }
    $details['missing_updates_list'] = array(
      '#theme' => 'item_list',
      '#items' => $releases,
    );
    // Prefix the missing updates list if there is an available updates lists
    // before it.
    if (!empty($details['available_updates_list']['#items'])) {
      $details['missing_updates_list']['#prefix'] = t('Missing translations for:');
    }
  }
  $variables['details'] = $details;
}

Like this :)

First patch shows the exceptions from not fixing the $releases = array() - so that does actually have to get fixed here.

Second patch + interdiff are the changes described in #16 and #17.

Looks good to me!

+++ b/core/modules/locale/locale.pages.incundefined
@@ -744,11 +753,11 @@ function theme_locale_translate_edit_form_strings($variables) {
 function template_preprocess_locale_translation_update_info(&$variables) {
...
+  $releases = array();
...
-      $releases = array();

@@ -765,7 +774,6 @@ function template_preprocess_locale_translation_update_info(&$variables) {
-      $releases = array();

This change does not seem correct - doesn't this mean the is we pass both the updates and not_found variable that the missing_updates_list item list will contain all the releases?

You're absolutely right, thanks @alexpott!

Applied patch from #21 and manually walked through adding new languages, configuring translatable interfaces, adding strings to translate, and verifying that translated strings show up in admin pages and front end of site. Worked as expected, no errors.

Committed 818d962 and pushed to 8.x. Thanks!