#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

Files: 
CommentFileSizeAuthor
#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

Comments

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

Subscribe

yched’s picture

Status:Active» Needs review
StatusFileSize
new1.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

StatusFileSize
new1.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
StatusFileSize
new1.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

subscribe

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
StatusFileSize
new16.17 KB
new16.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
core/lib/Drupal/Core/Render/Element/Table.php:312

// 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?