Show or hide the "Make field translatable" checkbox on the add field form depending on translatability

Needs patch :) looks good.

The reason is that the 2nd form is the field settings form. I think the difference between that one and the global settings in the main field form is that the field settings form is supposed to hold only values that cannot be changed. We could try to alter its behavior so that you can enter all global settings when creating the field.

Converted to Issue Summary Template standards

@plach: I think that would be great, I always found it odd that you could only set certain things AFTER the field was created.

Lukas: yeah, I think it applies to field settings in general. The field UI is painful in many ways. It does surface with the entity translation setup as well painfully.

It would be important to track for D8MI Usability, so let's keep the D8MI tags.

It feels very odd that translation_entity module only alters this Field UI form (whereas Field UI is not necessary enabled to begin with), but does not massage field [type] info at all. In fact, this feels a bit like D6-era drupal_execute().

I'm aware that this issue focuses on the UI, but I wanted to make sure to mention that somewhere.

Updated issue summary.

Next steps: re-evaluate if this is a closed (fixed) since #552604: Adding new fields leads to a confusing "Field settings" form is in.

If it's not closed, post the new current steps and screenshots (re-doing comment #1), and make a recommendation of what should be done.

It's fixed: the checkbox is there, although it may have a different default depeding on the bundle translatability (leaving open for this).

A patch to decide whether to show/hide field translatable setting depends on if the entity/bundle this field attached is translatable. If this entity/bundle is translatable, then we show the field's translatable setting, otherwise hide it.

Remaining tasks:
'admin/config/regional/translation_entity/translatable/' . $field_name, this path probably needs to redfine its access callback.
These fields' translatable setting is associated with the field itself, not with field instance. So, this setting could be un-reliable if a field is attached to more than one enity/bundle. Also, this field's translatable setting could be confused with settings here 'admin/config/regional/content-language'.
see http://drupal.org/node/1893568 and http://drupal.org/node/1893596

sorry, choosed the wrong status:(

This looks great!

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -653,30 +653,37 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+  // Only show field's translatable setting when entiy/bundle this field attached is translatable.

This comment is too long (it needs to wrap if it goes over 80 characters) and has a couple typos. Here's what I'd suggest:

Only show the field's translatable setting if the bundle is translatable.

Next let's add an automated test for this feature ensuring that the checkbox is shown or hidden appropriately when the bundle is translatable or not, including after making a change to the bundle. That will also help reviewers see if we are creating the correct behavior.

Also, what happens before this patch, if someone marks a field instance as translatable when the bundle is not?

Well, a field instance can be shared among different entities and bundles, which may be translatable or not. Hence there is no real consistency check atm.

Btw, the migration code is going away as soon as we complete the conversion to the entity field API, so I wouldn't waste time on it.

Steps to test.

1. enable content translation
2. add a language
3. without enabling translation on any content type, add a field to see if make translatable shows.

OH! it does show.

@plach, hi,
need concept 'field instance' clarification here? I think field instance is a specific field attached to bundle. So, the same field could be attached to different bundles, and we get different field instances for this field. A good example is the default field 'Body' for page and article delievered with drupal core.This field 'Body' has two instances at this point.

a field instance can be shared among different entities and bundles, which may be translatable or not. Hence there is no real consistency check atm.

I guess this a field instance can be shared among different entities is not right?

Also, this check/patch in #1831608-18: Show or hide the "Make field translatable" checkbox on the add field form depending on translatability is to detect the bundle translatability, and try to keep field instance translatability(not the 'instance' concept you mentioned, but mine opinion^^ ) consistency with the bundle that the field instance attached.

I suppose it could be a better idea to make field translatability become a private/instance setting, but not a global setting. see issue #1893568-2: Make each field instance have its own translatable property, instead of sharing translatable property with others.

It makes sense to me that we should add this check here. But you are right, even as we show or hide field setting here, this field's translatable property could still be be changed in other places, simply because this transalatable setting is a global setting and shared with all other instances.

a bit related #1893568: Make each field instance have its own translatable property, instead of sharing translatable property with others.

I think we might be confusing each other with field instance terminology, making it seem like we are disagreeing, but really we are saying similar things.

it might be correct to rephrase @plach in #22

Well, a field instance can be shared among different entities and bundles, which may be translatable or not. Hence there is no real consistency check atm.

as:

Well, a field can be shared among different entities and bundles, which may be translatable or not. Hence there is no real consistency check atm.

Yes, sorry, I meant just field. And I agree translatability should be per-instance, it'll make things simpler.

@plach in #1893568-1: Make each field instance have its own translatable property, instead of sharing translatable property with others. it sounds like berdir had concerns that it's not possible to make translatability per-instance. Can you provide an outline there for how to approach the problem?

I think this can proceed just checking if the bundle is translatable. ... which is done in #18

So I think the Next Steps here is to improve that patch according to the feedback in #21

Let me try to add the test:)

This test works fine for me.

However, even as we could hide field translatable setting from field UI, this setting is still available at path:
'admin/config/regional/translation_entity/translatable/$field_name'
because current design is to set each field's translatability, not field instance's translatability.

It looks like we have to resolve #1893568: Make each field instance have its own translatable property, instead of sharing translatable property with others. at first, then this fix here will make sense.

#31
It looks good, works as expected

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSettingsTest.phpundefined
@@ -119,4 +119,44 @@ protected function assertSettings($entity_type, $bundle, $enabled, $edit) {
+    // Create an extral field for content type article.
+    // At least one field needs to be translatable to enable article for
+    // translation. The extral field will be used for this purpose.

typo here "extral" also.. bit wordy.

suggestion (wrap it at 80 chars as needed):

+ // At least one field needs to be translatable to enable article for
+ // translation. Create an extra field to be used for this purpose.

---------

probably still needs a detailed code review.

when another patch is posted with fixes, include a test only patch version of that new patch. ([edit: added] and an interdiff to this patch)

Thanks for your review:)

I did a code review. Looks good, if the testbot says it's green.

I'm confused by the 'Users may translate this field.'. I worry that 'Users' may be interpreted as 'Visitors'. I seems like we should be a bit more specific. I know it is not part of this patch, but this seems like a good time to discuss it.

I looked for inspiration at:
admin/structure/types/manage/article comment settings and publishing options
admin/structure/types/manage/article/fields/field_image

Wording suggestions:

1. Translatable
2. Allow translation
3. Enable translation

My favorite: 3: Enable translation
It's also the same wording that appears as a link which leads to a batch process for stuff that already has content
not 1, because it's already translatable (or else the checkbox wont show)
not 2, because that confused with permissions

has tests.

autocomplete problem with tags. restoring (hopefully)

alternate suggestion:

Enable translation on this field

+1 for "Enable translation"

Enable translation

in tests and the module

I think that @Dries' focus was on "Users".

Shortening the entire checkbox label into "Enable translation" leads to a very techy checkbox label, as it doesn't really clarify the effect of the checkbox anymore. As a user, I'd ask myself: Will it automatically translate? Does "translation" actually refer to linguistic translation or some other translation mechanism I don't know of yet?

Furthermore, "Enable" on a checkbox label is almost always unnecessary word clutter, since the checkbox form element itself is "a control to enable or disable something." So a plain and simple "Translatable" would be better than "Enable translation".

After looking at the patch, I think we could also be a little bit more helpful in case the entity type/bundle is not enabled for translation yet. E.g., making the checkbox #disabled, and adding a short #description that provides a short hint along the lines of:

[/] Translatable
    To enable translation of this field, <a href="@language-content-url">enable language support</a> for this bundle.

Bundle? In user facing text?

This is what I'd like to see:

For "Enable translation"

Will it automatically translate?

This is a good point. 'Translatable' could be a better option. But "Enable translation" is still used in other places, even in patch #44. I guess we could use words "enable translatability" in other places, but use "Translatable" for the checkbox label?

The main inspiration for patch before #41 is to ensure field instance's translatability depends on bundle's translatability, so we decide whether to show or hide field translatable setting. Patch in #44 provides helpful description for the label, but it seems to break this original incentive again:(

+    if (field_has_data($field)) {
+      $toggle_url = url('admin/config/regional/translation_entity/translatable/' . $field_name, array(
+        'query' => drupal_get_destination(),
+      ));
+      $form['field']['translatable']['#description'] = t('This field has data in existing content. <a href="@toggle-translatable-url">@toggle-label</a>.', array(
+        '@toggle-translatable-url' => $toggle_url,
+        '@toggle-label' => !$translatable ? t('Associate all field values with a language') : t('Remove language association from all field values'),
+      ));
+    }

This part says if the field has data, the field's translatability could be enabled at other place, no matter the bundle is translatable or not.

+    elseif (!$bundle_is_translatable) {
+      $toggle_url = url('admin/config/regional/content-language', array(
+        'query' => drupal_get_destination(),
+      ));
+      $form['field']['translatable']['#description'] = t('To enable translation of this field, <a href="@language-settings-url">enable language support</a> for this type.', array(
+        '@language-settings-url' => $toggle_url,
+      ));

This part says if the field doesn't have data, the bundle must be translatable firstly, and then this field could be enabled too.

The two parts lose consistency for users. Aslo, a more serious problem is, no matter the field has data or not, and no matter the bundle is translatable or not, this field's translatability could still be enabled at 'admin/config/regional/translation_entity/translatable/$field_name'

I don't think we have to create a new path to enable/disbale field translatability depending on the field has data or not. At least, right now, this path mixes the settings. We could simply make this checkbox also available for field that has data, but it could break the rule:There is data for this field in the database. The field settings can no longer be changed.

So, here are two possible solutions:

One is to remove all the field translatable setting from UI, except 'admin/config/regional/content-language'. We leave only one central place for users to enable the translatability for both bundles and fields. User will see obviously he/she has to enable bundle translatability then, he/she could enable field translatability. See #1893596: Can we remove field's own translatable setting from UI?

The other one is to attach field translatable setting to form field_ui_field_edit_form, instead of field_ui_field_settings_form. Just like implementing hook_field_instance_settings_form, we create extra field translatable setting for field instance, instead of field itself.

In this way, we could keep the field translatable setting for field UI, and don't need that evil/redundant path for field has data or not.
It also makes sense to have field translatable thing become per-instance. However, this would need much more work to be done, like change property $field['translatable'] to $field_instance['trabslatable']. If this approach could work, I think we could merge this issue to #1893568: Make each field instance have its own translatable property, instead of sharing translatable property with others., and wait for that issue be finished first.

I think the new ideas since #42 should be a separate issue...

Well, it's a bit long, but among other ideas, @smiletrl raises a few good points in #46.

Before this patch, the field settings form always exposed a control, but which actually isn't always available (due to two constraints elsewhere). The proposed solution of this issue was to hide the control entirely, if the preconditions are not met.

I think that's confusing. Because, sometimes, when I visit this settings form, the control would appear. And some other times, it would not. There wouldn't be any kind of indication for why, and no hint at all for how to get the control "back".

This is the main aspect I tried to fix in #44 — instead of hiding, provide a concrete hint and clue for why the control is not available currently.

Additionally, I had to dig really deep into the code to figure out what exactly the difference between the "Enable translation" checkbox and the "Enable translation" link (+ confirmation + batch operation) is. The latter actually is a misnomer, since it doesn't just enable any translation, but instead, manipulates and updates all existing entities and field data values to have an explicit language assigned (and apparently, that's a dangerous operation that may lead to data loss, which is mentioned in code comments, but not at all in the UI).

It is problematic to have two completely different operations with the same label, especially if one of them isn't guaranteed to be safe. That's why I reworded the labels of the links in #44, also clarifying what that operation is actually doing.

The main problem from my perspective is that this control requires multiple preconditions to be met, in order to be available and functional. If the preconditions aren't met, then they can be changed — but as a user, I don't know how and where. (I did not know this either, I had to study a bit of code to figure it out.)

Therefore, this precondition situation is not as simple as the typical case of, say, "Why is there no Preview button on my comment form?" — "D'oh, sorry, wasn't enabled." — Instead, due to the multiple layers and conditions, it requires concrete hints/guidance.

The only other option I can see would be to remove the checkbox entirely from the field settings form. Doing so inherently removes the user expectation that there can be a checkbox in the first place. Instead, translatability would have to be configured in a separate and central spot only. (However, upfront, I'm not a fan of that general direction, since I dislike the idea of having to perform bulk/mass configuration changes to everything on my site when in fact only a single field should be configured.)

One thing I'd like to outline is that the data migration and the related route are going away as soon as we complete the ng conversion: once we are done the field values corresponding to the entity language will be always stored with the 'und' language code. This should solve part of the issues we are having here.

I looked to see if we had a issue tracking the batch processing that happens when translation is enabled on content that already has data.
I looked in #1188388: Entity translation UI in core and #1836086: [meta] Entity Translation UI improvements but I didn't see anything exactly to address that. I imagine that issue would be postponed on one of the NG conversion issues.

maybe #1818580: [Meta] Convert all entity types to the new Entity Field API or #1732730: [meta] Implement the new entity field API for all field types

I'm going to update the issue summary with my best guess as to other issues this is kind of waiting on that will simplify this issue. If there are better issues to reference, or if we need one (like fix the batch translation enable once NG gets in), please make a comment.

Updated issue summary next steps

Issue summnary update

Working on this issue at the Portland 2013 sprint.

tried to identify issues we are waiting on that will simplify this issue. -c

updated issue summary

Some reordering, corrections and linking the cited comments.

@ kmharrell
please don't post issue updates as comments!
If you have a look on the top of the issue, there should be an "edit" link below the issue title.
Then you have to type in a change notice and all the changes on the issue summary will be revisioned.
(please edit you previous comment and make it empty so that this issue doesn't get too long!)
Also remove the "Needs issue summary update" tag after you updated the issue summary

Thanks kmharrell, I reorganized the issue summary a bit and linked the comments. For now we can remove the tag.

Not working on the issue for some months now - then better unassign.

Needs reroll

Re-rolled the patch. Patch gets applied properly now. Attaching the re-rolled patch here.

Unfortunately does not pass anymore.

Something has gone wrong in this latest reroll. It appears as though the site-specific configuration has been included in the patch. Try reading through the reroll guide and see if that helps, https://drupal.org/patch/reroll

I've unassigned this issue since the patch is almost 2 months old. @piyuesh23, if you would like to still work on this reassign it back to yourself.

Same as #58 but without the unrelated stuff.

rerolled - there were some changes here and there, but wasn't able to get a decent interdiff.

And test fix - I changed the test also a little as the asserts didn't make any sense to me - feel free to correct me of course.

"Translatable" seems to me to be a very cryptic label for a checkbox and lacks a verb for the action you are deciding about. @sun objected to "Enable" because the presence of a checkbox suggests you are enabling or disabling anyway. So how about "Allow this field to be translated" - it is about the same length as the original ("Users may translate this field") but does clearly state what you are being asked to decide.

minor fix in the OP

I think "Translatable" is as simple as it can get, and "Allow this field to be translated" is not clearer. I think the checkbox before "Translatable" clearly indicates that field translation is either turned on or off. This is the field settings form. I don't think repeating things like "this field" is helpful. All other settings are for "this field".

Not sure how it plays with what we are doing here, but in #2076445: Make sure language codes for original field values always match entity language regardless of field translatability we are removing the field data migration as it will be always possible to switch field translatability.

@plach: my understanding is even if the storage keying changes on fields, if a field was multilingual and is turned off, the data on different languages will not be accessible anymore. So similar logic will be needed but the wording will be slightly different.

IMHO that is a classic case of whichever patch lands later need to accommodate the change.

Sounds sensible

Add on sprint for easier tracking.

66: drupal8.translatable-field.66.patch queued for re-testing.

Presumably due to #2076445: Make sure language codes for original field values always match entity language regardless of field translatability, this no longer applies.

Re-rolling #66

Rerolled #76

Important chunks of code in #66 were missed in the rerolls.

And now removed the checks for $field->hasData().

As #69 and #70 explain, there is no more field data migration so switching field translability is not dangerous anymore (#2076445: Make sure language codes for original field values always match entity language regardless of field translatability).

Looks good to me, do you agree it looks good @plach?

Looks good :)

+++ b/core/modules/content_translation/content_translation.module
@@ -816,13 +816,27 @@ function content_translation_field_extra_fields() {
+    '#disabled' => !$bundle_is_translatable,

I might be wrong, but AFAIK #disabled does not actually prevent the form element from being submitted. I.e. if you hack the HTML to remove the 'disabled' attribute you can still check the checkbox and it will be saved. This could be prevented by (conditionally) setting '#value' on the element.

You are right.

Actually no. if the disabled property is set, user input will be ignored.

- edit - if the form api property is set, not some class or anything of course. Look in FormBuilder.php, line 1543 for the actual code and processing. This has been a feature of Form API since the very beginning.

Awesome, this looks like a great little UX improvement.

Committed and pushed to 8.x. Thanks!

Woot, thanks! Good to see this is finally done :)

Removing from sprint.