Replace theme() with drupal_render() in field and field_ui modules

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

Patch attached.

In FieldPluginBase.php

This file belongs to the Views module, shouldn't it be replaced in the separate issue?

#1828536: Rename 'type' variable of theme_item_list() to 'list_type' is in so maybe we can work on this again.

…and we can start with a reroll.

From scratch.

Thanks @pplantinga, that's looking pretty good I'd say! Found a couple small tweaks.

+++ b/core/modules/field/field.form.incundefined
@@ -67,8 +71,17 @@ function theme_field_multiple_value_form($variables) {
+        'class' => array('field-multiple-table')

Needs a trailing comma per http://drupal.org/coding-standards#array.

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -355,19 +355,21 @@ function field_ui_view_mode_delete(EntityViewModeInterface $view_mode) {
+  $table = array(
+    '#theme' => 'table'
+  );

Same thing here, but I would probably collapse this into one line in which case the trailing comma should not be added.

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -355,19 +355,21 @@ function field_ui_view_mode_delete(EntityViewModeInterface $view_mode) {
-  foreach (array('header', 'attributes') as $key) {
-    if (isset($elements["#$key"])) {
-      $table[$key] = $elements["#$key"];
+  foreach (array('#header', '#attributes') as $key) {
+    if (isset($elements[$key])) {
+      $table[$key] = $elements[$key];
     }
   }

Nice :) doesn't need tweaking I just like the simplification here.

Cool, thanks!

Here's the updated patch.

#12: field-convert-theme-2009008-12.patch queued for re-testing.

#12 looks good.

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

+++ b/core/modules/field/field.form.incundefined
@@ -25,9 +25,13 @@ function theme_field_multiple_value_form($variables) {
+    $form_required_marker = array(
+      '#theme' => 'form_required_marker',
+      '#element' => $variables['element'],
+    );
+    $required = !empty($element['#required']) ? drupal_render($form_required_marker) : '';
     $table_id = drupal_html_id($element['#field_name'] . '_values');
     $order_class = $element['#field_name'] . '-delta-order';
-    $required = !empty($element['#required']) ? theme('form_required_marker', $variables) : '';

It looks like we passing in something different... $variables['element'] and not $variables... why? This appears to be totally unused in theme_form_required_marker()

updated with removed element key.

I agree with #17. Since theme_form_required_marker() ignores its argument, the original version of theme_field_multiple_value_form() might as well call theme('form_required_marker', array()) and then according to the instructions in the meta-issue #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() we would convert this to

    $form_required_marker = array(
      '#theme' => 'form_required_marker',
    );

(eliminating the '#element' key) and then use drupal_render($form_required_marker)

I reviewed the whole patch in #12 and made the change I suggested in #20. I have attached a new patch and an interdiff compared to the patch in #12.

The patch in #18 changed the value of $form_required_marker = array('#element'); I entirely remove the value. The patch in #18 also changed core/modules/file/file.field.inc. I think that was an accident.

@pplantinga:

Thanks for changing the status! The test bot seems to approve.

No idea why I added that snippet. Thanks for removing it.

Looks good to me... +1 (I'll let someone else mark it as RTBC since I wrote most of the patch).

Here.

Am I disqualified from marking it RTBC since I contributed the patch in #21? As @pplantinga said in #24, he did almost all the work.

I checked that the only difference between #21 and #26 is the change requested in #25.

+1 for RTBC.

Committed 2a1e147 and pushed to 8.x. Thanks!