#1164852-35: Inconsistencies in field language handling added a '#type = value' form element for the 'translatable' property in the "add new field" row.

This element, within a '#type = field_ui_table' form, gets interpreted as a new column cell in the table row, thus breaking the table layout (the row has one more cell than the other rows or the table heading).

Screenshot : http://drupal.org/files/add_new_extraneous_col.png

#11 manage_fields-1248940-11-before.png16.34 KBplach
#11 manage_fields-1248940-11-after.png16.17 KBplach
#7 manage_fields_table_layout-1248940-7.patch1.53 KByched
PASSED: [[SimpleTest]]: [MySQL] 33,637 pass(es). View
#4 manage_fields_table_layout-1248940-4.patch1.56 KByched
PASSED: [[SimpleTest]]: [MySQL] 33,646 pass(es). View
#3 manage_fields_table_layout-1248940-3.patch1.07 KByched
PASSED: [[SimpleTest]]: [MySQL] 33,649 pass(es). View
add_new_extraneous_col.png19.62 KByched
Members fund testing for the Drupal project. Drupal Association Learn more


plach’s picture

How is this supposed to be fixed? Should we count only "visible" elements as columns or move the '#translatable' property elsewhere?

andypost’s picture


yched’s picture

Status: Active » Needs review
1.07 KB
PASSED: [[SimpleTest]]: [MySQL] 33,649 pass(es). View

What's the point of this element to begin with ? I guess entity_translation form_alters it into translatable = TRUE ?

I'm not too fond of this dedicated form #value in core to explicitly support the contrib entity_translation case.
I guess this could be achieved without an explicit form element in core, just through form_set_value() in contrib validate or submit handlers, but this might not be too beautiful - and would require the core submit handler to take into account the possibility of contrib setting a value for the translatable property anyway.
And also entity_translation is one of those parts of core that didn't make it in time for core, so the special case is kind of acceptable.

So, hm, yes, I guess the best way out would be that the 'field_ui_table' element type supports non-cell children. Trying to think in terms of the generic 'table' element type we don't have but would like to (#991454: Add element #type table (with tableselect and tabledrag support)), it probably makes sense.

I considered checking for !empty(drupal_render($child_element)) before adding it as a cell. But the fact that a render comes out empty can depend on many things at runtime (e.g form_altered #access) and I don't think those runtime conditions should change the expected # of columns of a row - # of columns is a property that's expected to be consistent across the table, we can't make that change under your feet.

So just checking for '#type = value' children should be good enough for now. #type 'hidden' might need some special handling too, but we don't have those right now in the field_ui case, so let's not overcomplexify this.

yched’s picture

1.56 KB
PASSED: [[SimpleTest]]: [MySQL] 33,646 pass(es). View

Also added a comment about why we have a 'translatable' element as a '#type = value' element in the form.

plach’s picture

Well, the idea behind that value element is that any module can alter it. Previously every field property could be altered except for the translatable one, which was hard coded in the submit handler.

plach’s picture

Status: Needs review » Needs work
+++ b/modules/field_ui/field_ui.admin.inc
@@ -509,6 +514,9 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle
+      // Place the 'translatable' property as an explicit value so that the
+      // entity_translation contrib module can form_alter() the value for newly
+      // created fields.

IMO, we should have a more general comment, there is no reason to limit its scope to ET.

16 days to next Drupal core point release.

yched’s picture

Status: Needs work » Needs review
1.53 KB
PASSED: [[SimpleTest]]: [MySQL] 33,637 pass(es). View

Rephrased the comment.

plach’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good and fixes the issue. Since I don't think we need tests here, this should be ready to go.

bryancasler’s picture


catch’s picture

Status: Reviewed & tested by the community » Needs review

I'm OK committing this without tests, it's obscure and the extra column is a minor issue.

Could we get either tests or an 'after' screenshot though?

plach’s picture

Status: Needs review » Reviewed & tested by the community
16.17 KB
16.34 KB

Here is a couple of before/after screenshots.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I ran into this the other day with Drupal Commerce, and it was most irritating. Thanks for the fix!

Committed and pushed to 8.x and 7.x.

Automatically closed -- issue fixed for 2 weeks with no activity.

ivanjaros’s picture

Issue summary: View changes

I have an issue with empty table cells(values and hidden values) and the code in the table points to this issue in

// Turn second-level elements into table row columns.
// @todo Do not render a cell for children of #type 'value'.
// @see http://drupal.org/node/1248940

Is there a story for this or will this issue be reopened?

cburschka’s picture

Is there a story for this or will this issue be reopened?

This issue was fixed with a workaround in Field UI, which now automatically skips cells with #type=value. The TODO aims to put this into the Table element itself, but this hasn't been done yet (don't know if there's an issue for it).

For now, you'd have to duplicate the workaround from field_ui, which is still in D8 in Drupal\field_ui\Element\FieldUITable::preRenderRegionRows():

        // Render children as table cells.
        foreach (Element::children($element) as $cell_key) {
          $child = $element[$cell_key];
          // Do not render a cell for children of #type 'value'.
          if (!(isset($child['#type']) && $child['#type'] == 'value')) {
            $cell = ['data' => $child];
            if (isset($child['#cell_attributes'])) {
              $cell += $child['#cell_attributes'];
            $row['data'][] = $cell;